1145314 - Lock down the URI_IS_UI_RESOURCE bits of CheckLoadURI some more

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1144991 comment 3.  This is about the "more daring fix": just drop that hunk that lets any URI_IS_UI_RESOURCE URL to link to any other URL with that flag subject if nsIScriptSecurityManager::ALLOW_CHROME being set.

Comment 1

[Jet Villegas (inactive)]

+cc: yury to see if Shumway is impacted

Comment 2

[Tom Ritter [:tjr] (OOTO until April)]

Christoph, this seems related to harden-security-landscape-firefox - I'm ni-ing you to make sure you see this can re-assign it to a new category if appropriate and decide for yourself what the possible priority for this is. (To be clear: I'm not advocating for this to be above backlog.)

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

I am doing work that is closely related to this bug within Bug 1188538. I am assigning this one to myself because it aligns perfectly with hardening efforts (Meta Bug 1596359).

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Let's see where we are here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de7999c5eed39d3f3d05a4b14459112954d9ab7f

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

Attachment: Bug 1145314: Package all devtools css files and load them using the internal chrome: protocol. r=nchevobbe

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

Hey Jason,

Nicolas pointed me your way hoping that you can help me out.

Some Background:
We are trying to lock down CheckLoadURIFlags by dropping the check that lets any URI_IS_UI_RESOURCE URL link to any other URL with that flag. While that works in general we realized that quite often CSS files within devtools are loaded using resource:// and then can't access chrome:// resources. To make that work I chatted with Nicolas, Julian and Alexandre and we concluded that it's fine that we package all devtools CSS files, add them to the jar.mn and then load them using chrome://.

So here is the question:
That approach seems to work well but I am encountering some errors on TRY. One illustrative example is exposed when running browser_dbg-blackbox-original.js. In that case WelcomeBox.js tries to import "./WelcomeBox.css";, but WelcomeBox.css is now not a DevToolsModule anymore and hence can't be loaded through import. We would need to load that somehow different. (FWIW, here is the entire patch.)

Do you have any suggestions how to load WelcomeBox.css or any different advise on how to move forward?

Thanks for your time!

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

So Nicolas pointed out that there was an additional : in

chrome:://devtools/content/debugger/dist/vendors.css

which, when removed, allows test browser_dbg-blackbox-original.js to work correctly again.

I guess in that case we are all set for review.

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

Attachment: Bug 1145314: Lock down CheckLoadURIFlags by dropping the check that lets any URI_IS_UI_RESOURCE URL link to any other URL with that flag. r=bholley

Comment 9

[Ryan VanderMeulen [:RyanVM]]

This was backed out for node debugger failures.
https://hg.mozilla.org/integration/autoland/rev/75aff93dfab5

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308287410&repo=autoland&lineNumber=421

TEST-UNEXPECTED-FAIL flow | Cannot resolve module raw!chrome://devtools/content/debugger/src/utils/editor/source-editor.css.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed:
https://hg.mozilla.org/integration/autoland/rev/f524ffe669caa31102581e715275c35b49422fe3
https://hg.mozilla.org/integration/autoland/rev/0500cb344e6fe548f1a89c542f7540ea63a3fd68

Backed out for assertions during caps/tests/mochitest/test_bug995943.xhtml:
https://hg.mozilla.org/integration/autoland/rev/71d4148d32093785c94217409cb00abbef8143de

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=308348213&repo=autoland

Comment 11

[Christoph Kerschbaumer [:ckerschb}]

I am on it - thanks!

Comment 12

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #10)

Backed out for assertions during caps/tests/mochitest/test_bug995943.xhtml:
https://hg.mozilla.org/integration/autoland/rev/71d4148d32093785c94217409cb00abbef8143de

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=308348213&repo=autoland

So it seems the expected assertion range for test_bug995943.xhtml on Linux is expectAssertions(0, 2) while on Mac it's already pumped to 11. There are a multitude of bugs listed as comments in the test (e.g. Bug 1067022, Bug 1307988, Bug 1305241) which all pumped the expected assertion range with no obvious reason why it was pumped - I guess mainly to eliminate the intermittent test failures?.

Looking at the provided backout log there is also no indicator that this bug particularly had an impact on that increase of the intermittent.

I'll upload a patch which simply pumps the expected assertion range like the other listed bugs - I assume that's acceptable.

Comment 13

[Christoph Kerschbaumer [:ckerschb}]

Attachment: Bug 1145314: Increase expected assertion range to 9 for test_bug995943.xhtml on Linux. r=jmaher

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/5492ac0e42f79107dbac41a838477c127ed364db
https://hg.mozilla.org/integration/autoland/rev/9640a9d093c7503e16efb31860624212d2da869f
https://hg.mozilla.org/integration/autoland/rev/664cc562ddf3706b70584ce3f0af40de59793262

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5492ac0e42f7
https://hg.mozilla.org/mozilla-central/rev/9640a9d093c7
https://hg.mozilla.org/mozilla-central/rev/664cc562ddf3

Comment 16

[Adam Roach [:abr]]

I don't have setup that allows me to easily check, but (in addition to the credit card autofill bug I cite above) this looks like it will break the https://bugzilla.mozilla.org/show_bug.cgi?id=accessiblecaret feature as well: the Android version of https://searchfox.org/mozilla-central/source/layout/style/res/ua.css (which is loaded in on resource://gre-resources/ua.css) references a number of resources that reside on chrome:// endpoints. I was able to find this with a single quick search; it seems extremely likely that other breakages are lurking in the tree.

I recommend that we back this fix out until a more comprehensive audit of the tree can be performed to ensure that we're not breaking a slew of features. AFAICT, there's not generally testing in the tree to ensure that image resources are loaded properly from CSS, which is the kind of breakage this patch is causing. Given the rather distinctive console errors caused by this breakage (of the form Security Error: Content at resource://formautofill/autocomplete-item-shared.css may not load or link to chrome://formautofill/content/third-party/cc-logo-visa.svg.), this audit should probably be something that can be automated on mochitests.

Comment 17

[Adam Roach [:abr]]

More likely breakage (and I'm still sure I haven't found everything):

resource://activity-stream/aboutwelcome/aboutwelcome.css contains chrome: resources.
resource://activity-stream/css/activity-stream.css contains chrome: resources.
resource://devtools/server/actors/highlighters.css contains chrome: resources.

These are extremely high-visibility locations to our users. Maybe we should revert the change and put telemetry on this code path to see how much stuff it's going to break in the field? I have to assume that this change is not urgent, given its age.

Comment 18

[Christoph Kerschbaumer [:ckerschb}]

Aryx, given comment 16 and comment 17, can you back that out for me please? I guess we need to do a more thorough audit and even add telemetry. Thanks!

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout: https://hg.mozilla.org/integration/autoland/rev/556ca11e8c3c78db68ffba0ab6f8c039d0790f2a

Comment 20

[Adam Roach [:abr]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)

Aryx, given comment 16 and comment 17, can you back that out for me please? I guess we need to do a more thorough audit and even add telemetry. Thanks!

Thanks for the quick response! Could I ask that you wait until we close Bug 1650198 before doing any evaluation on Try? Because it's been mothballed for a bit, a lot of the Credit Card Autofill tests don't run on automation yet.

Comment 21

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)

Aryx, given comment 16 and comment 17, can you back that out for me please? I guess we need to do a more thorough audit and even add telemetry. Thanks!

I think a try run (with that failure path surfaced) is probably a reasonable standard, and that telemetry probably isn't warranted here.

(In reply to Adam Roach [:abr] from comment #20)

Could I ask that you wait until we close Bug 1650198 before doing any evaluation on Try? Because it's been mothballed for a bit, a lot of the Credit Card Autofill tests don't run on automation yet.

Do you have a timeframe for that?

I think it's generally fine to get started with the audit and fixups for affected code. If we get all those cases fixed before the credit card autofill tests are landed, we can consider how to proceed.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout meged to central: https://hg.mozilla.org/mozilla-central/rev/556ca11e8c3c

Comment 23

[Christoph Kerschbaumer [:ckerschb}]

Bobby, I backed out the changes within this bug, since there is a case that I forgot about, which is moz-icon.
Some examples are:

• chrome://global/skin/global.css may not load or link to moz-icon://stock/gtk-dialog-warning?size=dialog
• resource://gre/ may not load or link to moz-icon://.lldbinit?size=16
• chrome://global/skin/dirListing/dirListing.css may not load or link to moz-icon://stock/gtk-go-up?size=menu

I don't quite now how to move forward:

• Should we explicitly allow 'moz-icon' schemes that are URI_IS_UI_RESOURCE?

Additionally I think it makes sense to add a pref, which we keep for like 2 releases and then remove, just in case we are (again) experiencing some unexpected problems.

Comment 24

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #23)

Bobby, I backed out the changes within this bug, since there is a case that I forgot about, which is moz-icon.
Some examples are:

• chrome://global/skin/global.css may not load or link to moz-icon://stock/gtk-dialog-warning?size=dialog
• resource://gre/ may not load or link to moz-icon://.lldbinit?size=16

Not sure what this is about? Do we actually have an icon called .lldbinit?

• chrome://global/skin/dirListing/dirListing.css may not load or link to moz-icon://stock/gtk-go-up?size=menu

I don't quite now how to move forward:

• Should we explicitly allow 'moz-icon' schemes that are URI_IS_UI_RESOURCE?

That seems reasonable, I think.

Additionally I think it makes sense to add a pref, which we keep for like 2 releases and then remove, just in case we are (again) experiencing some unexpected problems.

Sure.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/915aa6a1a0ed
https://hg.mozilla.org/mozilla-central/rev/eed643efe71a