Closed Bug 591165 (CVE-2010-2761) Opened 14 years ago Closed 14 years ago

[SECURITY] Header injection, cross-site scripting in report.cgi

Categories

(Bugzilla :: Reporting/Charting, defect, P1)

Product:
Bugzilla
Component:
Reporting/Charting
Version:
2.17.1
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 3.2

People

(Reporter: lcamtuf, Assigned: reed)

References

(
URL
)

Details

(Whiteboard: (CVE-2010-2761 for original CGI.pm issue; CVE-2010-4411 for secondary issue))

Attachments

(2 files, 6 obsolete files)

v2 (trunk, 4.0, 3.6)
519 bytes, patch
mkanat
: review+
Details | Diff | Splinter Review
v2 (3.4, 3.2)
545 bytes, patch
mkanat
: review+
Details | Diff | Splinter Review 
Hi folks,

While doing something completely unrelated, I noticed that the query_format parameter passed to report.cgi is not properly sanitized when quoted in the "Location" header in the response.

This allows malicious headers or HTML body to be injected in order to steal cookies, poison reverse caches, etc. Proof-of-concept against bugzilla.mozilla.org (works in Firefox):

https://bugzilla.mozilla.org/report.cgi?x_axis_field=1&y_axis_field=1&z_axis_field=1&query_format=report-tablebogus%0a%0dC<html><script>alert(1)</script>:%20s%0a%0a%0a%0aFOO&short_desc_type=jellyfish&short_desc=jellyfish&product=1&component=1&version=1&long_desc_type=jellyfish&long_desc=jellyfish&bug_file_loc_type=1&bug_file_loc=1&bug_status=1&resolution=1&bug_severity=1&priority=1&rep_platform=1&op_sys=1&emailassigned_to1=on&emailreporter1=on&emailcc1=on&emaillongdesc1=on&emailtype1=skipfish@example.com&email1=skipfish@example.com&emailassigned_to2=on&emailreporter2=on&emailcc2=on&emaillongdesc2=on&emailtype2=skipfish@example.com&email2=skipfish@example.com&bugidtype=1&bug_id=1&votes=1&chfieldfrom=1&chfieldto=Now&chfield=1&chfieldvalue=1&format=table&action=wrap&negate0=on&field0-0-0=1&type0-0-0=1&value0-0-0=1&cmd-add0-0-1=Or&cmd-add0-1-0=And&cmd-add1-0-0=Add%20another%20boolean%20chart
Severity: major → critical
Priority: -- → P1
Target Milestone: --- → Bugzilla 3.2
Introduced in revision 1933, in 2002:

  http://bzr.mozilla.org/bugzilla/trunk/revision/1933
  We also need to audit the other uses of redirect for this problem.
Attached patch v1 (obsolete) — — Splinter Review 
This changes report.cgi to use the same code that buglist.cgi does, with which I cannot reproduce the XSS.
Assignee: charting → mkanat
Status: NEW → ASSIGNED
Attachment #469772 - Flags: review?(LpSolit)
Attached patch v2 (obsolete) — — Splinter Review 
This is the right patch.
Attachment #469772 - Attachment is obsolete: true
Attachment #469781 - Flags: review?(LpSolit)
Attachment #469772 - Flags: review?(LpSolit)
Comment on attachment 469781 [details] [diff] [review]
v2

>+if (grep { $_ =~ /^cmd\-/ } $cgi->param()) {

Why does the hyphen need to be escaped?
Comment on attachment 469781 [details] [diff] [review]
v2

>-    exit;

need to keep the exit, too..
Alias: CVE-2010-2761
Flags: blocking4.0?
Flags: blocking3.6.3?
Flags: blocking3.4.9?
Flags: blocking3.2.9?
(In reply to comment #1)
> Introduced in revision 1933, in 2002:
> 
>   http://bzr.mozilla.org/bugzilla/trunk/revision/1933

which is bug 180205
Flags: blocking4.0?
Flags: blocking4.0+
Flags: blocking3.6.3?
Flags: blocking3.6.3+
Flags: blocking3.4.9?
Flags: blocking3.4.9+
Flags: blocking3.2.9?
Flags: blocking3.2.9+
Attached patch v3 (obsolete) — — Splinter Review 
Addresses justdave's comments.
Attachment #469781 - Attachment is obsolete: true
Attachment #469934 - Flags: review?(LpSolit)
Attachment #469781 - Flags: review?(LpSolit)
(And reed's comment too.)
I don't really understand why, but I can only reproduce on bmo. Locally and on landfill, I cannot.

I added in the URL field a reduced testcase.
URL: report.cgi?cmd-0=1&format=table&query...
Maybe the bug is related to mod_perl?
(In reply to comment #11)
> Maybe the bug is related to mod_perl?

No idea. RedHat, KDE and Mandriva's Bugzilla are not vulnerable. Some of them correctly throw that the format is invalid, and some others say the page doesn't exist. No idea which ones run mod_perl.
bmo returns HTTP/1.1 200 OK while all other installations I tested return HTTP/1.1 302 Found. No idea if that helps or not.
RedHat, KDE, and Mandriva all run mod_cgi. You can tell from testagent.cgi.
By looking at each $cgi->redirect() individually, the one in report.cgi seems to be the only vulnerable one. Some others, such as in long_list.cgi, split the input string on commas and whitespaces, which seems to prevent this specific attack (no idea if it's possible to abuse CGI.pm without including spaces in the string). Also, QuickSearch.pm happily append the alias name to the URL, unfiltered, if the alias is found in the DB. As the alias is limited to 20 characters, I don't know how vulnerable QS is (can you write JS code with only 20 characters?), but probably it would be good to filter the alias there too.

$cgi->query_string(), which is what $buffer gets, escapes the data, which is why buglist.cgi is not vulnerable.
You probably can't squeeze a full XSS payload in 20 chars, but you throw in a partial tag, e.g.:

12345678901234567890
<script src=//ab.pl/

 can confuse the parser in a way that probably leads to one - say, by opening <style>, and then relying on a properly sanitized string displayed later to do something evil (CSS parser resyncs to the nearest } after parse errors).
Comment on attachment 469934 [details] [diff] [review]
v3

Looks good to me, and fixes the problem. r=LpSolit
Attachment #469934 - Flags: review?(LpSolit) → review+
I installed mod_perl 2.0.4 locally, but I couldn't trigger the alert() message. I'm not exactly sure how severe this bug really is. First affected version: Bugzilla 2.17.1.
Depends on: 180205
Flags: approval?
Flags: approval4.0?
Flags: approval3.6?
Flags: approval3.4?
Flags: approval3.2?
Summary: Header injection, cross-site scripting in report.cgi → [SECURITY] Header injection, cross-site scripting in report.cgi
Version: unspecified → 2.17.1
Okay. We do need to figure out how to reproduce this bug reliably on a system other than bmo. Honestly, I'd think that CGI.pm should be responsible for validating that headers don't contain malicious stuff like that, so this shouldn't be possible.
I tested this against a system with mod_perl, and looks like you can still inject at least one HTTP header (e.g. "Refresh" to javascript: URL, which will work in some browsers), multiple headers if you use %0D instead of %0A (works in MSIE and Chrome). I'm pretty sure this is exploitable.
This gets one header injected, "Location" booted off for me:
report-tablebogus%0aInjected-Hdr:val%00

This gets more than one header injected for MSIE/Chrome:
report-tablebogus%0dInjected-Hdr:val%00

The version of mod_perl on my test system is a bit rusty, so let me know if you can't repro.

There are many odd things about this thing that make it difficult to do a quick PoC (e.g., = gets recoded as :, the behavior of %0d changes after %0a, %00 behaves differently), and browser quirks that come into play (^M handling, header parsing precedence, behavior of "Refresh" to javascript:, etc), but I am pretty sure it's possible. If you really need to be convinced, I can dig a bit more.
(In reply to comment #21)
> If you really need to be convinced, I can dig a bit more.

No need. I already reviewed the patch. :)
This works in Chrome for my test instance:

query_format=report-tablebogus%0aRefresh:0%3Bjavascript:alert(1)%00

It used to work in Firefox, but you guys decided to disallow refresh to JS a while ago.
Yeah, thing is, header injection should *never* be possible in a CGI environment. I think that this is actually a bug in mod_perl, if it's only reproducible under mod_perl.
Comment on attachment 469934 [details] [diff] [review]
v3

OK, having played with Michal's example from comment 21 with the alias field, QuickSearch correctly detects e.g. "s%0aId-Hdr:val%00" as a valid alias (the attacker just needs to file a bug and add this string in the alias field), but then, QS is indeed confused by %00 and %0a in the string as QS passed the string unfiltered to $cgi->redirect(). To fix the problem, all you need to do is to add

  $alias = url_quote($alias);

at line 251 of Quicksearch.pm, right after |if $is_alias|.

IMO, this kind of PoC is a valid enough reason to revive bug 313583.
Attachment #469934 - Flags: review+ → review-
Has anybody proven yet that this is only reproducible under mod_perl, and that it can be consistently reproduced there? %0a and %0d should never be translated by the header printer, there's no reason.
My comment 25 is independent of using mod_perl or not.
Flags: approval?
Flags: approval4.0?
Flags: approval3.6?
Flags: approval3.4?
Flags: approval3.2?
(In reply to comment #27)
> My comment 25 is independent of using mod_perl or not.

  Oh, and CGI.pm lets you inject headers that way? In that case, this could be a bug in CGI.pm.
All right, I can confirm that this is a bug in CGI.pm. This will reproduce it:

print $cgi->redirect("http://example.com/\n\n<html><script>alert(1)</script>");

As this may affect many Perl applications, we should first report it to the CGI.pm maintainers and if they agree that it is a bug, coordinate our release with them.
I've informed the CGI.pm maintainers. Also, a slight correction, here's a simple PoC with plain CGI.pm that actually works on my local mod_cgi in Apache 2.2.15:

print $cgi->redirect("\n\rContent-Type: text/html\n\n<html><script>alert(1)</script>");
(In reply to comment #30)
> I've informed the CGI.pm maintainers.

What's the bug ID on rt.cpan.org? I cannot find it.
(In reply to comment #31)
> What's the bug ID on rt.cpan.org? I cannot find it.

  It's a security bug and RT has no mechanism for protecting security bugs, so I emailed the maintainers directly.
As I understand it, the fix will be to require CGI.pm 3.50 or any other relevant version which contains the security fix. I suppose we may still take the patch mkanat attached, in combination with the additional fix I proposed in comment 25.

mkanat, does the CGI.pm maintainer plan to release 3.50 in the near future?
I just emailed the CGI.pm maintainer to ask for an update, but apparently he's out of the office until the 26th.

So that everybody else on this bug knows--the CGI.pm maintainers agree that this is a bug in CGI.pm, and they are going to fix it.

The patch that I attached probably wouldn't fix this security issue in all of Bugzilla--to do that, we'd have to override redirect() in Bugzilla::CGI. However, since this bug affects all systems using CGI.pm, I think that we shouldn't publish it until there's a fix in CGI.pm.
The CGI.pm maintainer says that he will have a release this week that addresses this issue.
Looks like this is now fixed:

  [SECURITY]
  1. embedded newlines are now filtered out of header values in header(). 
     Thanks to Mark Stosberg and Yanick Champoux.
(In reply to comment #36)
>   [SECURITY]
>   1. embedded newlines are now filtered out of header values in header(). 
>      Thanks to Mark Stosberg and Yanick Champoux.

  No, that was an attempted fix, but you actually reported the bug against that version. (The fix was not complete.)
Ah, right, Feb 2010.
Still no new release of CGI.pm. mkanat, could you ping them?
Okay, I pinged them and will let you know what they say.
Flags: blocking3.6.4+
Flags: blocking3.6.3+
Flags: blocking3.4.9+
Flags: blocking3.4.10+
Flags: blocking3.2.9+
Flags: blocking3.2.10+
The CGI.pm release with the fix for this bug should be available later today.

Once it's been out for a little bit, I'm going to send out a Security Advisory simply advising people to update their CGI.pm module, along with posting a patch for all supported branches, updating the CGI.pm requirement to 3.50.
(In reply to comment #41)
> simply advising people to update their CGI.pm module, along with posting a
> patch for all supported branches, updating the CGI.pm requirement to 3.50.

Do we really want to require 3.50 everywhere? This is going to break many installations once they upgrade, because they won't have CGI 3.50 and maybe won't know how to install it exactly. And I'm pretty sure that the Bugzilla RPM provided with some Linux distros will be updated without updating CGI.pm, as it may affect other applications too.
> Do we really want to require 3.50 everywhere? This is going to break many
> installations once they upgrade, because they won't have CGI 3.50 and maybe
> won't know how to install it exactly. And I'm pretty sure that the Bugzilla RPM
> provided with some Linux distros will be updated without updating CGI.pm, as it
> may affect other applications too.

  Well, I don't really want to require 3.50 everywhere, no, but I don't see any reasonable alternative, and this is a very serious security issue.
CGI v3.50 released -- http://www.nntp.perl.org/group/perl.perl5.changes/2010/11/msg28043.html
Attachment #469934 - Attachment is obsolete: true
Attachment #489518 - Flags: review?(mkanat)
(In reply to comment #44)
> Created attachment 489518 [details] [diff] [review]
> Bump minimum required CGI.pm version - v1

Guh, sorry. This patch includes other stuff accidentally.
Attached patch v1 (trunk, 4.0, 3.6) (obsolete) — — Splinter Review 
Real patch this time.
Attachment #489518 - Attachment is obsolete: true
Attachment #489519 - Flags: review?(mkanat)
Attachment #489518 - Flags: review?(mkanat)
Attachment #489519 - Flags: review?(mkanat) → review+
We do need to think somewhat before we raise this requirement on both 3.4 and 3.2.
Flags: approval4.0+
Flags: approval3.6+
Flags: approval3.4?
Flags: approval3.2?
Flags: approval+
(In reply to comment #47)
> We do need to think somewhat before we raise this requirement on both 3.4 and
> 3.2.

For 3.2 and 3.4, I would say the security advisory should be enough. Or maybe add CGI 3.50 as an optional module to get attention when running checksetup.pl? I wouldn't worry too much about 3.2, which is almost EOL. The question remains for 3.4, though.
Hmm. I like the optional module idea. That way sites who trust all their users (like fully-internal corporate installations with a small team) don't *have* to upgrade, but everybody else knows that they should.
Blocks: 611201
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Install/Requirements.pm
Committed revision 7600.
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Install/Requirements.pm
Committed revision 7480.
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Install/Requirements.pm
Committed revision 7207.
Assignee: mkanat → reed
Attached patch v1 (3.4, 3.2) (obsolete) — — Splinter Review 
Something like this?
Attachment #489718 - Flags: review?(mkanat)
Comment on attachment 489718 [details] [diff] [review]
v1 (3.4, 3.2)

Yeah, just like that.
Attachment #489718 - Flags: review?(mkanat) → review+
Attachment #489519 - Attachment description: Bump minimum required CGI.pm version - v1.1 → v1 (trunk, 4.0, 3.6)
Attachment #489718 - Attachment description: Add CGI.pm v3.50 as optional module for 3.2/3.4 branch - v1 → v1 (3.4, 3.2)
Flags: approval3.4?
Flags: approval3.4+
Flags: approval3.2?
Flags: approval3.2+
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.4/
modified Bugzilla/Install/Requirements.pm
Committed revision 6784.
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.2/
modified Bugzilla/Install/Requirements.pm
Committed revision 6405.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
So, I just tested the testcase in the URL field, and I'm still getting an alert(1) dialog, even though I had justdave upgrade the CGI.pm modules on all bmo webheads last night. Are we sure this is correctly fixed upstream?
(In reply to comment #54)
> So, I just tested the testcase in the URL field, and I'm still getting an
> alert(1) dialog, even though I had justdave upgrade the CGI.pm modules on all
> bmo webheads last night. Are we sure this is correctly fixed upstream?

  Okay, that is bad. Is it possible that bmo is not properly using the lib/ directory? Is the lib/ directory missing from Apache's PerlSwitches line? Can I see the PerlSwitches line that the bmo webheads are using?
At the date and time of comment 54, we had reverted to CGI 3.49 on bmo because we were having severe performance issues that coincided with the upgrade and were trying to rule out all of the variables.  The performance issues were determined to be unrelated, and we reupgraded to CGI 3.50 probably an hour or two after you made that comment.  Retest?
hmm, no, retested myself and it's still happening.

> PerlSwitches -I/data/www/bugzilla.mozilla.org -I/data/www/bugzilla.mozilla.org/lib -w -T
> PerlConfigRequire /data/www/bugzilla.mozilla.org/mod_perl.pl

[root@mradm02 bugzilla.mozilla.org]# /data/bin/issue-bugzilla-command grep VERSION= /data/www/bugzilla.mozilla.org/lib/CGI.pm
===pm-app-bugs03===
$CGI::VERSION='3.50';
===pm-app-bugs04===
$CGI::VERSION='3.50';
===pm-app-bugs05===
$CGI::VERSION='3.50';
And yes, Apache on all the webheads has been restarted multiple times since it was re-upgraded.
This is definitely still broken.

I just removed lib/*, ran install-module.pl --all, and redeployed to the webheads, and it's still exploitable, and reporting CGI version 3.50.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I can also, unfortunately, sort of reproduce it on landfill:

https://landfill.bugzilla.org/bugzilla-tip/report.cgi?cmd-0=1&format=table&query_format=report-tablebogus%0A%0DC%3Chtml%3E%3Cscript%3Ealert(1)%3C/script%3E:%20s%0A%0A%0A%0AFOO

Although it's not rendering as text/html, there is still some content being injected into the page.

It's *not* reproducible unless I'm running under mod_perl, so there may also be a separate bug in the mod_perl header-handling code.
(In reply to comment #60)
> It's *not* reproducible unless I'm running under mod_perl

Yeah, I thought so. When reed asked me yesterday to test on my side, I was unable to reproduce locally, using mod_cgi. So we shouldn't send the sec adv before this is entirely fixed.
Thanks for the follow-up. A contribution to the CGI.pm test suite which triggers the remaining case would be welcome. It's less clear to me that this is a CGI.pm issue now. 

There is always the option of validating the input before giving it to CGI.pm, but if there's a way that remains that CGI.pm could be better handling invalid input, I'm continue to be interested to know about it.
I looked into this further. Here's a reduced test case for it which reproduces a behavior with CGI.pm:

 my $cgi = CGI->new('t=bogus%0D%0A<html>');
 my $out = $cgi->redirect( $cgi->param('t') );
 unlike($out, qr/s\n\n<html/, "redirect does not allow double-newline injection");

The issue is that CGI.pm has interpreted the spec strictly, and explicitly interprets CRLF as being CR + LF. The example here uses "\n\n". In mod_perl CGI.pm's call the "send_cgi_header()" method with the value, and your Apache 2.2 then accepts "\n\n" as a valid header/body separator and allows the content through. 

I'll have to consider how we might address this in CGI.pm. A reference how to treat "\n" as a valid CRLF value would be helpful.

Bugzilla also has the alternative of validating untrusted user input before handing it to CGI.pm.
Blocks: 619203
Blocks: 620540
Not to be too nosy, but what's 619203?
(In reply to comment #66)
> Not to be too nosy, but what's 619203?

Same bug as you, but for xml.cgi.
CVE-2010-4571 for the upstream CGI.pm issue.
Whiteboard: (CVE-2010-4571 for CGI.pm)
Whiteboard: (CVE-2010-4571 for CGI.pm) → (CVE-2010-2761 for original CGI.pm issue; CVE-2010-4571 for secondary issue)
Blocks: CVE-2010-4572
(In reply to comment #68)
> CVE-2010-4571 for the upstream CGI.pm issue.

Cancel that. Seems CVE-2010-4411 was already assigned to cover this.
Whiteboard: (CVE-2010-2761 for original CGI.pm issue; CVE-2010-4571 for secondary issue) → (CVE-2010-2761 for original CGI.pm issue; CVE-2010-4411 for secondary issue)
CGI.pm v3.51 was just released.
Attachment #489519 - Attachment is obsolete: true
Attachment #501362 - Flags: review?(mkanat)
Attachment #501362 - Flags: review?(mkanat) → review+
Attached patch v2 (3.4, 3.2) — — Splinter Review 

        Attachment #489718 -
        Attachment is obsolete: true

        Attachment #501363 -
        Flags: review?(mkanat)

    
  

        Attachment #501363 -
        Flags: review?(mkanat) → review+

    
    

    
  
CGI.pm 3.51 is now available via install-module.pl--could we update the requirement on all branches?

    
    

    
  
I can go ahead and land it, but http://search.cpan.org/~markstos/CGI.pm-3.51/ still says "UNAUTHORIZED RELEASE", so I'm a bit skeptical about it.

    
    

    
  
(In reply to comment #73)
> I can go ahead and land it, but http://search.cpan.org/~markstos/CGI.pm-3.51/
> still says "UNAUTHORIZED RELEASE", so I'm a bit skeptical about it.

  Yeah, Mark is working on getting that fixed. install-module still finds it, though.

    
    

    
  
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Install/Requirements.pm
Committed revision 7660.
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Install/Requirements.pm
Committed revision 7518.
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Install/Requirements.pm
Committed revision 7218.
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.4/
modified Bugzilla/Install/Requirements.pm
Committed revision 6786.
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.2/
modified Bugzilla/Install/Requirements.pm
Committed revision 6407.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED

    
    

    
  
Security advisory sent. Removing the security flag.
Group: bugzilla-security

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: