Closed Bug 515191 Opened 10 years ago Closed 10 years ago

[SECURITY] SQL Injection via Bug.search (CVE-2009-3125) and Bug.create (CVE-2009-3165)

Categories

(Bugzilla :: WebService, defect, P1, blocker)

3.3.2
defect

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

(Whiteboard: click "Hide Obsolete" at the bottom of the attachment list, then pick the patch that closest-matches your version number.)

Attachments

(3 files, 11 obsolete files)

Unfortunately, Bug.search does not validate its parameters to determine that the keys are valid.

The WebService does not taint its incoming parameters.

Bugzilla::Object->match does not validate its parameters, and will allow arbitrary SQL through in keys.

Interestingly enough, I haven't actually found a way to exploit it in a way that destroys data, due to the way that DBD::mysql and DBD::Pg work--DBD::mysql recognizes ";" as a syntax error, and Pg won't prepare multiple statements. I can't quite find anything harmful to do in a subquery, either.

It also can't violate security restrictions, because we call visible_bugs on the returned result.

However, SQL injections are still a bad, bad thing. Even if *I* can't find a way to exploit this one, I'm sure somebody will.
Flags: blocking3.4.2+
Depends on: 398281
Attached patch v1 (obsolete) — Splinter Review
This fixes it by ignoring invalid fields passed to search() and create().
Assignee: webservice → mkanat
Status: NEW → ASSIGNED
Attachment #399245 - Flags: review?(LpSolit)
No longer depends on: 398281
LpSolit successfully exploited.  Since you exposed it by setting a dependency on a public bug, we need this released ASAP.  Let's get the admins of any high-profile sites that already have 3.4 CCed on this bug, too.
Severity: critical → blocker
(In reply to comment #2)
> LpSolit successfully exploited.  Since you exposed it by setting a dependency
> on a public bug, we need this released ASAP.

How does that "expose it"? It just means there's some type of bug that isn't viewable by most people (but probably a security bug), but the people on that other bug have no idea what it is exactly. People could also think it's just a typo or something.
(In reply to comment #0)
> It also can't violate security restrictions, because we call visible_bugs on
> the returned result.

It's actually very easy to bypass security restrictions, and a logged out user
can get all the data he wants, including restricted bugs, private attachment
data, user passwords, etc.... Basically, all the DB content is freely
accessible. But I guess I shouldn't write here how to do this.

That's one of the most critical bug we ever had. We need to CC as many Bugzilla
administrators as possible (NASA, KDE, Mandriva, ....), well before we release 3.4.2, so that they can apply the patch locally before the bug becomes public. They cannot afford to be unpatched when 3.4.2 is available.
Priority: -- → P1
Summary: SQL Injection via Bug.search → [SECURITY] SQL Injection via Bug.search
GNOME, ASF, and Eclipse are the only people off the short lists at the top of the installation list that are running 3.4.x right now.
(In reply to comment #3)
> How does that "expose it"? It just means there's some type of bug that isn't
> viewable by most people (but probably a security bug), but the people on that
> other bug have no idea what it is exactly. People could also think it's just a
> typo or something.

It points out that something in the bug you set the dependency on caused a security issue.  There have been multiple instances on b.m.o where it's been proven that someone tracked down a security bug ahead of time that way because there are people who scan activity on bmo *looking* for things to exploit.  If they know something on that patch caused a security bug, they'll scrutinize it until they figure out what it was.  The fact that a new bug was filed at almost the same time suggesting that Bug.search should taint the incoming parameters gives a *really* good clue where to look, too.  That bug probably shouldn't have been filed until this was made public.
The webservice taint bug was pretty carefully worded actually.  Unless someone manages to tie the two together it's probably okay.
(In reply to comment #7)
> The webservice taint bug was pretty carefully worded actually.

Which bug are you referring to? Bug 513593? It was filed 9 days ago. Hard to think they are related.
Bugzilla 3.3.2 and newer are affected.
Version: 3.4 → 3.3.2
Yeah, 513593 is what I was thinking of.  bugbot mentioned it on-channel at near the same time.
CC'ing djm, who is the admin of OpenSSH's Bugzilla (which uses 3.4.1)
I put in a request for a CVE number from MITRE.
(In reply to comment #12)
> I put in a request for a CVE number from MITRE.

Oh, good, I was just going to ask about that actually :)  This definitely needs one.
Comment on attachment 399245 [details] [diff] [review]
v1

>         my $field_name = FIELD_MAP->{$field} || $field;

If $field exists as a hash key in FIELD_MAP, $field_name contains its hash value, but _is_valid_field() is collecting FIELD_MAP keys instead of values. In particular, this makes limit and offset to be always rejected.


>+        # This prevents SQL injection via field names.
>+        next if (!_is_valid_field($field_name));

You should throw an error here. If I pass bug_ids => 1, this wrong key will be ignored, and all visible bugs will be returned, which is not what you want.


>+sub _is_valid_field {

>+    my @valid_fields = (Bugzilla::Bug->fields, keys %{ FIELD_MAP() }, 
>+                        'qa_contact');

Using hash keys for FIELD_MAP is wrong as described above.


Also, I still manage to inject SQL thanks to OFFSET which is not validated by Object->match(). So I could pass limit => 'some SQL string' and the string will be passed as is to Object->match(). OFFSET should be validated using either detaint_natural() or detaint_signed().
Attachment #399245 - Flags: review?(LpSolit) → review-
(In reply to comment #14)
> You should throw an error here. If I pass bug_ids => 1, this wrong key will be
> ignored, and all visible bugs will be returned, which is not what you want.

  Hmm, that would change the API--not something I want to do on a stable branch. 


> Also, I still manage to inject SQL thanks to OFFSET which is not validated by
> Object->match(). So I could pass limit => 'some SQL string' and the string will
> be passed as is to Object->match(). OFFSET should be validated using either
> detaint_natural() or detaint_signed().

  Okay, I think I need to finish the WebService tainting feature here first so that we can be sure that we've eliminated every possible injection.
(In reply to comment #15)
>   Hmm, that would change the API--not something I want to do on a stable
> branch. 

Currently, the SQL server crashes. I wouldn't call it an API change. In both cases do you get an error.
Attached patch v2 (obsolete) — Splinter Review
Here we go. We also need to test if this affects all the create() functions in the WebService.
Attachment #399245 - Attachment is obsolete: true
Attachment #399368 - Flags: review?(LpSolit)
Also, we need to test this against my Bug.search WebService test (which I should fix the bugs of and check in).
(In reply to comment #14)
> Also, I still manage to inject SQL thanks to OFFSET which is not validated by
> Object->match().

Your patch doesn't fix this problem. Unless I miss something, OFFSET must be an interger, so you should call detaint_natural() on it and throw an error if it's not valid. I think Object::match() is the right place to do this check.

Also, why do you call ThrowCodeError() if the field doesn't exist? Here, it's the user's fault, so it should be ThrowUserError().
(In reply to comment #19)
> Also, why do you call ThrowCodeError() if the field doesn't exist? Here, it's
> the user's fault, so it should be ThrowUserError().

  The error is from code-error. We use it elsewhere in the same file already. The WebService clients have no idea whether it's a code error or a user error being thrown.

  I will fix the offset and limit stuff.
Attached patch v3 (obsolete) — Splinter Review
This fixes it at the edge (Bug.search) instead of fixing it in the core (Object->match). I did this because I want to be warned if tainted data gets into those parameters in the future. Also, what I really want to do in Object->match is use placeholders--but using placeholders there would break Oracle at the moment, because of the hack the Oracle driver uses to implement LIMIT.
Attachment #399368 - Attachment is obsolete: true
Attachment #399403 - Flags: review?(LpSolit)
Attachment #399368 - Flags: review?(LpSolit)
I'm going to test for other holes as part of bug 513593, also.
This is also possibly exploitable in Bug.create, meaning that this affects all versions of Bugzilla back to 3.0.
Target Milestone: Bugzilla 3.4 → Bugzilla 3.0
Attached patch v4 - HEAD and 3.4 (obsolete) — Splinter Review
Okay, I decided that you were right and it should just be fixed in match() now. We can worry about placeholders later.
Attachment #399403 - Attachment is obsolete: true
Attachment #399411 - Flags: review?(LpSolit)
Attachment #399403 - Flags: review?(LpSolit)
Attached patch v5 - 3.4 (obsolete) — Splinter Review
I realized that I had forgotten to update the POD to account for the fact that 3.0 and 3.2 are affected.
Attachment #399411 - Attachment is obsolete: true
Attachment #399412 - Flags: review?(LpSolit)
Attachment #399411 - Flags: review?(LpSolit)
Attached patch v5 - 3.2 and 3.0 (obsolete) — Splinter Review
Here's the backport for 3.2 and 3.0.
Attachment #399414 - Flags: review?(LpSolit)
My suspicion at the moment is that the Bug.create version is not exploitable.  The SQL in question is

INSERT INTO $table ( {@field_names} ) VALUES ($qmarks)

where $qmarks is a list of placeholder question marks.  You can't SQL-inject via placeholders, so passing something in a field name would be the only way to inject.  Any payload you could stick in there (even by using a valid field name followed by a close paren and your payload after that) would be mutually exclusive with the VALUES() construct on the end, and would end up throwing an error anyway.

We know DBD::mysql throws an error if you try to include a ";" in a query.  What about DBD::Pg and DBD::Oracle?  The DBI docs say it blindly passes your query through to the driver, so it'd be up to the driver whether it bails or not...
Also note that bug 515328 handles this on HEAD in a much more elegant way than the patches attached here (though I still want the patches attached here to land on the branches).
And bug 513593 should make it much more difficult for things like this to happen via the WebService in the future. So we're really attacking this from all angles to make sure that it can never happen again.
(In reply to comment #27)
> My suspicion at the moment is that the Bug.create version is not exploitable. 

That's what I think too. IMO, only 3.3.2 and newer are vulnerable via Bug.search(), which is the topic of this bug, btw. IMO, there is no need to backport this patch to 3.0 and 3.2, or this should be done in a separate bug but without the mention to be a security fix. Here, we are giving a false alarm that 3.0 and 3.2 are also vulnerable, which is not true, AFAICS.
No, there is a definite and dangerous exploit for Bug.create which justdave and I just tested and proved, but which we are not going to discuss here.
Blocks: 515454
(In reply to comment #32)
> No, there is a definite and dangerous exploit for Bug.create which justdave and
> I just tested and proved, but which we are not going to discuss here.

What justdave tried doesn't call Bug.create(). I still manage to do SQL
injection with Bug.create(), but I actually cannot insert anything as it
triggers an error. So that's less critical, but should still be fixed.
Attachment #399412 - Attachment description: v5 - HEAD and 3.4 → v5 - 3.4
(In reply to comment #33)
> injection with Bug.create(), but I actually cannot insert anything as it
> triggers an error.

OK, the error is because we use hashes and it's hard to guess in which order the keys will be sorted the next time the script is run. But I guess that with enough attempts, we may be lucky enough and trigger what we want. So we should also CC admins of main 3.0 and 3.2 installations, not only those mentioned in comment 5.
Summary: [SECURITY] SQL Injection via Bug.search → [SECURITY] SQL Injection via Bug.search and Bug.create
Duplicate of this bug: 515328
Attached patch v6 - HEAD and 3.4 (obsolete) — Splinter Review
This fixes the problem at the root instead of fixing it at the edge.
Attachment #399412 - Attachment is obsolete: true
Attachment #399414 - Attachment is obsolete: true
Attachment #399591 - Flags: review?(LpSolit)
Attachment #399412 - Flags: review?(LpSolit)
Attachment #399414 - Flags: review?(LpSolit)
Attached patch v6 - 3.2 (obsolete) — Splinter Review
Here's the backport for 3.2.
Attachment #399594 - Flags: review?(LpSolit)
Attached patch v6 - 3.0 (obsolete) — Splinter Review
Attachment #399596 - Flags: review?(LpSolit)
Attached patch v7 - HEAD and 3.4 (obsolete) — Splinter Review
I realized that I forgot to update WebService::Constants for HEAD and 3.4. (The other branches do not need new patches.)
Attachment #399591 - Attachment is obsolete: true
Attachment #399599 - Flags: review?(LpSolit)
Attachment #399591 - Flags: review?(LpSolit)
Here is a new patch with a unique webservice error number for param_invalid.
Attachment #399599 - Attachment is obsolete: true
Attachment #399634 - Flags: review?(LpSolit)
Attachment #399599 - Flags: review?(LpSolit)
Attached patch v8 - 3.2Splinter Review
Attachment #399594 - Attachment is obsolete: true
Attachment #399635 - Flags: review?(LpSolit)
Attachment #399594 - Flags: review?(LpSolit)
Attached patch v8 - 3.0 (obsolete) — Splinter Review
Attachment #399596 - Attachment is obsolete: true
Attachment #399636 - Flags: review?(LpSolit)
Attachment #399596 - Flags: review?(LpSolit)
Comment on attachment 399634 [details] [diff] [review]
v8 - HEAD and 3.4

Passes all QA tests, also passes my own testing. And it looks correct now. r=LpSolit
Attachment #399634 - Flags: review?(LpSolit) → review+
Comment on attachment 399635 [details] [diff] [review]
v8 - 3.2

r=LpSolit
Attachment #399635 - Flags: review?(LpSolit) → review+
Comment on attachment 399599 [details] [diff] [review]
v7 - HEAD and 3.4

Applied locally on GNOME Bugzilla. (if it has problems, we'll know soon enough:)
Urgh, of course I applied the patch with r+, not the one I commented upon.
Comment on attachment 399636 [details] [diff] [review]
v8 - 3.0

>Index: Bugzilla/Object.pm

>+sub _check_field {

>+    if (!Bugzilla->dbh->bz_column_info($class->DB_TABLE, $field)) {

Doesn't work on 3.0.8+:

-32000 DBD::mysql::db selectrow_array failed: Table 'bz_schema' was not locked with LOCK TABLES [for Statement "SELECT schema_data, version FROM bz_schema"] at Bugzilla/DB.pm line 1007
Attachment #399636 - Flags: review?(LpSolit) → review-
Attached patch v9 - 3.0Splinter Review
Okay, here's a different patch for 3.0, going back to fixing this just in the WebService.

I tried to think about how to safely load _bz_real_schema before Object::create() was called, but there was no way that I could do it that I could guarantee wouldn't cause any strange side effects. So for 3.0, we're going back to fixing this at the affected callsite, which is also a much smaller patch that only affects a single file, in any case.
Attachment #399636 - Attachment is obsolete: true
Attachment #399652 - Flags: review?(LpSolit)
Attachment #399652 - Flags: review?(LpSolit) → review+
Comment on attachment 399652 [details] [diff] [review]
v9 - 3.0

Indeed safer for 3.0. r=LpSolit
Flags: testcase?
Flags: blocking3.2.5+
Flags: blocking3.0.9+
Flags: approval?
Flags: approval3.4?
Flags: approval3.2?
Flags: approval3.0?
  Hello administrators of major Bugzilla installations! You are being CC'ed on this bug so that you have a chance to patch your installation before the security advisory is made public.

  Please do not mention the security issue anywhere public, and please do not check the patch into any publicly-available version control server. Basically, 
do not expose the patch or the vulnerability in any way, before we release.
How long do we have to patch our installations before this goes public?
(In reply to comment #51)
> How long do we have to patch our installations before this goes public?

  I would estimate between 24 and 48 hours. Given where we are with the release process, I could probably release tomorrow, though if there's some sort of snafu it could take until Friday.
bugzilla.mozilla.org is patched.
bugs.webkit.org is patched.
(In reply to comment #12)
> I put in a request for a CVE number from MITRE.

Did we get this yet?
(In reply to comment #55)
> Did we get this yet?

  We have one for Bug.search, I've put it in the whiteboard. reed is waiting on another for Bug.create.
Whiteboard: CVE-2009-3125
Alias: CVE-2009-3125
Whiteboard: CVE-2009-3125
(In reply to comment #56)
>   We have one for Bug.search, I've put it in the whiteboard. reed is waiting on
> another for Bug.create.

It's two different vectors into the same vulnerability isn't it?
(In reply to comment #57)
> (In reply to comment #56)
> It's two different vectors into the same vulnerability isn't it?

  No, it's two separate vulnerabilities--one in Object::match (which has predictable SQL and only affects 3.4) and another in Object::create (which has randomized SQL and affects back to 2.23.3 or so). See the Security Advisory for details. They just both happen to have a very similar fix.
bugs.gentoo.org is unaffected then, it still runs 2.22.x.
bugzilla.redhat.com is patched.
bugs.eclipse.org is patched.
issues.apache.org is patched
bugs.clamav.net is patched.
bugs.kde.org is patched.
Anyone knows who is the admin of bugzilla.kernel.org?
Adding warthog19@eaglescrag.net for bugzilla.kernel.org
qa.mandriva.com is patched. Thanks.
Ok v9 for 3.0 applied cleanly for me.  Should be in production now.
djm, did you patch OpenSSH's Bugzilla?
bugzilla.novell.com is patched
Everything is ready for the releases, and I plan to release tomorrow morning Pacific time, or possibly in the early afternoon.
Alias: CVE-2009-3125
Summary: [SECURITY] SQL Injection via Bug.search and Bug.create → [SECURITY] SQL Injection via Bug.search (CVE-2009-3125) and Bug.create (CVE-2009-3165)
tip:

Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.36; previous revision: 1.35
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.29; previous revision: 1.28
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.116; previous revision: 1.115
done


3.4:

Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.30.2.3; previous revision: 1.30.2.2
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.33.2.7; previous revision: 1.33.2.6
done
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.24.2.2; previous revision: 1.24.2.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.109.2.2; previous revision: 1.109.2.1
done


3.2:

Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.23.2.1; previous revision: 1.23
done
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.16.2.2; previous revision: 1.16.2.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.105.2.2; previous revision: 1.105.2.1
done

3.0:

Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.4.2.5; previous revision: 1.4.2.4
done
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval3.2?
Flags: approval3.2+
Flags: approval3.0?
Flags: approval3.0+
Flags: approval+
Resolution: --- → FIXED
Security advisory sent, unlocking bug.
Group: bugzilla-security
Whiteboard: click "Hide Obsolete" at the bottom of the attachment list, then pick the patch that closest-matches your version number.
For those coming here looking for a patch to apply, go here:

https://bugzilla.mozilla.org/show_bug.cgi?id=515191#a0

Then click the link for "Hide Obsolete".  Then choose the patch that closest matches the version of Bugzilla you're running.
bugs.maemo.org is also unaffected as it currently still runs 2.22.x.
Thanks for CC'ing though. I appreciate it.
> Thanks for CC'ing though. I appreciate it.

+1
Blocks: 517502
Depends on: 398281
You need to log in before you can comment on or make changes to this bug.