1230932 - Providing a condition as an ID to the webservice results in a taint error

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Netanel Rubin]

Attachment: Bugzilla - SQL Injection.pdf

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.86 Safari/537.36

Steps to reproduce:

Hello,

My name is Netanel Rubin, I work as a vulnerability researcher at PerimeterX.

This is a critical vulnerability report for an issue I discovered in the Bugzilla platform. The successful exploitation of the vulnerability allows an attacker to successfully exploit an SQL Injection flaw assuming Taint Mode is disabled at the vulnerable script.

As a PoC, I've tested the vulnerability on your installation. It appears you have disabled Taint on jsonrpc.cgi (and possibly other pages), as the attack succeeded and I've managed to execute any SQL statement I wanted under a SELECT query.

I'm attaching the complete vulnerability report to this bug, as I learned from past experience this is your preferred method of communication.
Please assign a CVE number for this issue. We would also like to coordinate the public disclosure with you.

Best regards,
Netanel.

Comment 1

[Frédéric Buclin]

Thanks for the report! Disabling taint mode is not an option in Bugzilla. It's enabled for all CGI scripts on purpose, defeating your attack. You mention bug 1186416, but this bug is specific to BMO, not something which should be ported upstream. Your PoC is exactly the reason why taint mode should not be disabled, despite what some developers discussed during a meeting this summer.

Due to the taint mode, this bug is not a security vulnerability. But this error should be fixed anyway:

Insecure dependency in parameter 1 of DBI::db=HASH(0xb22e888)->selectrow_hashref method call while running with -T switch at Bugzilla/Object.pm line 156.


If BMO is not affected (i.e. they didn't disable taint mode), then the security flag can be removed.

Comment 2

[Netanel Rubin]

Thanks for the response Frédéric.

It's important to note Mozilla is not the only one to disable Taint on their Bugzilla installation - it's a common procedure done due to many different reasons, mainly because of plugin compatibility. This is why I recommend keeping the bug as a security related bug, at least until a patch is available.
In any case, as I explained in the report, Taint should never be trusted, just like WAFs and other language specific safe modes.

It is also important to note BMO *is* affected. As I mentioned in the OP, I tested the vulnerability specifically on it and it worked, allowing me to dump the content of the entire DB.

Comment 3

[:glob ✱]

as far as i can tell BMO still has taint mode enabled (note bug 1186416 is not marked as fixed), and we cannot reproduce this issue against bugzilla.mozilla.org (it results in an internal-server-error, as a result of the taint failure).

netanel, do you have code that functions against bugzilla.mozilla.org?

Comment 4

[Netanel Rubin]

I can help you reproduce it, it's a little tricky though.

First, you have to create a new user for this to work on BMO (I've no idea why).
Then, you can use the following JSON request as a PoC:
{"method":"Bug.update_attachment","params":[{
"Bugzilla_login": "NEW_USERNAME",
"Bugzilla_password": "NEW_PASSWORD",
"ids": [{"condition":"5=?","values":["SOME_VALUE"]}]
}]}

You can send the JSON request as many times as you'd like and as long as SOME_VALUE won't be equal to "5" (the value needed to make the condition return true in this example) you'll get an internal error. BUT, the moment you send "5", you'll get an 200 OK response from the server, proving the SQL injection has worked.

Now, this is where it gets weird.
Any request sent *after* one successful exploitation of the vulnerability, as in getting a "200 OK" response from the server, will return an internal error, even if it's the EXACT same request.
We suspected it was a WAF thing, but of course there is no way for us to know for sure.

I actually have screen shots proving the whole thing, but I'm sure they will not be necessary.

Comment 5

[Frédéric Buclin]

But do you actually get the data back? Or do you just see a 200 OK response?

Comment 6

[Netanel Rubin]

200, making this a blind SQLI.

Comment 7

[:glob ✱]

thanks.

i see an intermittent http/200, however it always has content-length of 0 and there's still a corresponding error in the apache logs:

POST https://bugzilla.mozilla.org/jsonrpc.cgi
Content-Length: 167
Content-Type: application/json

{"method":"Bug.update_attachment","params":[{"Bugzilla_login": "-","Bugzilla_password":"-","ids": [{"condition":"5=?","values":["5"]}]}]}

HTTP/1.1 200 OK
Connection: close
Date: Mon, 07 Dec 2015 17:42:40 GMT
Server: Apache
Content-Length: 0
Content-Type: text/plain; charset=UTF-8
Client-Date: Mon, 07 Dec 2015 17:42:40 GMT
Client-Peer: 63.245.215.80:443
Client-Response-Num: 1
Client-SSL-Cert-Issuer: /C=US/O=DigiCert Inc/OU=www.digicert.com/CN=DigiCert SHA2 Extended Validation Server CA
Client-SSL-Cert-Subject: /businessCategory=Private Organization/1.3.6.1.4.1.311.60.2.1.3=US/1.3.6.1.4.1.311.60.2.1.2=California/serialNumber=C2543436/street=650 Castro St Ste 300/postalCode=94041/C=US/ST=California/L=Mountain View/O=Mozilla Foundation/CN=bugzilla.mozilla.org
Client-SSL-Cipher: ECDHE-RSA-AES128-GCM-SHA256
Client-SSL-Socket-Class: IO::Socket::SSL
Keep-Alive: timeout=5, max=1000
X-Backend-Server: web5.bugs.scl3.mozilla.com

09:42:40 #5 [error] malformed JSON string, neither array, object, number, string or atom, at character offset 0 (before "(end of string)") (Bugzilla/WebService/Server/JSONRPC.pm:319)

this is the same error logged when we return HTTP/500.

Comment 8

[Frédéric Buclin]

Well, this makes a big difference. Not getting the data back and not being able to alter the data is much less worse than getting the DB dump you mentioned earlier.

Comment 9

[:glob ✱]

my guess at this point is the http/200 return is erroneous; we're actually failing, then failing to fail correctly.  the investigation continues.

Comment 10

[Netanel Rubin]

Frederic, blind SQLI is just as severe an a regular SQLI. I could have easily extracted the admin password/activation token/other critical info.

Regarding the 200/500 issue, note that "Malformed JSON string" is returned when an uncaught exception occurs anywhere in the code. It could be a Taint exception, an SQL problem, and really any other reason for the code to fail when it gets an unexpected hash instead of a normal integer.
This does not matter for the vulnerability, though, as the attacker still gets an indication whether his condition was true or not, allowing him to exploit to SQLI regardless.

Comment 11

[Frédéric Buclin]

(In reply to Netanel Rubin from comment #10)
> Frederic, blind SQLI is just as severe an a regular SQLI. I could have
> easily extracted the admin password/activation token/other critical info.

Which admin password? Which activation token? Bugzilla has no such things, unless we don't talk about the same thing. If you have such a PoC, please share. This would help to track how the data is being sent and returned to the user.

Comment 12

[:glob ✱]

Attachment: 1230932_1.patch

as far as we can tell this an issue, but here's a possible fix.

Comment 13

[:glob ✱]

(In reply to Byron Jones ‹:glob› (Unavailable until 4th Jan) from comment #12)
> as far as we can tell this an issue, but here's a possible fix.

sorry, jetlagged..

as far as we can tell this _isn't_ an issue.

Comment 14

[Frédéric Buclin]

Yeah, I also had such a similar fix in mind. I will have to check each WS method to make sure that 1) they all call validate() before anything else, and 2) we don't pass other foo_ids to Bugzilla::Object, such as bug_ids or comment_ids. It's a pity we cannot let Bugzilla::Object detect it's originally being called from a WS method and let it block condition/values itself. Another way to fix it would be to pass a new argument to new() (and match()?) to explicitly allow the use of condition/values. Something like:

Bugzilla::Foo->new($params, ALLOW_CONDITIONS)

But I suppose this would be a separate RFE for master only.

Comment 15

[Netanel Rubin]

(In reply to Frédéric Buclin from comment #11)
> (In reply to Netanel Rubin from comment #10)
> > Frederic, blind SQLI is just as severe an a regular SQLI. I could have
> > easily extracted the admin password/activation token/other critical info.
> 
> Which admin password? Which activation token? Bugzilla has no such things,
> unless we don't talk about the same thing. If you have such a PoC, please
> share. This would help to track how the data is being sent and returned to
> the user.

I feel like there's some kind of miss-communication here.
If you're not familiar yet with the subject, Blind SQLI means the SQL statement output isn't completely available. In our case, it means an attacker can tell whether a statement has returned a row or not, or in other words, whether the statement's condition has returned true or false. This can be exploited in many different ways, but in our case a simple 'substring()' call should do the trick.

For example, if I'll inject this query into the 'condition' parameter after I've requested a password reset for userid 1:
substring((SELECT token FROM tokens WHERE userid=1 and tokentype='password',1,1)=SOME_CHAR
I could guess the first character of the token, and, of course, any other character afterwards, as I see whether that statement return true or false based on the server response number (200=true, 500=false).

This does mean I'll have to send more requests, yes, but as the password reset token is 10 characters long I could obtain it rather quickly, something a simple python script could accomplish.

Of course, I haven't tried doing it myself, as I don't want to breach anyone's privacy. Instead, I executed a simple true/false statement - a classic "1=?" condition, and it worked perfectly fine on your installation.

Comment 16

[Frédéric Buclin]

(In reply to Netanel Rubin from comment #15)
> Of course, I haven't tried doing it myself, as I don't want to breach
> anyone's privacy. Instead, I executed a simple true/false statement - a
> classic "1=?" condition, and it worked perfectly fine on your installation.

To be sure, can you reproduce this attack against https://landfill.bugzilla.org/bugzilla-tip/ and/or https://landfill.bugzilla.org/bugzilla-5.0-branch/? Both are test installations.

Comment 17

[:glob ✱]

i spent quite a bit of time investigating this issue.  this problem does _not_ exist on bmo - it is always caught by taint checks so even though you can inject conditions into the sql, they are never executed by the database.

it's still a problem that we should address, but it isn't a critical security issue.

it appears the intermittent http/200 responses do not indicate that the request was executed successfully.  at this stage it looks like an unexpected behaviour/interaction with the load balancer in front of bugzilla.mozilla.org.  i will dig deeper into this tomorrow.

Comment 18

[:glob ✱]

the http/200 responses are definitely not an indication of a sqli; tracking that issue in bug 1231918 (it's a bmo infra issue).

Comment 19

[Frédéric Buclin]

Per the investigation above, this is not a security bug.

Comment 20

[Frédéric Buclin]

Comment on attachment 8696521 [details] [diff] [review]
1230932_1.patch

Affected methods (tested with XML-RPC):

Bug.update_attachment    with: ids        => [{condition => '1 = 1', values => []}]
Bug.update_comment_tags  with: comment_id => {condition => '1 = 1', values => []}

The second case is not caught by your patch.


All the WS methods are very lenient about input data. They simply make sure that mandatory parameters are defined, but they do not check that parameters (both mandatory and optional) are of the expected format (i.e. strings or integers, most of the time). This is out of the scope of this bug, but we should really improve validators in Bugzilla/WebService/*.pm. That's IMO not the job of Bugzilla/*.pm to do these checks as they are supposed to be used internally only, where the data format is not controlled by users. So it's the job of the API to pass correctly formatted data to internal methods.

Comment 21

[Frédéric Buclin]

This problem isn't specific to JSON-RPC.

About your patch, I think validate() should be even more agressive and throw an error if 'ids' isn't an array of integers, instead of silently removing 'condition' and 'values'. Because if one day we extend Bugzilla::Object->new() to accept new parameters, we would have to fix validate() too, which we will very likely forget to do.

Comment 22

[Dylan Hardison [:dylan] (he/him)]

dkl: since glob is on vacation until the 4th, should I take over as assignee of this bug and address the review concerns in comment 20 and comment 21?

Comment 23

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #22)
> dkl: since glob is on vacation until the 4th, should I take over as assignee
> of this bug and address the review concerns in comment 20 and comment 21?

Yes please. Probably good to deal with this sooner rather than later.

Dkl

Comment 24

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1230932_3.patch

Comment 25

[Frédéric Buclin]

Comment on attachment 8699052 [details] [diff] [review]
1230932_3.patch

>+++ b/Bugzilla/WebService/Util.pm

>+    # Don't allow the id params to contain things other than ints.
>+    foreach my $id_param (qw( ids comment_ids )) {
>+        if (exists $params->{$id_param} && ref $params->{$id_param}) {

I don't think we should blindly validate ids and comment_ids everytime validate() is called. We should only validate them if they are part of @keys. So you should merge your code with the existing foreach loop. As a bonus point, you would already know that the param exists and is a ref.

Comment 26

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1230932_4.patch

Comment 27

[Frédéric Buclin]

Comment on attachment 8700593 [details] [diff] [review]
1230932_4.patch

>+++ b/Bugzilla/WebService/Util.pm

>+use List::MoreUtils qw(all);

You must also list 'any'.


>+++ b/template/en/default/global/code-error.html.tmpl

>+  [% ELSIF error == "param_integer_required" %]
>+    The function <code>[% function FILTER html %]</code> requires
>+    that <code>[% param FILTER html %]</code> be an integer and it is not.
>+
>+  [% ELSIF error == "param_integer_array_required" %]
>+    The <code>[% param FILTER html %]</code> parameter must be an array of integers and it is not.

Both error codes must be listed in WS_ERROR_CODE in Bugzilla/WebService/Constants.pm.
Nit: also, "and it is not" is useless, because if we throw these errors, then the user already knows they are not.

Comment 28

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1230932_5.patch

Comment 29

[Frédéric Buclin]

Comment on attachment 8700608 [details] [diff] [review]
1230932_5.patch

r=LpSolit

Comment 30

[Dylan Hardison [:dylan] (he/him)]

Attachment: 4.4 patch

Comment 31

[David Lawrence [:dkl]]

Comment on attachment 8700646 [details] [diff] [review]
4.4 patch

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

::: Bugzilla/WebService/Util.pm
@@ +116,5 @@
>                                : [ $params->{$key} ];
> +
> +            if (any { $key eq $_ } @id_params) {
> +                my $ids = $params->{$key};
> +                ThrowCodeError('param_integer_array_required', { param => $key })

Undefined subroutine &Bugzilla::WebService::Util::ThrowCodeError called at Bugzilla/WebService/Util.pm line 121.

Comment 32

[Dylan Hardison [:dylan] (he/him)]

Attachment: 4.4 2nd patch

Comment 33

[David Lawrence [:dkl]]

Comment on attachment 8700660 [details] [diff] [review]
4.4 2nd patch

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

r=dkl

Comment 34

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   0cd77b4..eb1357f  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   6a24138..396ae88  5.0 -> 5.0

To ssh://[e-mail]/bugzilla/bugzilla.git
   3e0ed9c..fc5cdf3  4.4 -> 4.4

Comment 35

[Dylan Hardison [:dylan] (he/him)]

This unfortunately breaks the "alias" feature. This won't make it into the current release.

Comment 36

[Frédéric Buclin]

Attachment: patch for master, v2

No need to check if ids are integers specifically. Validators will already do this check for us (and so bug aliases will still work). We simply need to make sure that no hashrefs or any other evil data is passed in.

Comment 37

[David Lawrence [:dkl]]

Comment on attachment 8726952 [details] [diff] [review]
patch for master, v2

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

Otherwise looks fine and works as expected.

::: Bugzilla/API/1_0/Util.pm
@@ +22,4 @@
>  use Storable qw(dclone);
>  use Test::Taint ();
>  use URI::Escape qw(uri_unescape);
> +use Bugzilla::WebService::Util qw(validate);

I would rather not do this. I realize it is extra duplication, but when we do finally remove XMLRPC/JSONRPC, it will be so much easier to just remove the old files. Otherwise we will need to do the copying at that time anyway when the tests start to fail.

Comment 38

[Frédéric Buclin]

Attachment: patch for master, v3

Comment 39

[David Lawrence [:dkl]]

Comment on attachment 8728618 [details] [diff] [review]
patch for master, v3

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

r=dkl

Comment 40

[Frédéric Buclin]

Attachment: patch for master, v3.1

I just realized that I forgot to fix update_comment_tags() for REST too.

Comment 41

[Frédéric Buclin]

Attachment: patch for 5.0, v1

If I understand the REST code in 5.0 correctly, it doesn't need to be fixed, because it calls the same methods as XMLRPC and JSONRPC themselves, i.e. WebService/*.pm.

Comment 42

[David Lawrence [:dkl]]

Comment on attachment 8728725 [details] [diff] [review]
patch for master, v3.1

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

r=dkl

Comment 43

[David Lawrence [:dkl]]

Comment on attachment 8728727 [details] [diff] [review]
patch for 5.0, v1

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

r=dkl

Comment 44

[Frédéric Buclin]

Both Bug.update_comment_tags and Bug.update_attachment only exist since 5.0. I couldn't find any place in the 4.4 code which could trigger this taint issue. All my attempts to abuse Bugzilla were correctly caught by Bugzilla::Object->check. So I won't commit my patch there. This will also avoid any unexpected regression.

Comment 45

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   6da063a..0cac98d  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   1c5ecdf..6e0182e  5.0 -> 5.0