Bug 1433400 (CVE-2018-5123)

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

RESOLVED FIXED in Bugzilla 4.4

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: hofusec, Assigned: dylan)

Tracking

({sec-critical, wsec-csrf, wsec-disclosure})

unspecified
Bugzilla 4.4
sec-critical, wsec-csrf, wsec-disclosure
Dependency tree / graph
Bug Flags:
sec-bounty +

Details

(Whiteboard: [reporter-external] [web-bounty-form] [verif?])

Attachments

(4 attachments, 15 obsolete attachments)

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
(Reporter)

Description

a year ago
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?
(Reporter)

Comment 1

a year ago
Created attachment 8945730 [details]
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.
Created attachment 8945771 [details]
Screen Shot 2018-01-26 at 9.25.58 AM.png
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)

Updated

a year ago
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.
(Assignee)

Comment 7

a year ago
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
(Assignee)

Updated

a year ago
Blocks: 1433493
Keywords: sec-high → sec-critical
(Assignee)

Comment 8

a year ago
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
Keywords: wsec-xss → wsec-csrf, wsec-disclosure
Created attachment 8945927 [details] [diff] [review]
bmo patch 1

First patch for bmo. Also testing patches for 4.2, 4.4, 5.0 and master.
Attachment #8945927 - Flags: review?(dkl)
Created attachment 8945932 [details] [diff] [review]
1433400_1_bmo.patch

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)
Created attachment 8946168 [details] [diff] [review]
1433400_2_bugzilla_4.4.patch
Attachment #8946168 - Flags: review?(jfearn)
Created attachment 8946169 [details] [diff] [review]
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.
Attachment #8946169 - Flags: review?(dkl)
(Assignee)

Updated

a year ago
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+
Created attachment 8946432 [details] [diff] [review]
1433400_3_bmo.patch

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.
Created attachment 8946436 [details] [diff] [review]
1433400_3.1_bmo.patch
Attachment #8946432 - Attachment is obsolete: true
Attachment #8946432 - Flags: review?(dkl)
Created attachment 8946439 [details] [diff] [review]
1433400_3_bugzilla_4.4.patch
Attachment #8946168 - Attachment is obsolete: true
Attachment #8946168 - Flags: review?(jfearn)
Attachment #8946439 - Flags: review?(jfearn)
(Assignee)

Updated

a year ago
Attachment #8946436 - Flags: review?(dkl)
Created attachment 8946443 [details] [diff] [review]
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 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 23

a year ago
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 25

a year ago
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-
Created attachment 8948556 [details] [diff] [review]
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.
Attachment #8946436 - Attachment is obsolete: true
Attachment #8948556 - Flags: review+
Created attachment 8948658 [details] [diff] [review]
1433400_5_bmo.patch
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+
(Assignee)

Updated

a year ago
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
Created attachment 8949138 [details] [diff] [review]
1433400_5_bugzilla-4.4.patch
Attachment #8949138 - Flags: review?(jfearn)
Created attachment 8949142 [details] [diff] [review]
1433400_5_bugzilla-5.0.patch
Attachment #8949142 - Flags: review?(jfearn)
Created attachment 8949143 [details] [diff] [review]
1433400_5_bugzilla-master.patch
Attachment #8949143 - Flags: review?(jfearn)
Created attachment 8950460 [details] [diff] [review]
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)
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)
Created attachment 8950461 [details] [diff] [review]
1433400_7_bmo.patch
Attachment #8948658 - Attachment is obsolete: true
Attachment #8950460 - Attachment is obsolete: true
Attachment #8950460 - Flags: review?(jfearn)
Attachment #8950461 - Flags: review?(dkl)
Created attachment 8950462 [details] [diff] [review]
1433400_7_bugzilla-4.4.patch
Attachment #8950462 - Flags: review?(jfearn)

Comment 36

a year ago
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-

Comment 37

a year ago
(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... :(
Created attachment 8950824 [details] [diff] [review]
1433400_7_bugzilla-4.4.patch

Without 'state'
Attachment #8950462 - Attachment is obsolete: true
Attachment #8950824 - Flags: review?(jfearn)

Comment 40

a year ago
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?
Created attachment 8951341 [details] [diff] [review]
1433400_7_bugzilla-5.0.patch
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
Last Resolved: a year 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!
You need to log in before you can comment on or make changes to this bug.