1439260 - (CVE-2023-5206) Debug param can force html content type on non-html responses, resulting in XSS

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Holger Fuhrmannek]

Steps to reproduce:

1.) create a bug report with the title "399877536 <script>alert(123);</script>" (without quotes)
2.) visit /report.cgi?debug=1&short_desc=399877536&short_desc_type=allwordssubstr&y_axis_field=short_desc&ctype=csv&action=plot

An alert prompt will appear.

Comment 1

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

Holger: I cannot replicate this issue.  I attempted to replicate this with bug 1439604, which I have CC'd you on.  Could you please provide a valid URL which POC's my example bug?

Comment 2

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

I did try to replicate the bug with this URL, but it doesn't seem to work...

https://bugzilla.mozilla.org/report.cgi?debug=1&short_desc=183718273918273182738&short_desc_type=allwordssubstr&y_axis_field=short_desc&ctype=csv&action=plot

Comment 3

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

It will only happen for those in the debug group. I'll remove this feature though.

Comment 4

[Holger Fuhrmannek]

The fix for Bug 1433400 is the problem, we need a referer from the bugzilla host.

New steps to reproduce:

1.) create a bug report with the title "399877536 <script>alert(123);</script>" (without quotes)
2.) post a comment with a link to  <Bugzilla-Host>/report.cgi?debug=1&short_desc=399877536&short_desc_type=allwordssubstr&y_axis_field=short_desc&ctype=csv&action=plot
3.) click on the link

or simply click on the link in your comment ;)

Comment 5

[Holger Fuhrmannek]

(In reply to Dylan Hardison [:dylan] (he/him) from comment #3)
> It will only happen for those in the debug group. I'll remove this feature
> though.

no, i see the alert if i click on the link in comment #2.

Comment 6

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

Holger: I can confirm the issue now, I didn't realize referer was required.  The XSS is also in the bugzilla.mozilla.org context, I adjusted the subject to reflect document.domain.

Comment 7

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

Of course. So there is a debug param that turns on some debugging, and another gem:

# If we get a template or CGI error, it comes out as HTML, which isn't valid
# PNG data, and the browser just displays a "corrupt PNG" message. So, you can
# set debug=1 to always get an HTML content-type, and view the error.
$format->{'ctype'} = "text/html" if $cgi->param('debug');

Comment 8

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

introduced in bug 173005

Comment 9

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

Adding jfearn. At least this bug is easy to fix.

Holger, nice catch.

Comment 10

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

It looks like this was introduced in bug 413851 actually.

Comment 11

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

Attachment: bug-1439260_1-bugzilla-4.4.patch

Comment 12

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

Attachment: bug-1439260_1-bugzilla-5.0.patch

Comment 13

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

Attachment: bug-1439260_1-bugzilla-master.patch

Comment 14

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

Attachment: bug-1439260_1.patch

Comment 15

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

Also, just as an added idea/improvement here, if I'm understanding this correct, the Content Security Policy in BMO should have stopped this vector...

The current content security policy for BMO is:

Content-security-policy:
  default-src 'self';
  child-src 'self';
  img-src 'self' https://secure.gravatar.com https://www.google-analytics.com;
  object-src 'none';
  script-src 'self' https://www.google-analytics.com;
  style-src 'self' 'unsafe-inline';
  frame-ancestors 'none';
  form-action 'self' https://www.google.com/search https://github.com/login/oauth/authorize https://github.com/login

However, it seems the response headers on this particular request did not contain the CSP headers, so maybe there is some short-circuiting happening here we should investigate to make sure the CSP policy is being applied completely for all responses?

Comment 16

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

Attachment: Screen Shot 2018-02-20 at 1.10.42 PM.png

Comment 17

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

Note that BMO's CSP rollout is per-endpoint, and report.cgi does not yet have those headers. I'm also investigating how much work it would be for that page.

As a means of fixing this in BMO *before* it gets fixed upstream, I may decide to turn on CSP for several end points even if it will break some functionality.

Comment 19

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

Though the chart.cgi variation of this is *worse* because it outputs both sets of headers.

Why is that worse? Because even if CSP was turned on, it may be possible to capture the nonce.

Comment 20

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

request.cgi *seems* to actually not do anything insane with a debug param, so we have that going for us.

Comment 21

[David Lawrence [:dkl]]

Comment on attachment 8952419 [details] [diff] [review]
bug-1439260_1.patch

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

r=dkl

Comment 22

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

Note we're *really* fixing this for BMO in bug 1439797, via subterfuge. This bug needs to stay private until we do an upstream release.

Comment 23

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

The threat posed by this and bug 1439786 should be neutralized on BMO now.

Comment 24

[Jeff Fearn 🐞]

Comment on attachment 8952415 [details] [diff] [review]
bug-1439260_1-bugzilla-4.4.patch

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

works for me

Comment 25

[Jeff Fearn 🐞]

Comment on attachment 8952416 [details] [diff] [review]
bug-1439260_1-bugzilla-5.0.patch

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

works for me

Comment 26

[April King [:April]]

I assume this was merged? Can we close this bug out?

Comment 27

[David Lawrence [:dkl]]

(In reply to April King [:April] from comment #26)

I assume this was merged? Can we close this bug out?

This has not been merged upstream for whatever reasons so cannot be closed yet.

Comment 28

[April King [:April]]

Are you able to check in on it? It's been almost two years now. :)

Comment 29

[Frida K [:frida]]

the issue doesn't seem to be fixed, the payload is not executed because it's blocked by CSP

Comment 30

[Frida K [:frida]]

Attachment: Screenshot 2022-01-21 at 12.48.25.png

Comment 31

[Daniel Veditz [:dveditz]]

(In reply to David Lawrence [:dkl] from comment #27)

This has not been merged upstream for whatever reasons so cannot be closed yet.

Has this been merged upstream by now? Is there a bug or GH reference for that?

Comment 32

[Dave Miller [:justdave]]

Just verified this has NOT been committed upstream yet. This will be included in the upcoming releases.

Comment 33

[Dave Miller [:justdave]]

I was thinking the fix for this would already included in Harmony by virtue of it having merged BMO after this was committed to BMO. However, upon investigating (good thing I looked), it has not been applied to Harmony, and it ALSO hasn't been applied to BMO. Per Comment 22, BMO "fixed" this by setting up the CSP to block the symptoms rather than fixing the root of the problem.

Per comment 19, chart.cgi has the same problem, and is not included in this set of patches, nor are there any patches on the dependent bug to cover chart.cgi.

I'll have revised patches shortly.

Comment 34

[Dave Miller [:justdave]]

Attachment: patch for harmony

The existing patch for harmony no longer applies to either harmony or BMO. The fixed patch for harmony also does not apply against BMO (the chart half applies, the reports.cgi part doesn't - and it looks like it's because of context)

This patch is for Harmony only.

Comment 35

[Dave Miller [:justdave]]

Attachment: patch for harmony

I guess I should actually attach the patch here, nobody will be able to see that PR since it's on a private repo. I actually tried to do that first but Bugzilla wouldn't let me pick Dylan for a reviewer.

Comment 36

[Dave Miller [:justdave]]

Attachment: patch for BMO

Comment 37

[Dave Miller [:justdave]]

Attachment: patch for 4.4 branch

Patch for 4.4 applies intact, but of course was missing the chart.cgi part. This one has both.

Comment 38

[Dave Miller [:justdave]]

Attachment: patch for 5.0.4 branch

5.0 patch applies cleanly, but didn't have chart.cgi portion. This one includes both.

Comment 39

[Dave Miller [:justdave]]

Attachment: patch for 5.2 branch and master

Comment 40

[Dave Miller [:justdave]]

Comment on attachment 9350590 [details] [diff] [review]
patch for 5.2 branch and master

Turns out the 5.2 patch applies cleanly on master. :-)

Comment 41

[Dave Miller [:justdave]]

Vulnerability Description:

Leftover debugging code allowed XSS injection within the bug title when viewing charts and reports if a specific URL param was passed to enable the debugging code.

Vulnerable versions:

Bug was introduced in version 2.18
From 2.18 prior to 4.4.14 -> upgrade to 4.4.14
5.0.x versions prior to 5.0.4.1 -> upgrade to 5.0.4.1
5.0.5 and 5.0.6 -> upgrade to 5.2
Developer snapshots in 5.1.x prior to 5.1.3 -> upgrade to 5.1.3

dveditz, can we get a CVE for this?

Comment 42

[David Lawrence [:dkl]]

Comment on attachment 9350584 [details] [diff] [review]
patch for BMO

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

lgtm

Comment 43

[Frédéric Buclin]

Comment on attachment 9350585 [details] [diff] [review]
patch for 4.4 branch

>diff --git a/chart.cgi b/chart.cgi

>-    # Debugging PNGs is a pain; we need to be able to see the error messages
>-    if ($cgi->param('debug')) {
>-        print $cgi->header();
>-        $vars->{'chart'}->dump();
>-    }

Could you give us a PoC explaining how to abuse chart.cgi? Else why removing this code?



>diff --git a/report.cgi b/report.cgi

>-if ($cgi->param('debug')
>-    && Bugzilla->params->{debug_group}
>-    && Bugzilla->user->in_group(Bugzilla->params->{debug_group})
>-) {
>-    $vars->{'debug'} = 1;
>-}

If you want to remove this code entirely (instead of preventing report.csv.tmpl from being called), then you should also remove obsolete code from reports/report.html.tmpl:

[% IF debug %]
  [% FOREACH query = queries %]
    <p>[% query.sql FILTER html %]</p>
  [% END %]
[% END %]

In that case, you must also fix the description of the debug_group parameter which states that you can view debug data from reports. This statement is no longer true with the removal of the debug code above.

IMO, a better solution would be to fallback to reports.html.tmpl instead of reports.csv.tmpl when debug=1.

Comment 44

[Dave Miller [:justdave]]

(In reply to Frédéric Buclin from comment #43)

Could you give us a PoC explaining how to abuse chart.cgi?

There's a PoC in bug 1439786 comment 1.

IMO, a better solution would be to fallback to reports.html.tmpl instead of
reports.csv.tmpl when debug=1.

This makes sense... I'm not that familiar with the reporting code, so I took the fix Dylan had already provided (which seemed to have been vetted by BMO) and ran with it. While it does actually remove the vulnerability, you're right that it's removing a useful feature that could have the actual XSS fixed instead of just removing it wholesale.

Comment 45

[Daniel Veditz [:dveditz]]

Bouncing request for CVE to Tom. Our local pool of reserved IDs is empty and we'll have to request more.

Comment 46

[Dave Miller [:justdave]]

OK, new set of patches for this incoming.

New philosophy, taking comment 43 into account. Instead of removing this feature, we're going to implement it properly.

1. The dump() function in Bugzilla::Chart now returns the dumped data raw instead of html_sanitizing it and dumping it to standard out.
2. debug_group is now checked in both CGIs (it was only being checked in report.cgi before, and debug=1 worked for anyone in chart.cgi
3. Previous implementation had it dumping raw HTML before the template was output, which meant it was outside of the html entity. Proper browsers would probably ignore it, but instead they all just use quirks mode and it looks weird. We now feed the debug data to the template, and the template filters it on output (data sanitation philosophy: filter as close to the user as possible)

Comment 47

[Dave Miller [:justdave]]

Attachment: patch for 4.4 branch

Comment 48

[Dave Miller [:justdave]]

Attachment: patch for 4.4 branch

Comment 49

[Dave Miller [:justdave]]

Attachment: patch for 5.0.4 branch

Comment 50

[Dave Miller [:justdave]]

The final patches can be found in the git commits as follows:

4.4: https://github.com/bugzilla/bugzilla/commit/095e8f1f039d7f7425ed32515d5964a84dcbf315
5.0.4: https://github.com/bugzilla/bugzilla/commit/430a93b7c4df3aadfa2262ef9b8bcc8e77f0eca8
5.2: https://github.com/bugzilla/bugzilla/commit/2b5bf1ccea9ca61989b5f0819d929252bd41fc6f
5.3: https://github.com/bugzilla/bugzilla/commit/5af7bc0be41b244d916888139616e0898d35e37e
harmony: https://github.com/bugzilla/harmony/commit/67d5f3c5f60598f33c88a6599c5ae5923097a16e