1433400 - (CVE-2018-5123) Prevent cross-site image requests from leaking contents of certain fields due to regex search

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Holger Fuhrmannek]

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.

Comment 1

[Holger Fuhrmannek]

Attachment: poc.html

Comment 2

[Jonathan Claudius [:claudijd] (use NEEDINFO)]

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.

Comment 3

[Jonathan Claudius [:claudijd] (use NEEDINFO)]

Attachment: Screen Shot 2018-01-26 at 9.25.58 AM.png

Comment 4

[Jonathan Claudius [:claudijd] (use NEEDINFO)]

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.

Comment 5

[Jonathan Claudius [:claudijd] (use NEEDINFO)]

Ok, I've confirmed the behavior as described.  I'll raise with the service owner now.  Again, thanks for the report!

Comment 6

[Jonathan Claudius [:claudijd] (use NEEDINFO)]

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.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

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.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

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.

Comment 9

[Daniel Veditz [:dveditz]]

CVE-2018-5123 has been assigned to this issue.

Comment 10

[Dylan Hardison [:dylan] (he/him)]

Attachment: bmo patch 1

First patch for bmo. Also testing patches for 4.2, 4.4, 5.0 and master.

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_1_bmo.patch

Man I hate patches. That was the wrong one. here is the right one.

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_2_bugzilla_4.4.patch

Comment 13

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_2_bmo.patch

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.

Comment 14

[April King [:April]]

: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.  :\

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_3_bmo.patch

The regex wasn't correct for cases where the referer policy is 'origin'.

Comment 16

[Dylan Hardison [:dylan] (he/him)]

(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.

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_3.1_bmo.patch

Comment 18

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_3_bugzilla_4.4.patch

Comment 19

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_3_bugzilla_5.0.patch

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 20

[David Lawrence [:dkl]]

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?

Comment 21

[April King [:April]]

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 22

[David Lawrence [:dkl]]

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

Comment 23

[Jeff Fearn 🐞]

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 :)

Comment 24

[Dylan Hardison [:dylan] (he/him)]

very good. 

dkl: 

let's sync up this week about doing the security release sometime next week.

Comment 25

[Jeff Fearn 🐞]

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"

Comment 26

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_4_bmo.patch

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.

Comment 27

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_5_bmo.patch

Comment 28

[David Lawrence [:dkl]]

Comment on attachment 8948658 [details] [diff] [review]
1433400_5_bmo.patch

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

r=dkl

Comment 29

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_5_bugzilla-4.4.patch

Comment 30

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_5_bugzilla-5.0.patch

Comment 31

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_5_bugzilla-master.patch

Comment 32

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_6_bugzilla-4.4.patch

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)

Comment 33

[Dylan Hardison [:dylan] (he/him)]

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?

Comment 34

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_7_bmo.patch

Comment 35

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_7_bugzilla-4.4.patch

Comment 36

[Jeff Fearn 🐞]

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.

Comment 37

[Jeff Fearn 🐞]

(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.

Comment 38

[Dylan Hardison [:dylan] (he/him)]

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... :(

Comment 39

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_7_bugzilla-4.4.patch

Without 'state'

Comment 40

[Jeff Fearn 🐞]

Comment on attachment 8950824 [details] [diff] [review]
1433400_7_bugzilla-4.4.patch

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

Works for me!

Comment 41

[Dylan Hardison [:dylan] (he/him)]

dkl will be doing a release tomorrow.

Comment 42

[April King [:April]]

Do we know when the upstream patches and CVE will be going live?

Comment 43

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1433400_7_bugzilla-5.0.patch

Comment 44

[Dylan Hardison [:dylan] (he/him)]

dkl has started the release process

Comment 45

[Dylan Hardison [:dylan] (he/him)]

when do we close and unrestrict this bug?

Comment 46

[David Lawrence [:dkl]]

Opening up as this is now public.

Comment 47

[David Lawrence [:dkl]]

This has been committed to all related repos.

Comment 48

[Dylan Hardison [:dylan] (he/him)]

Thanks for handling the release dkl!
Also thanks to both dkl and jfearn for reviewing the half-dozen revisions to this fix.

Comment 49

[Frédéric Buclin]

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?

Comment 50

[April King [:April]]

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!

Comment 51

[Ruben]

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.