1085182 - Bugzilla::Bug->check must check that a bug ID is defined when it gets a hashref

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[John Lightsey]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; Touch; .NET4.0E; .NET4.0C; Tablet PC 2.0; .NET CLR 3.5.30729; .NET CLR 2.0.50727; .NET CLR 3.0.30729; rv:11.0) like Gecko

Steps to reproduce:

Hi there,

I'm marking this as security in case you folks see any of these issues connecting up as a vulnerability in ways that I don't see it working. Feel free to chunk this bug if it has no value to you. It just seemed silly not to pass this along. I spent quite a few hours going over the 4.4.6 version and these are all issues that are trivial vulnerabilities or near-vulnerabilities that you might want to address.

1) It's possible to get a SQL injection all the way into DBI before DBI's taint checking traps it. It's possible that switching between the various database backends will make the SQL injection work or that there may be some way to untaint the injection string I've experimented with quite a few approaches, but none seems to work. DBI's documentation suggests that TaintIn isn't guaranteed to be reliable, but I haven't found any way around it. The payload for the xmlrpc.cgi interface is:

<methodCall>
 <methodName>Bug.update</methodName>
 <params>
  <param>
   <value>
    <struct>
     <member>
      <name>ids</name>
      <value>
       <array>
       <data>
       <value>
       <struct>
         <member><name>condition</name><value><string>sql_injection_goes_here</string></value></member>
        <member>
         <name>values</name>
         <value><array><data><value><int>2</int></value></data></array></value>
        </member>
       </struct>
       </value>
       </data>
       </array>
      </value>
     </member>
     <member><name>Bugzilla_login</name><value><string>[e-mail]</string></value></member>
     <member><name>Bugzilla_password</name><value><string>XXXXX</string></value></member>
    </struct>
   </value>
  </param>
 </params>
</methodCall>

The problem basically boils down to...when you're taking an ID as input, make sure it's actually an ID since Bugzilla::Object->new can't tell the difference.

2) In the XML and JSON APIs the dispatching system uses the module namespace to limit the available API methods. Bugzilla also uses exporter extensively, so this results in namespaces having many inappropriate methods available. Out of the methods available this way in 4.4.6, User.bz_locations and Bug.WS_DISPATCH, stand out as the ones that could be argued to return sensitive internal data. I'm not a big fan of path disclosure issues being treated as a vulnerabilities, but YMMV.

Using a whitelist of allowed method names for the XML/JSON API namespaces would probably be a better approach. If that is not possible, avoid importing any functions into these namespaces...add the empty list to your "use" statements and qualify all function/method calls that go outside of the module with their full namespace.

Instead of:

use Bugzilla::Constants;
bz_locations();

you write

use Bugzilla::Constants ();
Bugzilla::Constants::bz_locations()

3) The email_in.pl script has no meaningful authentication checks. It only does authorization checks. I was going to send this in as a vulnerability report since attacking it seems straightforward, but then I noticed that the Bugzilla documentation shifts responsibility for authentication here to the system mail server. This seems like an unrealistic suggestion. The strength of a Bugzilla server's authentication is limited by the strength of the SPF/DKIM configured for every admin's email address.

I didn't dig into this much further since the documentation makes it fairly clear that it's not expected to be safe without the system SMTP daemon being bulletproof in the mail it accepts. I would expect an attacker to focus on this aspect of Bugzilla though. A fake "From" header here looks sufficient to edit all bugs in the system. The attacker doesn't need to intercept any mail sent to the account he's pretending to be.


Overall though, the codebase looks nice. The CGI scripts do most of their validation at the outer edges of the attack surface, output filtering happens at the outer edges where content gets displayed, Bugzilla has taint checking turned on and isn't trying to defeat it blindly, the modules are fairly well organized, AuthN/AuthZ logic is centralized and reasonably straightforward. It's hard to find any really glaring issues.

Kudos to the Bugzilla team.

Comment 1

[Frédéric Buclin]

(In reply to John Lightsey from comment #0)
> 1) It's possible to get a SQL injection all the way into DBI before DBI's
> taint checking traps it.

I'm not sure I understand how you managed to do this. I already tried to play with 

>  <methodName>Bug.update</methodName>
   [...]
> <member><name>condition</name><value><string>sql_injection_goes_here</
> string></value></member>

in the past, without success. Do you have a PoC which works?



> 3) The email_in.pl script has no meaningful authentication checks.

We make the documentation pretty clear about this, so this is not a vulnerability, and email_in.pl is disabled by default. To enforce security, we already have bug 419203 and bug 943052.

Comment 2

[John Lightsey]

For #1, the payload is to get into Bugzilla::Webservice::Bug::update() with $params->{ids} of [{condition => "sql_injection", "values" => [1]}]

That splits the array into what it thinks are individual bug numbers (but are actually hashes of {condition => "sql_injection", "values" => [1]} since it doesn't do proper validation), then sends them into Bugzilla::Bug::check_for_edit(), which forwards them to Bugzilla::Bug::check(). Here, I'd expect trim() to stop it, but it, but it never did so in my testing, and it gets passed to Bugzilla::Bug::new(), which falls back to Bugzilla::Object::new() and Bugzilla::Object::_init() with $param set to {condition => "sql_injection", "values" => [1]}. Inside _init() it uses the value of the "condition" key as the where clause when loading trying to load the object from the database.

The response I was getting back from both the JSON and XML interfaces before I upgraded my test server to 4.5.6 was an error string from DBI about the where clause being tainted.

I can set up another 4.4.6 install and retest, but I couldn't get past the taint checking anyway, so it seems much simpler to just fix the input validation in Bugzilla::Webservice::Bug::update() to eliminate the problem entirely. The Bugzilla CGI entry points seem to do a better job of asserting that inputs are the correct datatypes than the XML/JSON RPC interfaces do. A simple fix for this particular input field would be something like switching Bugzills::Webservice::Bug::update() to assert that the IDs are numbers:

my @bugs = map { my $id = $_; Bugzilla::Bug->check_for_edit(detaint_natural($id)) } @$ids;

(The detaint functions are generally used on copies of the input data that have been pulled off the input hash...this definitely helps since it makes detainting the condition value in the input hash very difficult, maybe impossible. I tried to find any codepaths that would detaint parts of the input hash directly so that I could trick Bugzilla into detainting the SQL statement for me, but this programming style kept saving the day.)

My assumption in mentioning this is that taint checking is intended to be the last line of defense, not the first. The injection gets into DBI before it's stopped. When I audit code I generally try to second-guess assumptions like this (that an external module will do the right thing at all time and in all versions) since it's a dangerous assumption and sometimes wrong. At my day job I try to stress to the other developers that they should push the input validation and output escaping towards the edges of the codepath like the older CGI interfaces are doing.... It makes the code safer when the layers are very clear.

- validate input datatypes
- validate business logic constraints
- perform work
- escape for the output context
- output

The risk of relying on DBI's taint checking is that it happens in the middle of the workflow and it's importance won't be apparent to someone else modifying Bugzilla::Object::_init(). Looking only at that function, it's not at all apparent that the "condition" clause is going to be attacker-controllable. Anyway...I don't want to get all preachy...I do that enough at work and at YAPC.

For #3, yes, 419203 is exactly what I would expect a "fix" to look like. And since the design is documented this way, it's not a flaw in the Bugzilla software. I can't test the email handling of specific servers without directly attacking the systems, so I can't prove that this a vulnerability with individual deployments of Bugzilla. An attacker would not care about these issues though, and a sysadmin deploying Bugzilla is unlikely to understand them.

My day job is leading a security team and I certainly understand that a system that works as it is designed, where the documentation is correct about the risks associated with using it as designed, does not constitute a vulnerability in the implementation. The unfortunate reality is that the math works out in a different direction for "attackers" and "defenders" than it does for "builders". I'm not at all suggesting you treat this as a vulnerability...I wouldn't treat it as a vulnerability in my software. I would, however, be nagging regularly to get it fixed so that the design better fits the expectations of the folks that deploy the software. The odd reality about design flaws like this is that they are simple to forgive and overlook until someone starts actively attacking them. Again though, I'm getting too preachy.

Comment 3

[Frédéric Buclin]

(In reply to John Lightsey from comment #2)
> For #1, the payload is to get into Bugzilla::Webservice::Bug::update() with
> $params->{ids} of [{condition => "sql_injection", "values" => [1]}]

Ah, this trick is correctly caught in Bugzilla 5.0, and so 5.0 is not vulnerable. In 4.4.6, you indeed get "Insecure dependency in parameter 1 of DBI::db=HASH(0xa6b56c0)->selectrow_hashref method call". So this is not exploitable, but this invalid input should be caught much earlier, I agree.


About your point #2, you indeed can call methods which are not supposed to be called. You can e.g. also call Bug.quicksearch, or Bug.get_legal_field_values which causes (unexploitable) SQL injection. I agree we have to whitelist valid methods, else we are going to shoot ourselves in the foot sooner or later.

Comment 4

[Frédéric Buclin]

dkl: want to fix this bug? See points 1 and 2 in comment 0.

Comment 5

[:glob ✱]

as points 1 and 2 are completely different issues, please split #2 into a new bug for tracking and development.

this bug should be used to track the issue that we don't throw a nice error message (which imho isn't a security issue).

Comment 6

[Frédéric Buclin]

Attachment: patch for 4.4, v1

In 4.4 and older, Bugzilla::Bug::check() assumes that it always gets a scalar. This bad assumption leads to the error described in this bug. I backported the code from 5.0/master.

Comment 7

[Frédéric Buclin]

Attachment: patch for 4.0 and 4.2, v1

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8539807 [details] [diff] [review]
patch for 4.4, v1

Review of attachment 8539807 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 8539843 [details] [diff] [review]
patch for 4.0 and 4.2, v1

Review of attachment 8539843 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 10

[:glob ✱]

a=glob (clearing redundant blocking requests)

Comment 11

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   6fde3ca..9e8c557  4.4 -> 4.4

To ssh://[e-mail]/bugzilla/bugzilla.git
   3a5ed67..7195166  4.2 -> 4.2

To ssh://[e-mail]/bugzilla/bugzilla.git
   9f1c4fa..5fde851  4.0 -> 4.0

Comment 12

[Frédéric Buclin]

Due to the data contained in comments 0 - 3, this bug should not be made public till bug 1090275 is fixed.