Closed Bug 1433400 (CVE-2018-5123) Opened 6 years ago Closed 6 years ago

Prevent cross-site image requests from leaking contents of certain fields due to regex search

Categories

(Bugzilla :: Reporting/Charting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: hofusec, Assigned: dylan)

References

Details

(Keywords: sec-critical, wsec-csrf, wsec-disclosure, Whiteboard: [reporter-external] [web-bounty-form] [verif?])

Attachments

(4 files, 15 obsolete files)

37.02 KB, image/png
Details
4.80 KB, patch
dkl
: review+
Details | Diff | Splinter Review
3.60 KB, patch
jfearn
: review+
Details | Diff | Splinter Review
3.54 KB, patch
Details | Diff | Splinter Review
Via the image generation in report.cgi it is possible for a third-party website to extract confidential bug information if a logged-in user with access to a confidential bug visits the website. For example, a third-party website can access the summary of a security-sensitive bug report.

The problem exists because via report.cgi complex queries to the bugzilla database can be done and if there is a result, an image will be returned, but if the query has no result no image is returned. Because of this a third-party website can extract information by detecting if certain images can be loaded or not.

POC:
1.) download attached poc.html
2.) login to bugzilla
3.) open poc.html and enter a bug id of a confidential bug and the url of the bugzilla installation (I have tested the poc with URL: https://bugzilla.mozilla.org and Bug ID: 1395143)
4.) click on “Get Summary”

After a while, most of the summary is displayed at the html file.
Flags: sec-bounty?
Attached file poc.html
Holger: thank you for your report, the PoC is very creative and really helps makes this more visible and easy to understand and test.  That said, I did test this on multiple BMO accounts (1) with access to bug 1395143 and (2) without access to the bug.  In my testing with an account that has access to the bug, it's not surprising that it was able to enumerate the bug summary, but that account already has access to that bug.  In the test case without access to the bug, it just keeps attemping, but recognizes the char set as ".........." and continues to try without enumerating any content.  Again, bang up job on the PoC presentation, but I wasn't able to replicate the behavior where a BMO user without access to the private bug could enumerate the bug summary.  Please let me know if you think I'm doing this incorrectly and we can give it a go again.
On second review, I misread the original report, the issue you are stating is that a 3rd party site can access information from a user with access to the bug.  I'm re-reviewing this now given that context.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-high, wsec-xss
Ok, I've confirmed the behavior as described.  I'll raise with the service owner now.  Again, thanks for the report!
Group: websites-security → bugzilla-security
Component: Other → General
Product: Websites → bugzilla.mozilla.org
Version: unspecified → Production
Assignee: nobody → dylan
So, to try and simplify the PoC a little more for easier digestion...

Confirming first char X:

https://bugzilla.mozilla.org/report.cgi?bug_id=1395143&bug_id_type=anyexact&short_desc=^x&short_desc_type=regexp&x_axis_field=short_desc&format=bar&ctype=png&action=plot&width=60&height=35

Confirming first char is NOT Y:

https://bugzilla.mozilla.org/report.cgi?bug_id=1395143&bug_id_type=anyexact&short_desc=^y&short_desc_type=regexp&x_axis_field=short_desc&format=bar&ctype=png&action=plot&width=60&height=35

The issue being that a remote site can produce these URLs to leverage the users BMO session to generate them.  My initial thoughts here is that the browser should prevent this from happening.
This is a bug in upstream bugzilla as well.

I'm cc'ing jfearn for bugzilla.redhat.com on this so we can coordinate a fix.
I will also need to push out new bugzilla 5 and bugzilla 4.4 releases.
Component: General → Reporting/Charting
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: default-qa
Target Milestone: --- → Bugzilla 4.4
Version: Production → unspecified
We have one (load-balancer-based) mitigation deployed to bugzilla.mozilla.org. I'm working on a more general fix for the code and we'll be doing a security advisory for all bugzillas.
CVE-2018-5123 has been assigned to this issue.
Alias: CVE-2018-5123
Attached patch bmo patch 1 (obsolete) — Splinter Review
First patch for bmo. Also testing patches for 4.2, 4.4, 5.0 and master.
Attachment #8945927 - Flags: review?(dkl)
Attached patch 1433400_1_bmo.patch (obsolete) — Splinter Review
Man I hate patches. That was the wrong one. here is the right one.
Attachment #8945927 - Attachment is obsolete: true
Attachment #8945927 - Flags: review?(dkl)
Attachment #8945932 - Flags: review?(dkl)
Attached patch 1433400_2_bugzilla_4.4.patch (obsolete) — Splinter Review
Attachment #8946168 - Flags: review?(jfearn)
Attached patch 1433400_2_bmo.patch (obsolete) — Splinter Review
This is only different in that the regex for adding a / doesn't chop off the last not-/ character. An edge case which can't happen in BMO.
Attachment #8946169 - Flags: review?(dkl)
Attachment #8945932 - Attachment is obsolete: true
Attachment #8945932 - Flags: review?(dkl)
:emorley, in response to your problems with bug 1433493:

In theory, we could lock the Referrer patches down to only domains that we trust to not to run arbitrary JavaScript. Without a really strong Content Security Policy (currently not in place), I wouldn't trust any Etherpad codebase to not be vulnerable to XSS attacks, which could then be used against BMO. The codebase has a long history of XSS problems, which is why it was moved off of mozilla.org in the first place.  :\
Flags: sec-bounty? → sec-bounty+
Attached patch 1433400_3_bmo.patch (obsolete) — Splinter Review
The regex wasn't correct for cases where the referer policy is 'origin'.
Attachment #8946169 - Attachment is obsolete: true
Attachment #8946169 - Flags: review?(dkl)
Attachment #8946432 - Flags: review?(dkl)
(In reply to April King [:April] from comment #14)
> :emorley, in response to your problems with bug 1433493:
> 
> In theory, we could lock the Referrer patches down to only domains that we
> trust to not to run arbitrary JavaScript. Without a really strong Content
> Security Policy (currently not in place), I wouldn't trust any Etherpad
> codebase to not be vulnerable to XSS attacks, which could then be used
> against BMO. The codebase has a long history of XSS problems, which is why
> it was moved off of mozilla.org in the first place.  :\

Note that we're only blocking non-html in the final patch. The mitigation is dumber than the final fix.
Linking to images can't work.
Attached patch 1433400_3.1_bmo.patch (obsolete) — Splinter Review
Attachment #8946432 - Attachment is obsolete: true
Attachment #8946432 - Flags: review?(dkl)
Attached patch 1433400_3_bugzilla_4.4.patch (obsolete) — Splinter Review
Attachment #8946168 - Attachment is obsolete: true
Attachment #8946168 - Flags: review?(jfearn)
Attachment #8946439 - Flags: review?(jfearn)
Attachment #8946436 - Flags: review?(dkl)
Attached patch 1433400_3_bugzilla_5.0.patch (obsolete) — Splinter Review
Here is the patch to 5.0, which is somewhere between the bmo patch and the 4.4 patch. It will probably cleanly apply to master as well.
Comment on attachment 8946436 [details] [diff] [review]
1433400_3.1_bmo.patch

Review of attachment 8946436 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/CGI.pm
@@ +444,5 @@
>      }
>  
> +    if (Bugzilla->usage_mode == USAGE_MODE_BROWSER) {
> +
> +        # Safe content types are ones that arn't images.

aren't

@@ +450,5 @@
> +        my $content_type         = $headers{'-type'} // $headers{'-content_type'} // 'text/html';
> +        my $is_safe_content_type = ( $content_type eq 'text/html' || $content_type eq 'text/plain' );
> +
> +        # Safe referers are ones that begin with the urlbase.
> +        my $referer = $self->referer;

Isn't the referer easily spoofable?
It is, but not from within a web browser.

The person with privileged access would have to be coerced into spoofing their referrer, and there's no easy way to make that happen.
Comment on attachment 8946436 [details] [diff] [review]
1433400_3.1_bmo.patch

Review of attachment 8946436 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to April King [:April] from comment #21)
> It is, but not from within a web browser.
> 
> The person with privileged access would have to be coerced into spoofing
> their referrer, and there's no easy way to make that happen.

Ah correct. I was able to do so but had to install a simple web extension. The victim would have to intentionally do so.

With that, this looks good and fixes the provided exploit for me. r=dkl
Attachment #8946436 - Flags: review?(dkl) → review+
Comment on attachment 8946439 [details] [diff] [review]
1433400_3_bugzilla_4.4.patch

Review of attachment 8946439 [details] [diff] [review]:
-----------------------------------------------------------------

Aside from the unrelated white space changes this looks good :)
Attachment #8946439 - Flags: review?(jfearn) → review+
very good. 

dkl: 

let's sync up this week about doing the security release sometime next week.
Flags: needinfo?(dkl)
Comment on attachment 8946439 [details] [diff] [review]
1433400_3_bugzilla_4.4.patch

Review of attachment 8946439 [details] [diff] [review]:
-----------------------------------------------------------------

On furtehr testing this patch breaks searches in Firefox, because the content type doesn't match.

content_type: multipart/x-mixed-replace;boundary="------- =_lh0NRAkY1DxpHkn5K"
Attachment #8946439 - Flags: review+ → review-
Attached patch 1433400_4_bmo.patch (obsolete) — Splinter Review
This one allows multipart responses and uses a regex that is faster (between 10 and 40 operations instead of 60-200 operations)

However I think have found another problem -- this will block attachments when the attachment base is not the url base.

I'll have to do a few more special things for attachments.

I will carry forward the r+, but dkl can look over this version. The next one will just have an additional option to $cgi->header(): allow_unsafe_referer.
Attachment #8946436 - Attachment is obsolete: true
Attachment #8948556 - Flags: review+
Attached patch 1433400_5_bmo.patch (obsolete) — Splinter Review
Attachment #8946439 - Attachment is obsolete: true
Attachment #8946443 - Attachment is obsolete: true
Attachment #8948556 - Attachment is obsolete: true
Attachment #8948658 - Flags: review?(dkl)
Comment on attachment 8948658 [details] [diff] [review]
1433400_5_bmo.patch

Review of attachment 8948658 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl
Attachment #8948658 - Flags: review?(dkl) → review+
Summary: Bugzilla report.cgi cross-site extraction of confidential bug information → Prevent cross-site image requests from leaking contents of certain fields due to regex search
Attached patch 1433400_5_bugzilla-4.4.patch (obsolete) — Splinter Review
Attachment #8949138 - Flags: review?(jfearn)
Attached patch 1433400_5_bugzilla-5.0.patch (obsolete) — Splinter Review
Attachment #8949142 - Flags: review?(jfearn)
Attached patch 1433400_5_bugzilla-master.patch (obsolete) — Splinter Review
Attachment #8949143 - Flags: review?(jfearn)
Attached patch 1433400_6_bugzilla-4.4.patch (obsolete) — Splinter Review
Okay. Let's start with 4.4 and if there are no issues with this, I'll do 5.0 and master (and 4.2 for good measure)
Attachment #8949138 - Attachment is obsolete: true
Attachment #8949142 - Attachment is obsolete: true
Attachment #8949143 - Attachment is obsolete: true
Attachment #8949138 - Flags: review?(jfearn)
Attachment #8949142 - Flags: review?(jfearn)
Attachment #8949143 - Flags: review?(jfearn)
Attachment #8950460 - Flags: review?(jfearn)
Bah, one more revision, Let's consider the following content types "safe":

text/html
application/rdf+xml
application/atom+xml
application/xml
application/xml-dtd
application/x-javascript
application/json
text/csv
text/calendar
text/plain

This should take care of the situation you noticed on BRC, right?
Flags: needinfo?(jfearn)
Attachment #8948658 - Attachment is obsolete: true
Attachment #8950460 - Attachment is obsolete: true
Attachment #8950460 - Flags: review?(jfearn)
Attachment #8950461 - Flags: review?(dkl)
Attached patch 1433400_7_bugzilla-4.4.patch (obsolete) — Splinter Review
Attachment #8950462 - Flags: review?(jfearn)
Comment on attachment 8950462 [details] [diff] [review]
1433400_7_bugzilla-4.4.patch

Review of attachment 8950462 [details] [diff] [review]:
-----------------------------------------------------------------

state is too new, if you s/state/my/ it works fine.
Attachment #8950462 - Flags: review?(jfearn) → review-
(In reply to Dylan Hardison [:dylan] (he/him) from comment #33)
> Bah, one more revision, Let's consider the following content types "safe":
> 
> text/html
> application/rdf+xml
> application/atom+xml
> application/xml
> application/xml-dtd
> application/x-javascript
> application/json
> text/csv
> text/calendar
> text/plain
> 
> This should take care of the situation you noticed on BRC, right?

Yeah I think this is correct, I'm not absolutely sure you can't tunnel queries through any of those formats though.
Flags: needinfo?(jfearn)
The only dangerous one would be text/xml, if we had something producing SVGs. otherwise the onload/onerror communication channel fails to work. So that's good.

per our irc chat, I'll revise the 4.4 patch to avoid using the 'state' keyword since the files back in 4.4-land were not yet using the more-than-a-decade-old perl 5.10 syntax... :(
Without 'state'
Attachment #8950462 - Attachment is obsolete: true
Attachment #8950824 - Flags: review?(jfearn)
Comment on attachment 8950824 [details] [diff] [review]
1433400_7_bugzilla-4.4.patch

Review of attachment 8950824 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me!
Attachment #8950824 - Flags: review?(jfearn) → review+
dkl will be doing a release tomorrow.
Flags: needinfo?(dkl)
Do we know when the upstream patches and CVE will be going live?
dkl has started the release process
Blocks: 1438592
Attachment #8950461 - Flags: review?(dkl) → review+
when do we close and unrestrict this bug?
Flags: needinfo?(dkl)
Opening up as this is now public.
Group: bugzilla-security
Flags: needinfo?(dkl)
This has been committed to all related repos.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Thanks for handling the release dkl!
Also thanks to both dkl and jfearn for reviewing the half-dozen revisions to this fix.
Comment on attachment 8951341 [details] [diff] [review]
1433400_7_bugzilla-5.0.patch

>--- a/Bugzilla/CGI.pm

>+# responding to text/plain or text/html is safe

This is confusing. This comment gives the feeling that those are the only two safe content types. You have to read the regexp to discover that there is more than that.


>+sub _prevent_unsafe_response {
>+    my ($self, $headers) = @_;
>+    my $safe_content_type_re = qr{
>+        ^ (*COMMIT) # COMMIT makes the regex faster
>+                    # by preventing back-tracking. see also perldoc pelre.
>+        # application/x-javascript, xml, atom+xml, rdf+xml, xml-dtd, and json
>+        (?: application/ (?: x(?: -javascript | ml (?: -dtd )? )
>+                           | (?: atom | rdf) \+ xml
>+                           | json )
>+        # text/csv, text/calendar, text/plain, and text/html
>+          | text/ (?: c (?: alendar | sv )

Wouldn't it be more readable to write calendar | csv instead of this ugly c (?: alendar | sv ) ?


>+                    | plain
>+                    | html )
>+        # used for HTTP push responses
>+          | multipart/x-mixed-replace)
>+    }sx;
>+    my $safe_referer_re = do {
>+        # Note that urlbase must end with a /.
>+        # It almost certainly does, but let's be extra careful.
>+        my $urlbase = correct_urlbase();
>+        $urlbase =~ s{/$}{};
>+        qr{
>+            # Begins with literal urlbase
>+            ^ (*COMMIT)
>+            \Q$urlbase\E
>+            # followed by a slash or end of string
>+            (?: /
>+              | $ )
>+        }sx
>+    };
>+
>+    return if $ALLOW_UNSAFE_RESPONSE;

What's the point of returning only now if $ALLOW_UNSAFE_RESPONSE is true instead of doing it immediately in this method, before anything else (especially calling correct_urlbase())?


>+        if (!$is_safe_referer && !$is_safe_content_type) {
>+            print $self->SUPER::header(-type => 'text/html',  -status => '403 Forbidden');
>+            if ($content_type ne 'text/html') {

This last check is useless. You already know that $content_type is not 'text/html', else $is_safe_content_type would be true and you wouldn't enter this block at all.


Could the PoC be made public so that other security experts have the possibility to see how it works, please?
Blocks: 1438992
Holger, how would you like to be credited on our hall of fame? If you have a site you would like us to link to, we can do that as well.

Thanks for your submission and for participating in our bug bounty program!

Hello,

Looks like this fix is prevent to update Bugzilla from version 4.4.12 to 4.4.13. Problem with: ^ (*COMMIT)

patch -p1 < bugzilla-4.4.12-to-4.4.13-nodocs.diff

patching file Bugzilla/CGI.pm
patching file Bugzilla/Constants.pm
patching file Bugzilla/DB/Sqlite.pm
patching file attachment.cgi
patching file taskgraph.json
patching file template/en/default/pages/release-notes.html.tmpl

./checksetup.pl

  • This is Bugzilla 4.4.13 on perl 5.8.8
  • Running on Linux 2.6.18-xenU-ec2-v1.0 #2 SMP Tue Feb 19 10:51:53 EST 2008

Checking perl modules...
Checking for CGI.pm (v3.51) ok: found v4.21
Checking for Digest-SHA (any) ok: found v5.95
Checking for TimeDate (v2.23) ok: found v2.24
Checking for DateTime (v0.28) ok: found v1.20
Checking for DateTime-TimeZone (v0.71) ok: found v1.93
Checking for DBI (v1.54) ok: found v1.634
Checking for Template-Toolkit (v2.22) ok: found v2.22
Checking for Email-Send (v2.04) ok: found v2.198
Checking for Email-MIME (v1.904) ok: found v1.936
Checking for URI (v1.37) ok: found v1.69
Checking for List-MoreUtils (v0.32) ok: found v0.413
Checking for Math-Random-ISAAC (v1.0.1) ok: found v1.004

Checking available perl DBD modules...
Checking for DBD-Pg (v2.7.0) not found
Checking for DBD-mysql (v4.001) ok: found v4.032
Checking for DBD-SQLite (v1.29) not found
Checking for DBD-Oracle (v1.19) not found

The following Perl modules are optional:
Checking for GD (v1.20) ok: found v2.45
Checking for Chart (v2.1.0) ok: found v2.4.1
Checking for Template-GD (any) ok: found v1.56
Checking for GDTextUtil (any) ok: found v0.86
Checking for GDGraph (any) ok: found v1.44
Checking for MIME-tools (v5.406) ok: found v5.427
Checking for libwww-perl (any) ok: found v5.834
Checking for XML-Twig (any) not found
Checking for PatchReader (v0.9.6) ok: found v0.9.6
Checking for perl-ldap (any) ok: found v0.39
Checking for Authen-SASL (any) ok: found v2.10
Checking for Net-SMTP-SSL (v1.01) ok: found v1.01
Checking for RadiusPerl (any) not found
Checking for SOAP-Lite (v0.712) ok: found v1.11
Checking for XMLRPC-Lite (v0.712) not found
Checking for JSON-RPC (any) not found
Checking for JSON-XS (v2.0) not found
Checking for Test-Taint (any) ok: found v1.06
Checking for HTML-Parser (v3.40) ok: found v3.71
Checking for HTML-Scrubber (any) ok: found v0.08
Checking for Encode (v2.21) ok: found v2.78
Checking for Encode-Detect (any) not found
Checking for Email-Reply (any) ok: found v1.202
Checking for HTML-FormatText-WithLinks (v0.13) not found
Checking for TheSchwartz (v1.07) not found
Checking for Daemon-Generic (any) not found
Checking for File-Slurp (v9999.13) not found
Checking for mod_perl (v1.999022) ok: found v2.000005
Checking for Apache-SizeLimit (v0.96) not found
Checking for File-MimeInfo (any) not found
Checking for IO-stringy (any) ok: found v2.110
Checking for mod_headers (any) ok
Checking for mod_expires (any) ok
Checking for mod_env (any) ok


  • OPTIONAL MODULES *

  • Certain Perl modules are not required by Bugzilla, but by *
  • installing the latest version you gain access to additional *
  • features. *
  •                                                                 *
    
  • The optional modules you do not have installed are listed below, *
  • with the name of the feature they enable. Below that table are the *
  • commands to install each module. *

  •           MODULE NAME * ENABLES FEATURE(S)                      *
    

  •              XML-Twig * Move Bugs Between Installations, Automatic Update Notifications *
    
  •            RadiusPerl * RADIUS Authentication                   *
    
  •           XMLRPC-Lite * XML-RPC Interface                       *
    
  •              JSON-RPC * JSON-RPC Interface                      *
    
  •               JSON-XS * Make JSON-RPC Faster                    *
    
  •         Encode-Detect * Automatic charset detection for text attachments *
    
  • HTML-FormatText-WithLinks * Inbound Email *
  •           TheSchwartz * Mail Queueing                           *
    
  •        Daemon-Generic * Mail Queueing                           *
    
  •            File-Slurp * Mail Queueing                           *
    
  •      Apache-SizeLimit * mod_perl                                *
    
  •         File-MimeInfo * Sniff MIME type of attachments          *
    

COMMANDS TO INSTALL OPTIONAL MODULES:

   XML-Twig: /usr/bin/perl install-module.pl XML::Twig
 RadiusPerl: /usr/bin/perl install-module.pl Authen::Radius
XMLRPC-Lite: /usr/bin/perl install-module.pl XMLRPC::Lite
   JSON-RPC: /usr/bin/perl install-module.pl JSON::RPC
    JSON-XS: /usr/bin/perl install-module.pl JSON::XS

Encode-Detect: /usr/bin/perl install-module.pl Encode::Detect
HTML-FormatText-WithLinks: /usr/bin/perl install-module.pl HTML::FormatText::WithLinks
TheSchwartz: /usr/bin/perl install-module.pl TheSchwartz
Daemon-Generic: /usr/bin/perl install-module.pl Daemon::Generic
File-Slurp: /usr/bin/perl install-module.pl File::Slurp
Apache-SizeLimit: /usr/bin/perl install-module.pl Apache2::SizeLimit
File-MimeInfo: /usr/bin/perl install-module.pl File::MimeInfo::Magic

To attempt an automatic install of every required and optional module
with one command, do:

/usr/bin/perl install-module.pl --all

Quantifier follows nothing in regex; marked by <-- HERE in m/
^ (* <-- HERE COMMIT) # COMMIT makes the regex faster
# by preventing back-tracking. see also perldoc pelre.
# application/x-javascript, xml, atom+xml, rdf+xml, xml-dtd, and json
(?: application/ (?: x(?: -javascript | ml (?: -dtd )? )
| (?: atom | rdf) + xml
| json )
# text/csv, text/calendar, text/plain, and text/html
| text/ (?: c (?: alendar | sv )
| plain
| html )
# used for HTTP push responses
| multipart/x-mixed-replace)
/ at Bugzilla/CGI.pm line 306, <DATA> line 275.
Compilation failed in require at Bugzilla.pm line 25, <DATA> line 275.
BEGIN failed--compilation aborted at Bugzilla.pm line 25, <DATA> line 275.
Compilation failed in require at ./checksetup.pl line 73, <DATA> line 275.

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

Attachment

General

Created:
Updated:
Size: