1280113 - [observatory] Improve the security of DXR's web configuration (D, dxr.mozilla.org)

Status: RESOLVED WONTFIX

(Webtools Graveyard :: DXR, defect)

Description

[April King [:April]]

From a cursory glance, it looks like DXR should at least implement CSP and probably X-Frame-Options.  HSTS is already implemented, hooray!

I'll work on coming up with what seems to be a working CSP implementation.

Comment 1

[April King [:April]]

Okay, here are my recommend changes for DXR, based on my testing.

CSP seems to work great, since DXR is so self-contained:

Content-Security-Policy: default-src 'none'; connect-src 'self'; font-src 'self'; frame-ancestors 'none'; img-src 'self'; script-src 'self'; style-src 'self'
X-Frame-Options: DENY

Both frame-ancestors and XFO depend on whether or not DXR should be frameable.  If that's functionality that we should have, just use this:

Content-Security-Policy: default-src 'none'; connect-src 'self'; font-src 'self'; img-src 'self'; script-src 'self'; style-src 'self'

Erik, would you be able to do some testing with CSP/XFO and verify that everything works correctly?  I get an error in Firefox about an attribute being blocked, but I can't seem to find it.  Chrome doesn't produce any errors.  Either way, seems to function perfectly in both of them for me in my testing, but there might be some nooks and crannies I don't know about.

Comment 2

[Kendall Libby [:fubar] (he/him)]

(In reply to April King [:April] from comment #1)
> Okay, here are my recommend changes for DXR, based on my testing.
> 
> CSP seems to work great, since DXR is so self-contained:
> 
> Content-Security-Policy: default-src 'none'; connect-src 'self'; font-src
> 'self'; frame-ancestors 'none'; img-src 'self'; script-src 'self'; style-src
> 'self'
> X-Frame-Options: DENY

I've temporarily added this to httpd://dxr.allizom.org/ if that helps to test/debug/verify.

Comment 3

[April King [:April]]

Okay, it looks like it does cause some weird CSP errors, and I'm not sure why.  I *think* this might be what is causing the error:

> window.onhashchange = function() {
>     if (window.location.hash) {
>         scrollIntoView(window.location.hash.substr(1));
>     }
> };

Because it's not using an event handler, but I could honestly be completely wrong.  I can trigger it whenever I use the Operator pulldown and select an item.

:erik, any idea?

Comment 4

[April King [:April]]

Erik, bump for visibility!

Comment 5

[Erik Rose [:erik][:erikrose]]

You're on my list! There's just things ahead of it (like m-c and c-c both caught fire and quit building yesterday).

Comment 6

[Kendall Libby [:fubar] (he/him)]

https://observatory.mozilla.org/analyze.html?host=dxr.mozilla.org

Test 	Pass 	Score 	Explanation 	
Content Security Policy 		-25 	Content Security Policy (CSP) header not implemented 	
Contribute.json 		-5 	Contribute.json file missing from root of website
X-Content-Type-Options 		-5 	X-Content-Type-Options header not implemented 	
X-Frame-Options 		-20 	X-Frame-Options (XFO) header not implemented 	
X-XSS-Protection 		-10 	X-XSS-Protection header not implemented

Score: 35/100
Grade: D

Comment 7

[Kendall Libby [:fubar] (he/him)]

Added a first pass at a contribute.json to bring us up to a D+. Erik, I'm happy to take any changes to it, if you'd like.

Comment 8

[April King [:April]]

And likewise that I am always happy to help.  :)

Comment 9

[Kendall Libby [:fubar] (he/him)]

Resurrecting.

https://dxr.allizom.org/ currently has the following enabled:

  Header set X-Frame-Options "DENY"
  Header set Content-Security-Policy "default-src 'none'; connect-src 'self'; font-src 'self'; img-src 'self'; script-src 'self'; style-src 'self'"
  Header set X-XSS-Protection "1; mode=block"
  Header set X-Content-Type-Options "nosniff"

and it seems to be fairly happy and brings us it to a A+. #c3 appears to be fixed, but browser console is logging one issue:

Content Security Policy: The page’s settings observed the loading of a resource at self (“style-src http: https: ftp:”). A CSP report is being sent. Source: background-color:#F4FAAA.

(five times, actually, for different background colors). I wasn't able to generate any other errors/issues, but I'm not exactly a power user here.

Thoughts?

Comment 10

[Erik Rose [:erik][:erikrose]]

It looks like it's getting grumpy about the inline styling, where we assign a background-color. I'm not sure why off the top of my head.

Comment 11

[Erik Rose [:erik][:erikrose]]

Okay, https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src says you have to include 'unsafe-inline' for inline styles to be okay.

Comment 12

[Kendall Libby [:fubar] (he/him)]

Huh, I swear I had tried that and gotten the same result, but clearly not. Added 'unsafe-inline' to style-src and it's happy about that. OTOH, I get a new error:

downloadable font: download failed (font-family: "icons" style:normal weight:normal stretch:normal src index:1): status=2147746065 source: https://dxr.allizom.org/static/fonts/icons.woff?2013  filter.0b15c5d8.css:1:12
GET  https://dxr.allizom.org/static/fonts/icons.ttf

which is getting a 404.

Comment 13

[Erik Rose [:erik][:erikrose]]

You found a bug! It happens on production, too, so you didn't make anything worse. I just filed bug 1356328.

Comment 14

[:Atoll]

Success, in theory! Anything left that you need to do here, fubar?

Comment 15

[April King [:April]]

If it's the same color that is getting called out again and again, it's much better to add a css class:

.fubar {
  background-color: #F4FAAA
}

And then use:

<tag class="fubar"></tag>

Instead of using inline scripts.  Having unsafe-inline in style-src isn't the end of the world, but moving the styles out (if not too much work) not only makes things safer but it makes your code cleaner as well.  :)

Comment 16

[:Atoll]

++, though I generally encourage separating the "making CSS/JS changes in support of CSP policy improvement" work from the "deploy CSP for the first time" work.

Comment 17

[April King [:April]]

Absolutely, no reason why we can't do 'unsafe-inline' in style-src now, fix things up, and then revisit our decisions later.  All depends on the work level involved.  :)

Comment 18

[:Atoll]

Okay, I'm going to reset this bug a bit.

Score: 40 [D+]
Modifiers:
    [  -5] X-Content-Type-Options header not implemented
    [ -10] X-XSS-Protection header not implemented
    [ -20] X-Frame-Options (XFO) header not implemented
    [ -25] Content Security Policy (CSP) header not implemented

First, CSP. Using April's super cool Observatory extension, I get the following CSP:

default-src 'none'; connect-src 'self'; img-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline';

It's not loading a font as far as I can tell, but I'll add font-src 'self' per comment 9 until the confusion around bug 1356328 is involved.

default-src 'none'; connect-src 'self'; img-src 'self'; font-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline';

This has unsafe-inline, which is fine, resolving the issue with comment 10.

:fubar, in comment 9, you set various DXR stage headers. Could you update the CSP header in that stage environment to:

  Header set X-Frame-Options "DENY"
  Header set Content-Security-Policy "default-src 'none'; connect-src 'self'; img-src 'self'; font-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline';"
  Header set X-XSS-Protection "1; mode=block"
  Header set X-Content-Type-Options "nosniff"

And then we can push forward with deploying this.

Comment 19

[Erik Rose [:erik][:erikrose]]

FWIW, the CSP headers should really be set by DXR itself, so they get set everywhere it's deployed and can change as the app itself does. I'm happy to take a patch to that effect, though I'm allocating very little time to DXR these days.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

DXR is no longer available. Searchfox is now replacing it.
See meta bug 1669906 & https://groups.google.com/g/mozilla.dev.platform/c/jDRjrq3l-CY for more details.