Last Comment Bug 600464 - (CVE-2010-3172) [SECURITY] Content/Header injection due to non-random multipart/x-mixed-replace boundary
(CVE-2010-3172)
: [SECURITY] Content/Header injection due to non-random multipart/x-mixed-repla...
Status: VERIFIED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 2.10
: All All
: P1 critical (vote)
: Bugzilla 3.2
Assigned To: Byron Jones ‹:glob› [PTO until 2016-10-10]
: default-qa
:
Mentors:
buglist.cgi?query_format=advanced&sho...
Depends on:
Blocks: 606854
  Show dependency treegraph
 
Reported: 2010-09-28 22:05 PDT by Masahiro YAMADA
Modified: 2010-12-01 10:56 PST (History)
11 users (show)
LpSolit: approval+
LpSolit: approval4.0+
LpSolit: blocking4.0+
LpSolit: approval3.6+
LpSolit: blocking3.6.3+
LpSolit: approval3.4+
LpSolit: blocking3.4.9+
LpSolit: approval3.2+
LpSolit: blocking3.2.9+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
randomise mime boundary.1 (696 bytes, patch)
2010-09-28 23:43 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review-
Details | Diff | Splinter Review
randomise mime boundary.2 (543 bytes, patch)
2010-09-29 03:34 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
mkanat: review+
Details | Diff | Splinter Review

Description Masahiro YAMADA 2010-09-28 22:05:38 PDT
Search result of b.m.o. does not escape "--------- =_aaaaaaaaaa0": it is used as boudary of multipart/x-mixed-replace.

Attackers can inject boundary of multipart/x-mixed-replace.
It may be able to be used for HTTP Header injection.

Example:
https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc=---------%20%3D_aaaaaaaaaa0&short_desc_type=allwordssubstr&resolution=---&resolution=DUPLICATE
Comment 1 Masahiro YAMADA 2010-09-28 22:08:38 PDT
I think, there are two solutions.
1. Change boudary text to be random.
2. If search result contains boundary text, replace it to HTML character reference.
Comment 2 Reed Loden [:reed] (use needinfo?) 2010-09-28 23:25:06 PDT
This is really a bug with upstream CGI module... The default boundary is hardcoded when it should be randomized.

sub multipart_init {
    my($self,@p) = self_or_default(@_);
    my($boundary,@other) = rearrange_header([BOUNDARY],@p);
    $boundary = $boundary || '------- =_aaaaaaaaaa0';
    $self->{'separator'} = "$CRLF--$boundary$CRLF";
    $self->{'final_separator'} = "$CRLF--$boundary--$CRLF";
    $type = SERVER_PUSH($boundary);
    return $self->header(
	-nph => 0,
	-type => $type,
	(map { split "=", $_, 2 } @other),
    ) . "WARNING: YOUR BROWSER DOESN'T SUPPORT THIS SERVER-PUSH TECHNOLOGY." . $self->multipart_end;
}
Comment 3 Byron Jones ‹:glob› [PTO until 2016-10-10] 2010-09-28 23:43:15 PDT
Created attachment 479324 [details] [diff] [review]
randomise mime boundary.1

we already overwrite multipart_init, so the fix is trivial.
Comment 4 Byron Jones ‹:glob› [PTO until 2016-10-10] 2010-09-29 00:15:02 PDT
i have emailed the maintainers of CGI.pm to make them aware of this issue.
Comment 5 Gervase Markham [:gerv] 2010-09-29 02:00:22 PDT
Well played, sir! Excellent bug-finding skills. 

The fix looks fine to me, although someone might claim there is a slightly more performant way to do it. But this code only runs once per request, so clarity probably wins. You might want to add a comment about why you are doing it.

Gerv
Comment 6 Frédéric Buclin 2010-09-29 03:20:05 PDT
Comment on attachment 479324 [details] [diff] [review]
randomise mime boundary.1

>+        my @chrs = ('0'..'9', 'A'..'Z', 'a'..'z');
>+        foreach (0..16) {
>+            $boundary .= $chrs[rand(scalar @chrs)];
>+        }

This code already exists in generate_random_password(); no need to duplicate it. Please use it instead.
Comment 7 Byron Jones ‹:glob› [PTO until 2016-10-10] 2010-09-29 03:34:14 PDT
Created attachment 479344 [details] [diff] [review]
randomise mime boundary.2
Comment 9 Dave Miller [:justdave] (justdave@bugzilla.org) 2010-09-29 14:09:40 PDT
(In reply to comment #4)
> i have emailed the maintainers of CGI.pm to make them aware of this issue.

Did you let them know we already have a CVE number assigned for it?  I assume the number can be given to them at this point since it's actually they're bug and we're working around it.  Or does the fact that we were already overriding that subroutine make ours a separate issue?
Comment 10 Dave Miller [:justdave] (justdave@bugzilla.org) 2010-09-29 14:10:11 PDT
s/they're/their/
Comment 11 Dave Miller [:justdave] (justdave@bugzilla.org) 2010-09-29 14:12:33 PDT
(In reply to comment #6)
> Comment on attachment 479324 [details] [diff] [review]
> randomise mime boundary.1
> 
> >+        my @chrs = ('0'..'9', 'A'..'Z', 'a'..'z');
> >+        foreach (0..16) {
> >+            $boundary .= $chrs[rand(scalar @chrs)];
> >+        }
> 
> This code already exists in generate_random_password(); no need to duplicate
> it. Please use it instead.

Using our generate_random_password() routine is not upward-portable back to the upstream CGI module, however, since they don't have that routine.
Comment 12 Reed Loden [:reed] (use needinfo?) 2010-09-30 00:06:50 PDT
(In reply to comment #9)
> Did you let them know we already have a CVE number assigned for it?  I assume
> the number can be given to them at this point since it's actually they're bug
> and we're working around it.  Or does the fact that we were already overriding
> that subroutine make ours a separate issue?

The CVE for this bug is solely for the Bugzilla-specific fix, as it's not actually patching the bug within CGI.pm but in our override function. However, if the CGI.pm maintainers need a CVE to be assigned for them, that can be easily arranged.
Comment 13 Masahiro YAMADA 2010-09-30 04:23:00 PDT
CGI::Simple seems to have same problem.

http://cpansearch.perl.org/src/ANDYA/CGI-Simple-1.112/lib/CGI/Simple.pm
Comment 14 Masahiro YAMADA 2010-09-30 04:47:52 PDT
FYI:
It might be better to write documentation that describes why randomization is needed.

I cannot find any documentation that describes "why boundary is needed to randomized on dynamic contents that uses multipart/x-mixed-replace".

Almost of documentations and example uses non-random boundary.
  for example: http://perldoc.perl.org/CGI.html#Server-Push

I can find only two documents that suggests randomization of boundary in example. But both of them does not describe why randomize is needed.
   (1)Original Documentation of Netscape uses "--ThisRandomText".
      http://web.archive.org/web/20060424004246/wp.netscape.com/assist/net_sites/pushpull.html
   (2)O'Reilly Koala book (HTML: The Definitive Guide) uses "--SomeRandomText".
       (Latest edition is renamed to "HTML & XHTML : The Definitive Guide")
Comment 15 Masahiro YAMADA 2010-09-30 04:58:04 PDT
Source code list that may have same problem:

http://www.google.com/codesearch?lr=&q=multipart_init\s*\%28+lang%3Aperl+file%3A[^C][^G][^I][^\.][^p][^m]%24
Comment 16 Mark Stosberg 2010-09-30 08:27:00 PDT
This page explains how x-mixed-replace is designed to be used. It's also makes it clear how a known boundary could

http://www.abiglime.com/webmaster/articles/cgi/032498.htm

It's not clear to me what an attacker would gain by using the known boundary. They already have the ability to send content. x-mixed-replace with a known boundary allows them to send two copies of the content and have the second replace the first. 

Perhaps the issue is a case in which the first copy of the content is "good" content from a victim, but an attacker manages to craft a URL so that the good content is replaced with second piece of content which is malicious?

Having a clearer picture of how an exploit might would be helpful.
Comment 17 Byron Jones ‹:glob› [PTO until 2016-10-10] 2010-09-30 09:01:55 PDT
it's a variation on 'http response splitting', which can be used to perform cross-site scripting attacks, cross-user defacement, web cache poisoning, and similar exploits.

http://cwe.mitre.org/data/definitions/113.html
Comment 18 Masahiro YAMADA 2010-10-14 03:02:59 PDT
I think there are two patterns of attack for this bug.

1. set "--------- =_aaaaaaaaaa0" to query parameter. (for example, comment #8)

2. set "--------- =_aaaaaaaaaa0" to description of bug.
   I did not test, because it is too dangerous to test at here. It may fail.
Comment 20 Masahiro YAMADA 2010-10-14 04:07:07 PDT
(In reply to comment #19)
> Attackers can inject ANY HTML tags code using x-mac-arabic
> 
And UTF-7, x-max-farasi and x-mac-hebrew can be used too (see bug 601429).
Comment 21 Masahiro YAMADA 2010-10-20 03:28:33 PDT
(In reply to comment #18)
> I think there are two patterns of attack for this bug.
> 
> 2. set "--------- =_aaaaaaaaaa0" to description of bug.
>    I did not test, because it is too dangerous to test at here. It may fail.

At bugzilla, CRLF in Summary of bug is not allowed, and CRLF will be converted to SPACE.
As a result of this convert, What attackers can do with bug description (Summary) seems to be only "breaking response".
Comment 22 Max Kanat-Alexander 2010-10-31 18:02:36 PDT
Wow, this problem actually goes back to the VERY FIRST CHECKIN EVER for Bugzilla, meaning that it was in Bugzilla 1.0, probably, even.
Comment 23 Frédéric Buclin 2010-11-02 16:25:16 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/CGI.pm
Committed revision 7590.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/CGI.pm
Committed revision 7472.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/CGI.pm
Committed revision 7200.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.4/
modified Bugzilla/CGI.pm
Committed revision 6781.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.2/
modified Bugzilla/CGI.pm
Committed revision 6402.
Comment 24 Max Kanat-Alexander 2010-11-02 19:15:02 PDT
Security advisory sent, unlocking bug.
Comment 25 Max Kanat-Alexander 2010-11-02 19:24:46 PDT
Re-locking so as to not 0-day CGI.pm.
Comment 26 Masahiro YAMADA 2010-11-03 03:58:27 PDT
Secuyity Advisory http://www.bugzilla.org/security/3.2.8/ seems to contain MOJIBAKE in Credits.
> Frédéric Buclin
Comment 27 Frédéric Buclin 2010-11-03 05:46:09 PDT
(In reply to comment #26)
> Secuyity Advisory http://www.bugzilla.org/security/3.2.8/ seems to contain
> MOJIBAKE in Credits.
> > Frédéric Buclin

I fixed it. Thanks for catching that! (I saw this encoding problem in the security advisory, but I thought this was a problem with Bugzilla itself when displaying the advisory.)
Comment 28 Masahiro YAMADA 2010-11-03 07:08:08 PDT
-> VERIFIED
Now boundary is randomized.
Comment 29 Reed Loden [:reed] (use needinfo?) 2010-11-10 09:11:53 PST
CGI v3.50 released -- http://www.nntp.perl.org/group/perl.perl5.changes/2010/11/msg28043.html
Comment 30 Max Kanat-Alexander 2010-11-10 16:25:35 PST
The CGI.pm with the fix has been released, unlocking this bug.
Comment 31 Mark Stosberg 2010-11-10 16:42:35 PST
I have submitted a patch to CGI::Simple now, based on the CGI.pm fix.
Comment 32 Tom "spot" Callaway 2010-12-01 10:24:43 PST
(In reply to comment #31)
> I have submitted a patch to CGI::Simple now, based on the CGI.pm fix.

Mark, can you attach a copy of that patch (or email it to me : tcallawa@redhat.com) ?

Thanks.
Comment 33 Tom "spot" Callaway 2010-12-01 10:44:53 PST
(In reply to comment #32)
> (In reply to comment #31)
> > I have submitted a patch to CGI::Simple now, based on the CGI.pm fix.
> 
> Mark, can you attach a copy of that patch (or email it to me :
> tcallawa@redhat.com) ?

Never mind, found it here:
https://github.com/AndyA/CGI--Simple/commit/e4942b871a26c1317a175a91ebb7262eea59b380

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