583690 - (CVE-2010-2759) [SECURITY][PostgreSQL] Bugzilla crashes when viewing a bug if a comment contains 'bug <num>' or 'attachment <num>' where <num> is greater than the max allowed integer

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Slava Buhtiarov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; ru; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 ( .NET CLR 3.5.30729)
Build Identifier: 3.6.1

Crash with error 'value "4294967296" is out of range for type integer ' when I type 'bug 4294967296' in comments for bug. 
Database server is PostgreSQL 8.3.7
Bugzilla version 3.6.1


Reproducible: Always

Steps to Reproduce:
1. Open any existing bug
2. Enter 'bug 4294967296' in comment
3. Save changes
Actual Results:  
undef error - DBD::Pg::db selectrow_hashref failed: ERROR: value "4294967296" is out of range for type integer [for Statement " SELECT alias,assigned_to,bug_file_loc,bug_id,bug_severity,bug_status,cclist_accessible,component_id,delta_ts,estimated_time,everconfirmed,op_sys,priority,product_id,qa_contact,remaining_time,rep_platform,reporter_accessible,resolution,short_desc,status_whiteboard,target_milestone,version,reporter AS reporter_id,TO_CHAR(creation_ts, 'YYYY.MM.DD HH24:MI') AS creation_ts,TO_CHAR(deadline, 'YYYY-MM-DD') AS deadline FROM bugs WHERE bug_id = ?"] at Bugzilla/Object.pm line 85 Bugzilla::Object::_init('Bugzilla::Bug', 4294967296) called at Bugzilla/Object.pm line 53 Bugzilla::Object::new('Bugzilla::Bug', 4294967296) called at Bugzilla/Bug.pm line 294 Bugzilla::Bug::new('undef', 4294967296) called at Bugzilla/Template.pm line 326 Bugzilla::Template::get_bug_link('undef', 'undef', 'HASH(0x973b1f4)') called at Bugzilla/Template.pm line 245 Bugzilla::Template::quoteUrls('bug 4294967296', 'Bugzilla::Bug=HASH(0x915426c)', 'Bugzilla::Comment=HASH(0x94cdab4)') called at Bugzilla/Template.pm line 562 Bugzilla::Template::__ANON__('bug 4294967296') called at template/en/default/bug/comments.html.tmpl line 241 eval {...} called at template/en/default/bug/comments.html.tmpl line 175 Template::Provider::__ANON__('Bugzilla::Template::Context=HASH(0x8f6fcfc)') called at lib/i386-freebsd-64int/Template/Context.pm line 348 eval {...} called at lib/i386-freebsd-64int/Template/Context.pm line 321 Template::Context::process('Bugzilla::Template::Context=HASH(0x8f6fcfc)', 'a_comment') called at Bugzilla/Template/Context.pm line 45 Bugzilla::Template::Context::process('Bugzilla::Template::Context=HASH(0x8f6fcfc)', 'a_comment') called at template/en/default/bug/comments.html.tmpl line 130 eval {...} called at template/en/default/bug/comments.html.tmpl line 136 eval {...} called at template/en/default/bug/comments.html.tmpl line 18 Template::Provider::__ANON__('Bugzilla::Template::Context=HASH(0x8f6fcfc)') called at lib/i386-freebsd-64int/Template/Document.pm line 151 eval {...} called at lib/i386-freebsd-64int/Template/Document.pm line 149 Template::Document::process('Template::Document=HASH(0x972e548)', 'Bugzilla::Template::Context=HASH(0x8f6fcfc)') called at lib/i386-freebsd-64int/Template/Context.pm line 351 eval {...} called at lib/i386-freebsd-64int/Template/Context.pm line 321 Template::Context::process('Bugzilla::Template::Context=HASH(0x8f6fcfc)', 'bug/comments.html.tmpl', 'HASH(0x971a910)') called at Bugzilla/Template/Context.pm line 45 Bugzilla::Template::Context::process('Bugzilla::Template::Context=HASH(0x8f6fcfc)', 'bug/comments.html.tmpl', 'HASH(0x971a910)') called at template/en/default/bug/edit.html.tmpl line 238 eval {...} called at template/en/default/bug/edit.html.tmpl line 18 Template::Provider::__ANON__('Bugzilla::Template::Context=HASH(0x8f6fcfc)') called at lib/i386-freebsd-64int/Template/Document.pm line 151 eval {...} called at lib/i386-freebsd-64int/Template/Document.pm line 149 Template::Document::process('Template::Document=HASH(0x94b3fcc)', 'Bugzilla::Template::Context=HASH(0x8f6fcfc)') called at lib/i386-freebsd-64int/Template/Context.pm line 351 eval {...} called at lib/i386-freebsd-64int/Template/Context.pm line 321 Template::Context::process('Bugzilla::Template::Context=HASH(0x8f6fcfc)', 'bug/edit.html.tmpl') called at Bugzilla/Template/Context.pm line 45 Bugzilla::Template::Context::process('Bugzilla::Template::Context=HASH(0x8f6fcfc)', 'bug/edit.html.tmpl') called at template/en/default/bug/show.html.tmpl line 45 eval {...} called at template/en/default/bug/show.html.tmpl line 18 Template::Provider::__ANON__('Bugzilla::Template::Context=HASH(0x8f6fcfc)') called at lib/i386-freebsd-64int/Template/Document.pm line 151 eval {...} called at lib/i386-freebsd-64int/Template/Document.pm line 149 Template::Document::process('Template::Document=HASH(0x92a9544)', 'Bugzilla::Template::Context=HASH(0x8f6fcfc)') called at lib/i386-freebsd-64int/Template/Context.pm line 351 eval {...} called at lib/i386-freebsd-64int/Template/Context.pm line 321 Template::Context::process('Bugzilla::Template::Context=HASH(0x8f6fcfc)', 'Template::Document=HASH(0x92a9544)') called at Bugzilla/Template/Context.pm line 45 Bugzilla::Template::Context::process('Bugzilla::Template::Context=HASH(0x8f6fcfc)', 'Template::Document=HASH(0x92a9544)') called at lib/i386-freebsd-64int/Template/Service.pm line 94 eval {...} called at lib/i386-freebsd-64int/Template/Service.pm line 91 Template::Service::process('Template::Service=HASH(0x8ef5f7c)', 'bug/show.html.tmpl', 'HASH(0x804d16c)') called at lib/i386-freebsd-64int/Template.pm line 66 Template::process('Bugzilla::Template=HASH(0x8e6f028)', 'bug/show.html.tmpl', 'HASH(0x804d16c)') called at /usr/local/www/bugzilla/show_bug.cgi line 128

Comment 1

[Frédéric Buclin]

Confirmed. MySQL silently returns no result while Pg throws the error reported in comment 0 (no idea about Oracle). This problem is also visible for attachment IDs being too large. You could use this trick to prevent access to all bug reports as you could comment in all bugs, making Bugzilla to crash every time you try to access a bug.

Comment 2

[Frédéric Buclin]

Attachment: patch for 3.6 - 4.1, v1

Technically, the constant could be named MAX_INTEGER as Pg makes no distinction between INT3 (mediumint) and INT4 (integer) types. If we rename it MAX_INTEGER, then it could go as a constant in Constants.pm, with value 2147483647, as it would be the same for both MySQL and PostgreSQL (I think Oracle doesn't care), and MySQL won't crash with a number between 8388607 and 2147483647.

Comment 3

[Frédéric Buclin]

Attachment: patch for 3.6 - 4.1, v1.1

I just removed a useless empty line.

Comment 4

[Slava Buhtiarov]

May be this error must be processed in contructor of "Bugzilla::Bug" class?

Try this: /show_bug.cgi?id=4294967296

DBD::Pg::db selectrow_hashref failed: ERROR: value "4294967296" is out of range for type integer [for Statement " SELECT alias,assigned_to,bug_file_loc,bug_id,bug_severity,bug_status,cclist_accessible,component_id,delta_ts,estimated_time,everconfirmed,op_sys,priority,product_id,qa_contact,remaining_time,rep_platform,reporter_accessible,resolution,short_desc,status_whiteboard,target_milestone,version,reporter AS reporter_id,TO_CHAR(creation_ts, 'YYYY.MM.DD HH24:MI') AS creation_ts,TO_CHAR(deadline, 'YYYY-MM-DD') AS deadline FROM bugs WHERE bug_id = ?"] at Bugzilla/Object.pm line 85 Bugzilla::Object::_init('Bugzilla::Bug', 4294967296) called at Bugzilla/Object.pm line 53 Bugzilla::Object::new('Bugzilla::Bug', 4294967296) called at Bugzilla/Bug.pm line 294 Bugzilla::Bug::new(undef, 4294967296) called at Bugzilla/Bug.pm line 323 Bugzilla::Bug::check('Bugzilla::Bug', 4294967296) called at /usr/local/www/data/bugzilla32/show_bug.cgi line 65

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> May be this error must be processed in contructor of "Bugzilla::Bug" class?

Well, all places where an ID is passed to the new() method, not only Bugzilla::Bug, but also Bugzilla::Group, Bugzilla::User, etc... could be concerned by this problem. We could put a single check in Bugzilla::Object->new(), but the problem would be to know what to do when a too large integer is passed. Throw an error or fail silently? In the problem described in this bug, you don't want to throw an error, while in some other cases, such as the one you mentioned in comment 4, it may be fine to throw an error telling the user that the ID is too large. But that's definitely out of the scope of this bug.

Comment 6

[Frédéric Buclin]

One more thing: the testcase you describe in comment 4 cannot be used to "DoS" Bugzilla, while you can with the testcase described in comment 0 (which is why I marked it as a security bug).

Comment 7

[Slava Buhtiarov]

Ок. But constructor of bug class already have some checking code. And if you try to request bug with invalid or float point id you will get error 
----
            # Aliases are off, and we got something that's not a number.
            my $error_self = {};
            bless $error_self, $class;
            $error_self->{'bug_id'} = $param;
            $error_self->{'error'}  = 'InvalidBugId';
            return $error_self;
----

Comment 8

[Max Kanat-Alexander]

Comment on attachment 462037 [details] [diff] [review]
patch for 3.6 - 4.1, v1.1

The check needs to go into Bugzilla::Object->new, I'm pretty sure (at least on trunk). Perhaps for the branches, it should just go into Bugzilla::Bug->new and Bugzilla::Attachment->new.

Comment 9

[Max Kanat-Alexander]

Also, I think we should target this for 3.6.3 instead--we already have too many security bugs going into 3.6.2.

Comment 10

[Max Kanat-Alexander]

Also, I suspect this only happens on Pg 8.3 and higher.

Comment 11

[Max Kanat-Alexander]

Also, new() should return undef in this case--that's what it normally does for invalid values. Just act like somebody asked for an object that doesn't exist.

new_from_list should just remove those values from the input list before running the SQL.

Comment 12

[Frédéric Buclin]

Comment on attachment 462037 [details] [diff] [review]
patch for 3.6 - 4.1, v1.1

No, as I said, this bug is not about fixing new(). We don't want to throw an error, but simply return the string which was passed. Fixing new() is another bug. Also, we should take it for 3.6.2. It's safe and non-invasive.

Comment 13

[Frédéric Buclin]

Also, fixing new() may have unexpected side-effects I don't want for older branches.

Comment 14

[Max Kanat-Alexander]

Well, I get what you're saying. However, fixing new() won't have any unexpected side effects. The behavior of "new" is to return "undef" when given an id that does not exist. Since ids greater than the integer maximum can't possibly exist in the database, you are simply correctly returning "undef" for an object that doesn't exist.

Comment 15

[Max Kanat-Alexander]

Also, since only Pg has the problem, I agree that just having a MAX_INT constant would be fine. Maybe MAX_INT_32 would be the most appropriate name, since it's a 32-bit integer. We could just check things against the MAX_INT constant.

Comment 16

[Max Kanat-Alexander]

Although yes, for the branches it's hard to say if you'd run into funny things having to do with invalid input--perhaps we should only check it once we've verified that it's an integer (which I suppose we already do, in new()).

Comment 17

[Frédéric Buclin]

Attachment: patch for 3.6-4.1, v2

Fix new() and new_from_list(). check() calls new(), so it's automatically fixed too.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 462281 [details] [diff] [review]
patch for 3.6-4.1, v2

This looks good. The Object changes I feel pretty confident about and I think they can go on the branches.

I'm a little nervous about modifying get_attachment_link on the 3.6 branch or any earlier. When did we actually start supporting Pg 8.3--I think it was 3.6, yeah? Maybe it was 3.4. Not sure.

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
> I'm a little nervous about modifying get_attachment_link on the 3.6 branch or
> any earlier.

Why? I'm personally not.

Comment 20

[Max Kanat-Alexander]

Mmm, I don't know the performance impact of getting an object (even though it's probably not big), and it seems like a lot of code being changed in a somewhat-critical area.

For the branches I think we should just put the "if $id > MAX_INT_32" thing inside of get_attachment_link--that's a smaller change that accomplishes the same thing, no?

Comment 21

[Frédéric Buclin]

Oh, there is no impact for 3.4 and newer, as I refactored Attachment.pm to use Object.pm, see bug 388251. So you also have a single SQL query. For 3.2, we still use Bugzilla::Attachment->get(), but even there, it's a single SQL query.

Comment 22

[Max Kanat-Alexander]

Hmm. I see what you're saying. Might it be good to have a smaller patch for the branches anyway, to conflict less with people's customizations and so forth?

Comment 23

[Max Kanat-Alexander]

Also, does Bugzilla 3.2 even support Pg 8.3? I don't think it does. And this problem almost surely won't happen on any Pg before 8.3.

Comment 24

[Frédéric Buclin]

(In reply to comment #23)
> Also, does Bugzilla 3.2 even support Pg 8.3?

Sure it does. I'm running QA tests using Pg 8.4.4 on all branches, including 3.2.

Comment 25

[Max Kanat-Alexander]

Okay. Well, I'd be more comfortable with a smaller patch for the branches, which would probably also be good since then one patch would apply to all of them, most likely.

Comment 26

[Frédéric Buclin]

Attachment 462281 [details] [diff] applies correctly on the 3.6 branch too. No reason not to take it there, as we are going to support this branch for a long time, and using an attachment object here is going to save a lot of trouble in the future.

For 3.4 and 3.2, I will attach another patch which doesn't use the attachment object, to minimize changes in Template.pm.

Comment 27

[Frédéric Buclin]

Attachment: patch for 3.2-3.4, v1

The patch for 4.1 doesn't apply to 3.4 as get_bug_link() was not using a bug object but calls the DB directly. So I had to write another patch for it anyway, which also applies to 3.2, and works.

Comment 28

[Frédéric Buclin]

Fun, Bugzilla 2.20.7 and 2.22.7 work fine with Pg 8.4.4, and are not vulnerable to this bug, because GetBugLink() in globals.pl injected the bug ID and the attachment ID directly in the SQL queries, without placeholders. But Bugzilla 2.23.1 is vulnerable as we removed globals.pl and started using placeholders everywhere.

Comment 29

[Max Kanat-Alexander]

Comment on attachment 462372 [details] [diff] [review]
patch for 3.2-3.4, v1

Looks good. :-) I think the plan to do this one on 3.4 + 3.2 and the above patch on the rest is a good plan.

Comment 30

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 7430.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 7371.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 7160.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.4/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 6774.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.2/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 6395.

Comment 31

[Max Kanat-Alexander]

Security advisory sent, unlocking bug.