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

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect, P1)

Description

[Max Kanat-Alexander]

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.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

This fixes it by ignoring invalid fields passed to search() and create().

Comment 2

[Dave Miller [:justdave]]

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.

Comment 3

[Reed Loden [:reed]]

(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.

Comment 4

[Frédéric Buclin]

(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.

Comment 5

[Dave Miller [:justdave]]

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.

Comment 6

[Dave Miller [:justdave]]

(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.

Comment 7

[Dave Miller [:justdave]]

The webservice taint bug was pretty carefully worded actually.  Unless someone manages to tie the two together it's probably okay.

Comment 8

[Frédéric Buclin]

(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.

Comment 9

[Frédéric Buclin]

Bugzilla 3.3.2 and newer are affected.

Comment 10

[Dave Miller [:justdave]]

Yeah, 513593 is what I was thinking of.  bugbot mentioned it on-channel at near the same time.

Comment 11

[Frédéric Buclin]

CC'ing djm, who is the admin of OpenSSH's Bugzilla (which uses 3.4.1)

Comment 12

[Reed Loden [:reed]]

I put in a request for a CVE number from MITRE.

Comment 13

[Dave Miller [:justdave]]

(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 14

[Frédéric Buclin]

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().

Comment 15

[Max Kanat-Alexander]

(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.

Comment 16

[Frédéric Buclin]

(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.

Comment 17

[Max Kanat-Alexander]

Attachment: v2

Here we go. We also need to test if this affects all the create() functions in the WebService.

Comment 18

[Max Kanat-Alexander]

Also, we need to test this against my Bug.search WebService test (which I should fix the bugs of and check in).

Comment 19

[Frédéric Buclin]

(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().

Comment 20

[Max Kanat-Alexander]

(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.

Comment 21

[Max Kanat-Alexander]

Attachment: v3

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.

Comment 22

[Max Kanat-Alexander]

I'm going to test for other holes as part of bug 513593, also.

Comment 23

[Max Kanat-Alexander]

This is also possibly exploitable in Bug.create, meaning that this affects all versions of Bugzilla back to 3.0.

Comment 24

[Max Kanat-Alexander]

Attachment: v4 - HEAD and 3.4

Okay, I decided that you were right and it should just be fixed in match() now. We can worry about placeholders later.

Comment 25

[Max Kanat-Alexander]

Attachment: v5 - 3.4

I realized that I had forgotten to update the POD to account for the fact that 3.0 and 3.2 are affected.

Comment 26

[Max Kanat-Alexander]

Attachment: v5 - 3.2 and 3.0

Here's the backport for 3.2 and 3.0.

Comment 27

[Dave Miller [:justdave]]

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...

Comment 28

[Dave Miller [:justdave]]

http://dev.mysql.com/doc/refman/5.1/en/insert.html
http://www.postgresql.org/docs/8.4/interactive/sql-insert.html
http://download.oracle.com/docs/cd/B19306_01/server.102/b14200/statements_9014.htm

Comment 29

[Max Kanat-Alexander]

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).

Comment 30

[Max Kanat-Alexander]

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.

Comment 31

[Frédéric Buclin]

(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.

Comment 32

[Max Kanat-Alexander]

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.

Comment 33

[Frédéric Buclin]

(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.

Comment 34

[Frédéric Buclin]

(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.

Comment 36

[Max Kanat-Alexander]

Attachment: v6 - HEAD and 3.4

This fixes the problem at the root instead of fixing it at the edge.

Comment 37

[Max Kanat-Alexander]

Attachment: v6 - 3.2

Here's the backport for 3.2.

Comment 38

[Max Kanat-Alexander]

Attachment: v6 - 3.0

Comment 39

[Max Kanat-Alexander]

Attachment: v7 - HEAD and 3.4

I realized that I forgot to update WebService::Constants for HEAD and 3.4. (The other branches do not need new patches.)

Comment 40

[Max Kanat-Alexander]

Attachment: v8 - HEAD and 3.4

Here is a new patch with a unique webservice error number for param_invalid.

Comment 41

[Max Kanat-Alexander]

Attachment: v8 - 3.2

Comment 42

[Max Kanat-Alexander]

Attachment: v8 - 3.0

Comment 43

[Frédéric Buclin]

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

Comment 44

[Frédéric Buclin]

Comment on attachment 399635 [details] [diff] [review]
v8 - 3.2

r=LpSolit

Comment 45

[Olav Vitters]

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:)

Comment 46

[Olav Vitters]

Urgh, of course I applied the patch with r+, not the one I commented upon.

Comment 47

[Frédéric Buclin]

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

Comment 48

[Max Kanat-Alexander]

Attachment: v9 - 3.0

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.

Comment 49

[Frédéric Buclin]

Comment on attachment 399652 [details] [diff] [review]
v9 - 3.0

Indeed safer for 3.0. r=LpSolit

Comment 50

[Max Kanat-Alexander]

  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.

Comment 51

[Matt Rogers]

How long do we have to patch our installations before this goes public?

Comment 52

[Max Kanat-Alexander]

(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.

Comment 53

[Dave Miller [:justdave]]

bugzilla.mozilla.org is patched.

Comment 54

[David D. Kilzer (ddk)]

bugs.webkit.org is patched.

Comment 55

[Dave Miller [:justdave]]

(In reply to comment #12)
> I put in a request for a CVE number from MITRE.

Did we get this yet?

Comment 56

[Max Kanat-Alexander]

(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.

Comment 57

[Dave Miller [:justdave]]

(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?

Comment 58

[Max Kanat-Alexander]

(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.

Comment 59

[Robin H. Johnson [:robbat2]]

bugs.gentoo.org is unaffected then, it still runs 2.22.x.

Comment 60

[David Lawrence [:dkl]]

bugzilla.redhat.com is patched.

Comment 61

[Denis Roy]

bugs.eclipse.org is patched.

Comment 62

[Paul Querna]

issues.apache.org is patched

Comment 63

[Luca Gibelli]

bugs.clamav.net is patched.

Comment 64

[Matt Rogers]

bugs.kde.org is patched.

Comment 65

[Frédéric Buclin]

Anyone knows who is the admin of bugzilla.kernel.org?

Comment 66

[Robin H. Johnson [:robbat2]]

Adding [e-mail] for bugzilla.kernel.org

Comment 67

[Gustavo De Nardin (spuk)]

qa.mandriva.com is patched. Thanks.

Comment 68

[J.H.]

Ok v9 for 3.0 applied cleanly for me.  Should be in production now.

Comment 69

[Frédéric Buclin]

djm, did you patch OpenSSH's Bugzilla?

Comment 70

[Greg Hendricks]

bugzilla.novell.com is patched

Comment 71

[Max Kanat-Alexander]

Everything is ready for the releases, and I plan to release tomorrow morning Pacific time, or possibly in the early afternoon.

Comment 72

[Max Kanat-Alexander]

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

Comment 73

[Max Kanat-Alexander]

Security advisory sent, unlocking bug.

Comment 74

[Dave Miller [:justdave]]

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.

Comment 75

[Andre Klapper]

bugs.maemo.org is also unaffected as it currently still runs 2.22.x.
Thanks for CC'ing though. I appreciate it.

Comment 76

[Denis Roy]

> Thanks for CC'ing though. I appreciate it.

+1