Last Comment Bug 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
(CVE-2010-2759)
: [SECURITY][PostgreSQL] Bugzilla crashes when viewing a bug if a comment conta...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 2.23.1
: All All
: -- normal (vote)
: Bugzilla 3.2
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
Depends on:
Blocks: 580214
  Show dependency treegraph
 
Reported: 2010-08-02 00:59 PDT by Slava Buhtiarov
Modified: 2010-08-05 21:40 PDT (History)
3 users (show)
LpSolit: approval+
LpSolit: approval4.0+
LpSolit: blocking4.0+
LpSolit: approval3.6+
LpSolit: blocking3.6.2+
LpSolit: approval3.4+
LpSolit: blocking3.4.8+
LpSolit: approval3.2+
LpSolit: blocking3.2.8+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for 3.6 - 4.1, v1 (3.06 KB, patch)
2010-08-02 04:52 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch for 3.6 - 4.1, v1.1 (2.99 KB, patch)
2010-08-02 04:55 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch for 3.6-4.1, v2 (3.48 KB, patch)
2010-08-02 18:01 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review
patch for 3.2-3.4, v1 (2.24 KB, patch)
2010-08-03 05:26 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Slava Buhtiarov 2010-08-02 00:59:04 PDT
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 2010-08-02 04:27:46 PDT
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 2010-08-02 04:52:31 PDT
Created attachment 462035 [details] [diff] [review]
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 2010-08-02 04:55:28 PDT
Created attachment 462037 [details] [diff] [review]
patch for 3.6 - 4.1, v1.1

I just removed a useless empty line.
Comment 4 Slava Buhtiarov 2010-08-02 05:30:46 PDT
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 2010-08-02 05:37:10 PDT
(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 2010-08-02 05:39:09 PDT
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 2010-08-02 06:44:13 PDT
Ок. 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 2010-08-02 15:00:30 PDT
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 2010-08-02 15:02:52 PDT
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 2010-08-02 15:04:46 PDT
Also, I suspect this only happens on Pg 8.3 and higher.
Comment 11 Max Kanat-Alexander 2010-08-02 15:06:41 PDT
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 2010-08-02 15:07:53 PDT
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 2010-08-02 15:10:00 PDT
Also, fixing new() may have unexpected side-effects I don't want for older branches.
Comment 14 Max Kanat-Alexander 2010-08-02 15:17:45 PDT
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 2010-08-02 15:18:36 PDT
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 2010-08-02 15:19:45 PDT
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 2010-08-02 18:01:49 PDT
Created attachment 462281 [details] [diff] [review]
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 2010-08-02 18:30:55 PDT
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 2010-08-02 18:33:04 PDT
(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 2010-08-02 18:33:59 PDT
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 2010-08-02 18:39:16 PDT
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 2010-08-02 18:57:37 PDT
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 2010-08-02 18:58:10 PDT
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 2010-08-02 19:15:46 PDT
(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 2010-08-02 19:20:14 PDT
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 2010-08-03 04:43:17 PDT
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 2010-08-03 05:26:41 PDT
Created attachment 462372 [details] [diff] [review]
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 2010-08-03 14:39:45 PDT
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 2010-08-03 15:56:03 PDT
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 2010-08-04 15:18:17 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 7430.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 7371.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 7160.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.4/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 6774.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.2/
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Template.pm
Committed revision 6395.
Comment 31 Max Kanat-Alexander 2010-08-05 21:40:03 PDT
Security advisory sent, unlocking bug.

Note You need to log in before you can comment on or make changes to this bug.