Closed
Bug 1230932
Opened 9 years ago
Closed 9 years ago
Providing a condition as an ID to the webservice results in a taint error
Categories
(Bugzilla :: WebService, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 5.0
People
(Reporter: nati, Assigned: LpSolit)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 9 obsolete files)
5.79 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
3.24 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•9 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 2•9 years ago
|
||
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.
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?
Flags: needinfo?(nati)
Reporter | ||
Comment 4•9 years ago
|
||
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.
Flags: needinfo?(nati)
![]() |
Assignee | |
Comment 5•9 years ago
|
||
But do you actually get the data back? Or do you just see a 200 OK response?
Reporter | ||
Comment 6•9 years ago
|
||
200, making this a blind SQLI.
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.
Summary: WebService SQL Injection → WebService Blind SQL Injection
![]() |
Assignee | |
Comment 8•9 years ago
|
||
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.
my guess at this point is the http/200 return is erroneous; we're actually failing, then failing to fail correctly. the investigation continues.
Reporter | ||
Comment 10•9 years ago
|
||
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.
![]() |
Assignee | |
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
as far as we can tell this an issue, but here's a possible fix.
Attachment #8696521 -
Flags: review?(LpSolit)
Comment 13•9 years ago
|
||
(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.
![]() |
Assignee | |
Comment 14•9 years ago
|
||
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.
Reporter | ||
Comment 15•9 years ago
|
||
(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.
![]() |
Assignee | |
Comment 16•9 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
the http/200 responses are definitely not an indication of a sqli; tracking that issue in bug 1231918 (it's a bmo infra issue).
Summary: WebService Blind SQL Injection → Providing a condition as an ID to the jsonrpc webservice results in a taint error
![]() |
Assignee | |
Comment 19•9 years ago
|
||
Per the investigation above, this is not a security bug.
Group: bugzilla-security
![]() |
Assignee | |
Comment 20•9 years ago
|
||
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.
Attachment #8696521 -
Flags: review?(LpSolit) → review-
![]() |
Assignee | |
Comment 21•9 years ago
|
||
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.
Summary: Providing a condition as an ID to the jsonrpc webservice results in a taint error → Providing a condition as an ID to the webservice results in a taint error
Target Milestone: --- → Bugzilla 4.4
Comment 22•9 years ago
|
||
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?
Flags: needinfo?(dkl)
Comment 23•9 years ago
|
||
(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
Flags: needinfo?(dkl)
Updated•9 years ago
|
Assignee: glob → dylan
Comment 24•9 years ago
|
||
Attachment #8696521 -
Attachment is obsolete: true
Attachment #8699052 -
Flags: review?(LpSolit)
![]() |
Assignee | |
Comment 25•9 years ago
|
||
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.
Attachment #8699052 -
Flags: review?(LpSolit) → review-
Comment 26•9 years ago
|
||
Attachment #8699052 -
Attachment is obsolete: true
Attachment #8700593 -
Flags: review?(LpSolit)
![]() |
Assignee | |
Comment 27•9 years ago
|
||
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.
Attachment #8700593 -
Flags: review?(LpSolit) → review-
Comment 28•9 years ago
|
||
Attachment #8700593 -
Attachment is obsolete: true
Attachment #8700608 -
Flags: review?(LpSolit)
![]() |
Assignee | |
Comment 29•9 years ago
|
||
Comment on attachment 8700608 [details] [diff] [review]
1230932_5.patch
r=LpSolit
Attachment #8700608 -
Flags: review?(LpSolit) → review+
Updated•9 years ago
|
Flags: approval5.0?
Flags: approval4.4?
Comment 30•9 years ago
|
||
Attachment #8700646 -
Flags: review?(dkl)
Comment 31•9 years ago
|
||
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.
Attachment #8700646 -
Flags: review?(dkl) → review-
Comment 32•9 years ago
|
||
Attachment #8700646 -
Attachment is obsolete: true
Attachment #8700660 -
Flags: review?(dkl)
Comment 33•9 years ago
|
||
Comment on attachment 8700660 [details] [diff] [review]
4.4 2nd patch
Review of attachment 8700660 [details] [diff] [review]:
-----------------------------------------------------------------
r=dkl
Attachment #8700660 -
Flags: review?(dkl) → review+
Updated•9 years ago
|
Flags: approval5.0?
Flags: approval5.0+
Flags: approval4.4?
Flags: approval4.4+
Comment 34•9 years ago
|
||
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
0cd77b4..eb1357f master -> master
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
6a24138..396ae88 5.0 -> 5.0
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
3e0ed9c..fc5cdf3 4.4 -> 4.4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 35•9 years ago
|
||
This unfortunately breaks the "alias" feature. This won't make it into the current release.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•9 years ago
|
Attachment #8700608 -
Flags: review+ → review-
Updated•9 years ago
|
Attachment #8700660 -
Flags: review+ → review-
![]() |
Assignee | |
Updated•9 years ago
|
Flags: approval5.0+
Flags: approval4.4+
Updated•9 years ago
|
Flags: blocking5.0.3+
![]() |
Assignee | |
Updated•9 years ago
|
Assignee: dylan → LpSolit
Status: REOPENED → ASSIGNED
![]() |
Assignee | |
Comment 36•9 years ago
|
||
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.
Attachment #8696458 -
Attachment is obsolete: true
Attachment #8700608 -
Attachment is obsolete: true
Attachment #8700660 -
Attachment is obsolete: true
Attachment #8726952 -
Flags: review?(dkl)
Comment 37•9 years ago
|
||
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.
Attachment #8726952 -
Flags: review?(dkl) → review-
![]() |
Assignee | |
Comment 38•9 years ago
|
||
Attachment #8726952 -
Attachment is obsolete: true
Attachment #8728618 -
Flags: review?(dkl)
Comment 39•9 years ago
|
||
Comment on attachment 8728618 [details] [diff] [review]
patch for master, v3
Review of attachment 8728618 [details] [diff] [review]:
-----------------------------------------------------------------
r=dkl
Attachment #8728618 -
Flags: review?(dkl) → review+
![]() |
Assignee | |
Comment 40•9 years ago
|
||
I just realized that I forgot to fix update_comment_tags() for REST too.
Attachment #8728618 -
Attachment is obsolete: true
Attachment #8728725 -
Flags: review?(dkl)
![]() |
Assignee | |
Comment 41•9 years ago
|
||
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.
Attachment #8728727 -
Flags: review?(dkl)
Comment 42•9 years ago
|
||
Comment on attachment 8728725 [details] [diff] [review]
patch for master, v3.1
Review of attachment 8728725 [details] [diff] [review]:
-----------------------------------------------------------------
r=dkl
Attachment #8728725 -
Flags: review?(dkl) → review+
Comment 43•9 years ago
|
||
Comment on attachment 8728727 [details] [diff] [review]
patch for 5.0, v1
Review of attachment 8728727 [details] [diff] [review]:
-----------------------------------------------------------------
r=dkl
Attachment #8728727 -
Flags: review?(dkl) → review+
Updated•9 years ago
|
Flags: approval5.0+
Flags: approval+
![]() |
Assignee | |
Comment 44•9 years ago
|
||
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.
Target Milestone: Bugzilla 4.4 → Bugzilla 5.0
![]() |
Assignee | |
Comment 45•9 years ago
|
||
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
6da063a..0cac98d master -> master
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
1c5ecdf..6e0182e 5.0 -> 5.0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•