Closed Bug 1319172 Opened 8 years ago Closed 8 years ago

Add a CSP policy to hg.mozilla.org

Categories

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Product:
Developer Services
Component:
Mercurial: hg.mozilla.org
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file)

ansible/hg-web: enable CSP on hg.mozilla.org (bug 1319172);
59 bytes, text/x-review-board-request
gps
: review+
Details
We should add a CSP policy to hg.mozilla.org. atoll suggested "Content-Security-Policy: frame-ancestors: *;" as a place to start.
cc :april for CSP review, because CSP is a nightmare. frame-ancestors: * is only for framing, and doesn't set any of the "normally desired" CSP headers like style-src and script-src. We have a separate bug open somewhere about reviewboard and CSP, I believe? :fubar has a link from me with a guide to setting up CSP without any previous knowledge of CSP, if you want to self-serve here, or various of us can consult (April's best at this).
To clarify my comment, because I realized I left something unspoken. I would set frame-ancestors - using CSP - *solely* to address the framing policy of HG. Designing and implementing a fuller CSP policy, with default-src and script-src and style-src, should *also* be done - but I would not generally advise blocking frame-ancestors on that work if it'll take days/weeks/months to review the full -src set.
I suspect we'll be limited what CSP options we can deploy. e.g. Mercurial upstream serves inline JavaScript, which I hear is frowned upon these days. I'm all for starting with something minimal and performing incremental ratcheting.
It's more valuable to have a CSP policy that best describes the current site than to not have a CSP policy while trying to alter, patch, and write a secure CSP policy at the same time, so ++ to that approach. This may serve as a useful starting point: Content-Security-Policy: frame-ancestors: *; default-src: https:; style-src: https: 'unsafe-inline'; script-src: https: 'unsafe-inline'; object-src: 'none'; With the most likely change being that https: should be replaced with 'self' so that the site still works in browsers at http://hg.m.o while refusing to load offsite js/css resources.
Are people wanting me to work up a policy for hg? I'd be happy to do it today if so.
(In reply to April King [:April] from comment #5) > Are people wanting me to work up a policy for hg? I'd be happy to do it > today if so. I'd welcome help on this! Please read bug 983704 before implementing the policy.
QA Contact: hwine → klibby
Sorry for taking so long on this; a license for an item in my toolchain expired, and it is taking a while to get it replaced. And my alternative (ZAP) seems to have problems with hg.mozilla.org and hg.mozilla.org only.
Okay, sorry, still don't have my toolchain working, but this seems to be working for me: Content-Security-Policy: default-src 'none'; img-src 'self'; script-src 'self' 'sha256-1SNj4RIZasUJJBg00R3mnRpYu2vOdTtOYLDixe1tGwc=' 'sha256-HlT1MTmEEy7zJrD85kelEdAaYkKnFyDmQUiWeQbR13g=' 'sha256-i6oIoomtatpn79tKjYuKlkFrIWXj6ZiRfWxi9dOEPx4='; style-src 'self' 'unsafe-inline' Note that we can kill those three 'sha256' entries by removing the inline javascript from the pages that they reside. But this allows those script snippets to still execute as is.
Forgive my CSP ignorance, but what is hashed to obtain those hashes? I ask because the emitted HTML goes through a templating system. And those templates are largely synchronized from upstream Mercurial when we do upgrades. So we'll need a way to easily update those hashes when the underlying content changes. It might just be easier to remove the inline JS upstream. But a code freeze for the upcoming 4.1 release is this weekend. So it's a tight timetable.
It's just the sha-2 hash of the contents of what is inside the script tags. If that inline script changes frequently, then that code won't load on upgrading. There are a couple options: a) remove the inline code, which is best practice anyways for maintainability, with the understanding that this may delay CSP until 4.2 b) if you have a way to inject code into each tag, you can add a nonce (a unique code, different with every HTTP request) as an attribute to each script tag: <script nonce="aa1fa3a4-53b5-426f-8df5-38ad6887dffa">...</script> And then you'd add 'nonce-aa1fa3a4-53b5-426f-8df5-38ad6887dffa' to script-src. But that requires code changes.
So the server would generate a nonce and add that to the HTTP response headers and the inline JavaScript. I think that is easier than moving all the JS to standalone files (although I concede I'd prefer to move the JS). But in terms of code churn, it should be smaller. A downside is the Mercurial server (as opposed to the load balancer) would need to produce the CSP header since it is generating the nonce. That shouldn't be a problem for Mercurial. I'm not sure if WebOps or others care though. (We currently handle HSTS on the origin servers.)
Yes, that's correct. The trick of it is that the nonce has to be unique and never reused, which can be tricky for some (or a lot) of different setups.
I've never known generating a type 4 UUID to be difficult ;) (Or am I missing something completely obvious?)
Some people just use integers, so a UUID is especially nice.
It's not the generation that's hard, it's the inserting them into the page on the fly. A lot of sites are generated once from templates and are static thereafter, making the dynamic inserting of a nonce into a page kind of painful. Some web servers (like the nginx extensions) make it possible, but even there it's a bit painful. Anyways, it doesn't need to be a UUID, I just like them because most languages have a nice library for it. Other options are taking a piece of /dev/random and sha256'ing it or whatever you like. :)
Looking into this a bit more, changing upstream Mercurial to not use inline <script> is a bit of work. There are currently a few features (namely the graph and shortlog pages) that generate dynamic code inside of <script>. The dynamic code is data (JSON describing changesets for the graph and basic parameters for the shortlog view). This would need to be refactored to put the data elsewhere. If outside of the DOM, that would change Mercurial's page load model, which currently only requires a single dynamic HTTP for each page. That would likely be a bit controversial. Because the inline <script> is dynamic, calculating a hash is non-trivial and would require buffering output so the hash can be sent in the HTTP header. That's a non-starter. That leaves a <script> nonce. We can have Mercurial generate a nonce add it the <script> elements. Adding the nonce to the CSP HTTP header is a bit more complicated. We could teach Mercurial to do that. Or we could teach Mercurial to set the nonce in an environment variable or HTTP header then have WSGI middleware or the WSGI proxying HTTP server extract the nonce and convert it to a CSP header. That being said, the nonce is only useful if the content behind it can be trusted. In the case of graph pages, the inline <script> contains JSON derived from repository content. So if repository content could be constructed in such a way that it fools the JSON escaping mechanism in Mercurial's templating layer and is able to write a </script> and therefore to the DOM, we're in trouble. I don't think that's a super high risk. But it's worth calling out. It's also worth calling out that the produced HTML has tons of inline data derived from repository content. There's a large surface area to attack and we give anyone with level 1 access to create a user repo to probe it. So even if we deploy CSP and disallow e.g. third party host access, custom code committed to source control could be made to execute. I suppose defense in depth applies and we should continue with as closed a CSP policy as possible. I just wanted to call out that CSP <script> policies may not be as effective as desired.
Oh, Mercurial sends an ETag header by default. We'll have to turn that off if we deploy inline nonces.
I submitted support for CSP to upstream Mercurial: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-January/092201.html April: I'd appreciate if you did a spot check to make sure you don't see any obvious problems.
In my experience the Mercurial people are really good about this sort of stuff. I have a real hard time reading that style of diff, but overall it looks fine. One thing I might change is instead of using base64 to do all the encoding, just use uuid.uuid4().hex; this produces a urlsafe base16 string and is probably overall faster and makes for cleaner code.
The CSP patch has been accepted upstream and should be part of Mercurial 4.1. Our deploy policy is to wait 1 month after new versions and roll out the point release. So that means we can roll out 4.1 as early as March 1. However, that likely won't happen this time since I'll be away on March 1. We're likely looking at mid March for rolling out 4.1. I propose rolling out a stop-gap CSP policy (defined in the httpd config) that allows "unsafe-inline" scripts and then switches to Mercurial + nonce scripts after we roll out 4.1. That policy would be: Content-Security-Policy: default-src 'none'; img-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'
Works for me! :)
Comment on attachment 8828189 [details] ansible/hg-web: enable CSP on hg.mozilla.org (bug 1319172); https://reviewboard.mozilla.org/r/105662/#review106732 Seems like a great start and I look forward to locking it down with Mercurial 4.1. :)
Attachment #8828189 - Flags: review?(april) → review+
Firefox didn't like this policy :/ Content Security Policy: The page’s settings blocked the loading of a resource at https://hg.mozilla.org/mozilla-central/shortlog/5d4a0370b0a7b30b73342cf53f4cf2374d34cfb4 (“default-src 'none'”). I /think/ it is referring to JS since the infinite scrollbar on that URL stopped working.
Chrome will have a more useful error message detailing the resource in question. I'm not sure how to persuade it out of Firefox, though.
atoll: thanks for the tip about using Chrome to debug this. I installed Mercurial 4.1-rc and launched a local server configured like hg.mo via: $ hg --config extensions.hgmo=/path/to/version-control-tools/hgext/hgmo serve --hgmo -config web.csp="default-src 'none'; img-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'" Then I opened http://localhost:8000/ in Chrome, opened the developer console, navigated to the /shortlog URL, scrolled down to trigger the JS loading, and got the following helpful error messages: Refused to connect to 'http://localhost:8000/shortlog/67ee7874e53b71f4c29afed3c8f1d8fa33a3c723' because it violates the following Content Security Policy directive: "default-src 'none'". Note that 'connect-src' was not explicitly set, so 'default-src' is used as a fallback. makeRequest @ mercurial.js:317 scrollHandler @ mercurial.js:374 mercurial.js:317 Uncaught DOMException: Failed to execute 'open' on 'XMLHttpRequest': Refused to connect to 'http://localhost:8000/shortlog/67ee7874e53b71f4c29afed3c8f1d8fa33a3c723' because it violates the document's Content Security Policy. at makeRequest (http://localhost:8000/static/mercurial.js:317:9) at scrollHandler (http://localhost:8000/static/mercurial.js:374:13) I'll poke around with connect-src and hopefully have a working policy up for review shortly.
Comment on attachment 8828189 [details] ansible/hg-web: enable CSP on hg.mozilla.org (bug 1319172); Added "connect-src: 'self'" to allow XHR.
Attachment #8828189 - Flags: review+ → review?(april)
Assignee: nobody → gps
Status: NEW → ASSIGNED
Made it through my stuff, cleaned up a bit. I'm left with https://bugzilla.mozilla.org/ for XHR requests.
The reftest analyser (linked from Treeherder amongst others) makes XHR requests to wherever the log is hosted. For buildbot jobs this is https://archive.mozilla.org and for taskcluster https://queue.taskcluster.net (though that 303 redirects to https://public-artifacts.taskcluster.net - does the CSP header needs to account for both domains or just one?). eg: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32/1484914584/mozilla-inbound_win7_vm_gfx_test-reftest-gpu-e10s-bm139-tests1-windows-build26.txt.gz&only_show_unexpected=1 https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/DGmwTiuJRDi0fuINEgTLKg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1 There may be additional locations (eg from third party job submission), but they are rare and probably fine to ignore since unlikely to be reftests. I agree that the reftest analyser should have it's own home not on hg.mozilla.org (bug 1200501; though whether that should be Treeherder or not is another matter), but in lieu of a responsive owner, this is the situation we have right now.
Sorry about that, I must have missed the pages that make XHR requests. It can be tricky to find all the nooks and crannies of a website when you're just a random outsider like myself. :)
Not a problem - I would expect people to have realised about it :-)
Comment on attachment 8828189 [details] ansible/hg-web: enable CSP on hg.mozilla.org (bug 1319172); https://reviewboard.mozilla.org/r/105662/#review107548
Attachment #8828189 - Flags: review?(april) → review+
Comment on attachment 8828189 [details] ansible/hg-web: enable CSP on hg.mozilla.org (bug 1319172); Added bugzilla.mo, archive.mo, and queue.taskcluster.net to connect-src.
Attachment #8828189 - Flags: review+ → review?(april)
Pushed by gszorc@mozilla.com: https://hg.mozilla.org/hgcustom/version-control-tools/rev/5676a267dad8 ansible/hg-web: enable CSP on hg.mozilla.org ; r=April
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 1333615
Oh and there's also: Content Security Policy: The page's settings blocked the loading of a resource at data:text/css;charset=utf8,body%3Anot(%5... ("style-src https://hg.mozilla.org 'unsafe-inline'"). Guessing that's a little more problematic.
Depends on: 1333929
Neerja filed bug 1333929 on comment 38, if it's better to track that in a followup bug.
Yeah I only reopened since it seemed like the issue was an omission between what was requested (comment 31) and what was added. However given there is another entry required now too (comment 39), a new bug makes more sense.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Flags: needinfo?(gps)
Resolution: --- → FIXED
:emorley - could you reference this bug in the see-also from the data:text/css bug? That's .. kind of an unexpected source for a stylesheet, and I'd enjoy following along.
Attachment #8828189 - Flags: review?(april) → review+
The new bug is bug 1333929.
Blocks: 1335626
Blocks: 1336663
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: