Bug 1353975 (CVE-2017-5466)

UXSS: Origin confusion when reloading isolated data:text/html URL

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: websec02.g02, Assigned: ckerschb)

Tracking

({csectype-sop, regression, sec-critical})

52 Branch
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?
qe-verify ?

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 wontfix, firefox-esr5253+ fixed, firefox53blocking fixed, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main53+][adv-esr52.1+])

Attachments

(4 attachments, 9 obsolete attachments)

6.56 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
4.82 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.05 KB, patch
ckerschb
: review+
ckerschb
: sec-approval+
Details | Diff | Splinter Review
2.06 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170323105023

Steps to reproduce:

- Assume http://target/ has a link to http://evil/ and a victim user clicks the link.
  <a href=http://evil>link</a>
- http://evil/ redirects to data:text/html URL with Location header.
  Location: data:text/html,<script>d=document.domain;if(d){alert(d)}else{location.reload()}</script>
- The page's JavaScript executes location.reload() because its origin is blank.



Actual results:

The reloaded data:text/html page runs in the context of http://target.
So, in the example above, a popup saying "target" shows up.

The test redirector is at http://ttera.lolipop.jp/.
The attack was confirmed to work on the latest version (v52.0.2 on Windows7 / MacOS 10).


Expected results:

The reloaded page should run in a blank isolated origin.
(Reporter)

Comment 1

2 years ago
The link above (to http://ttera.lolipop.jp/) doesn't seem to work from this bugzilla page.

Use https://lolipop-ttera.ssl-lolipop.jp/target.php as a target page.
The JS from http://ttera.lolipop.jp/ runs in the context of https://lolipop-ttera.ssl-lolipop.jp.

Comment 2

2 years ago
Christoph, seems like the principalToInherit is wrong in this case (presumably as a result of the reload). Can you investigate?

We should try to get this fixed for 53 still if at all feasible, IMO. Feels like sec-high or sec-crit to me.
Group: firefox-core-security → core-security
Component: Untriaged → Document Navigation
Flags: needinfo?(ckerschb)
Product: Firefox → Core

Comment 3

2 years ago
Setting flags based on bug 1317641.
(In reply to :Gijs from comment #3)
> Setting flags based on bug 1317641.

I haven't debugged the problem yet, but I think the problem is that we have several security flags within the loadInfo which can appear together. In other words we should not do a == comparison but rather check if one of those flags is set by using &.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/9935254c39eef9d14de419dd6163ff453cc1ce16#l1.18
Assignee: nobody → ckerschb
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(ckerschb)
That ESR45 change might have been premature. Is it also affected, Christoph?
Flags: needinfo?(ckerschb)
Boris, when clicking the link in the page we load the page using the following args:

doContentSecurityCheck {
  channelURI: http://ttera.lolipop.jp/
  loadingPrincipal: nullptr
  triggeringPrincipal: https://lolipop-ttera.ssl-lolipop.jp/target.php
  principalToInherit: https://lolipop-ttera.ssl-lolipop.jp/target.php
  contentPolicyType: 6
  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, 
  initalSecurityChecksDone: no
  enforceSecurity: no
}

when encountering the redirect we load the data: URI using the following args:

doContentSecurityCheck {
  channelURI: data:text/html,<script>d=document.domain;if(d){alert(d)}else{location.reload()}</script>
  loadingPrincipal: nullptr
  triggeringPrincipal: https://lolipop-ttera.ssl-lolipop.jp/target.php
  principalToInherit: https://lolipop-ttera.ssl-lolipop.jp/target.php
  contentPolicyType: 6
  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, SEC_FORCE_INHERIT_PRINCIPAL, 
  initalSecurityChecksDone: no
  enforceSecurity: no
}

Initially I thought the problem lies within GetChannelResultPrincipal, but the fact that we query the securityMode() from the LoadInfo and not the securityFlags proofed me wrong. So, it seems something wrong with our principalToInherit. Any idea where to start chasing the problem?
Flags: needinfo?(bzbarsky)
> So, it seems something wrong with our principalToInherit.

Where is the SEC_FORCE_INHERIT_PRINCIPAL coming from on the redirect?  That part is clearly bogus, no?  The whole point of setting SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL there is that data: should get the null principal.
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

2 years ago
Flags: needinfo?(ckerschb)
I converted the STRs from comment 0 into a first test. Only difference, I load everything within an iframe which works correctly and provides the correct principalToInherit. So it seems the problem only occurs when redirecting top level loads to a data: URI.

I am about to write a test for converting the STRs into a navigating toplevel URI test next.

My guess is that the SEC_FORCE_INHERIT_PRINCIPAL comes from the docshell load here [1], but why that is not happening for subdocument loads I can't tell as of now. Needs more digging!

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11005
Attachment #8855359 - Attachment is obsolete: true
Boris, I created testcases for iframe as well as a top-level loads (see the other two attachmnets) that illustrates the problem. In both cases we 'reload' the data: URI using FORCE_INHERIT_PRINCIPAL. Unfortunately we can not land any of those two test cases because after applying this fix, document.domain is always empty, which in turn causes the test to hang in an endless loop, always performing a reload. But I guess that's expected behavior now.

In general, similar to what we do after a redirect within the base channel, we also should not set the SEC_FORCE_INHERIT_PRINCIPAL after a reload. Does that sound ok to you?

Also, if you have an idea how we could tweak the test so we can land them, I would highly appreciated to hear those suggestions.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsBaseChannel.cpp#89
Attachment #8855415 - Flags: review?(bzbarsky)
Hmm.  So if we _don_'t set SEC_FORCE_INHERIT_PRINCIPAL on this codepath, but we would have inherited for a "normal" load in such circumstances, will we still do so on reload?
Flags: needinfo?(ckerschb)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #12)
> Hmm.  So if we _don_'t set SEC_FORCE_INHERIT_PRINCIPAL on this codepath, but
> we would have inherited for a "normal" load in such circumstances, will we
> still do so on reload?

Hm, good point - I guess not. Are you saying we should check for data: URI in particular?
Flags: needinfo?(ckerschb)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> That ESR45 change might have been premature. Is it also affected, Christoph?

We introduced the principle of a principalToInherit within Bug 1297338 which landed in FF52. So (without doing further reserach) I would imagine that's when the problem started. ESR45 should be unaffected.
No, I'm just saying a reload should replay the load that actually happened, however we do that.

The best fix would be to stop using SEC_FORCE_INHERIT_PRINCIPAL in docshell altogether.  But then it's not clear how to make the about:blank or srcdoc cases work, right?

The next best fix is to stop using it just for data: somehow without hardcoding the "data:" scheme, because it's not that "data:" is terribly special here; any scheme that ChannelShouldInheritPrincipal() returns true for will have this problem, right?

The simplest fix may be for necko redirects to set principalToInherit to a nullprincipal.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #15)
> The simplest fix may be for necko redirects to set principalToInherit to a
> nullprincipal.

Thanks Boris - that sounds like the best way forward, given that we also have to uplift that patch.

Regarding automated tests: It would be nice if we could land the testcases as well. Can you think of a different condition to use (other than branching on document.domain) to trigger the reload case? Currently we end up in an endless loop because of that - thanks!
Attachment #8855415 - Attachment is obsolete: true
Attachment #8855415 - Flags: review?(bzbarsky)
Attachment #8855455 - Flags: review?(mcmanus)
Attachment #8855455 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #15)

> The simplest fix may be for necko redirects to set principalToInherit to a
> nullprincipal.

I can get behind that. Thanks bz!
Attachment #8855455 - Flags: review?(mcmanus) → review+
Gijs, I think the best option we have is using performance.navigation.type which is 0 for the initial load and 1 for every reload. Can you take a look at the test please?
Attachment #8855411 - Attachment is obsolete: true
Attachment #8855747 - Flags: review?(gijskruitbosch+bugs)
Gijs, similar test, but for a top level redirect. I am slightly worried about the loadPromise because of the reload. It works reliable locally but wanted to hear your opinion if it's safe to use. Thanks!
Attachment #8855412 - Attachment is obsolete: true
Attachment #8855748 - Flags: review?(gijskruitbosch+bugs)

Comment 20

2 years ago
Comment on attachment 8855747 [details] [diff] [review]
bug_1353975_data_redirect_iframe_test.patch

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

r=me with the below addressed.

::: dom/security/test/general/file_data_initial_page.html
@@ +12,5 @@
> +// check the initial principals within the loadinfo
> +window.parent.postMessage({result: 'initial-page-loaded'}, '*')
> +
> +var link = document.getElementById("testlink)");
> +testlink.click();

Does the DOM spec / API implementation guarantee that the message posted from postMessage will be processed synchronously, before the testlink.click() resolves? Because if not (which is what I suspect), I think the tests will fail intermittently. You might need to wait for a postMessage back from the parent frame that indicates you're good to load the sjs which location-redirects to the data: URI.

::: dom/security/test/general/test_data_uri_redirect.html
@@ +51,5 @@
> +    is(initialLoadInfo.loadingPrincipal.URI.asciiSpec, TEST_URI,
> +      "initial loadingPrincipal correct");
> +    is(initialLoadInfo.triggeringPrincipal.URI.asciiSpec, TEST_URI,
> +      "initial triggeringPrincipal correct");
> +    is(initialLoadInfo.principalToInherit.URI.asciiSpec, TEST_URI,

This and all the other checks against URI.asciiSpec (why asciiSpec and not just .spec?) would throw if any of these were not codebase principals, which AIUI would break the test and make it time out (because we never call checkFinish() and we'd never call SimpleTest.finish()).

Might be worth just adding a trivial helper function inside receiveMessage that converts a principal into a uri spec and returns null or empty string if principal.URI is null.
Attachment #8855747 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 21

2 years ago
Comment on attachment 8855748 [details] [diff] [review]
bug_1353975_data_redirect_toplevel_test.patch

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

Eh, I don't like throwing up roadblocks but I guess it makes sense for me to look at this one again.

::: docshell/test/browser/browser_principaltoinherit_data_redirect.js
@@ +5,5 @@
> +const DATA_URI = "data:text/html,<script>if(performance.navigation.type==0){location.reload()}</script>"
> +
> +add_task(function* test_principal_when_redirect_to_data() {
> +  yield BrowserTestUtils.withNewTab(TEST_PAGE, function*(browser) {
> +    let loadPromise = BrowserTestUtils.browserLoaded(browser, false, DATA_URI);

Yeah, I wouldn't trust this to have resolved only after the last redirect, I expect it'll intermittently resolve after only the first redirect (ie before the .reload() takes effect).

I'd use the ContentTask, and in the ContentTask, before asserting anything, create a waitForEvent promise (https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/ContentTaskUtils.jsm#100 ) that waits for a load on a document with the data URI documentURI and that has the performance.navigation.type set correctly (you can use the checkFn parameter for this).

@@ +7,5 @@
> +add_task(function* test_principal_when_redirect_to_data() {
> +  yield BrowserTestUtils.withNewTab(TEST_PAGE, function*(browser) {
> +    let loadPromise = BrowserTestUtils.browserLoaded(browser, false, DATA_URI);
> +    let myLink = browser.contentDocument.getElementById("testlink");
> +    myLink.click();

This uses CPOWs. It shouldn't. Use the contenttask to click the link (in the contenttask, you can just use content.document.getElementById(...).click()), or use BrowserTestUtils.synthesizeMouse() ( https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#751 ) with a selector.
Attachment #8855748 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8855455 [details] [diff] [review]
bug_1353975_do_not_inherit_principal_for_top_level_redirect_loads.patch

r=me
Attachment #8855455 - Flags: review?(bzbarsky) → review+
Gijs, thanks for the speedy review. I added a helper function for extracting the URI of a principal and further converted the test so the iframe waits to hear back from the parent before clicking the link. Carrying over r+.
Attachment #8855747 - Attachment is obsolete: true
Attachment #8855805 - Flags: review+
Comment on attachment 8855455 [details] [diff] [review]
bug_1353975_do_not_inherit_principal_for_top_level_redirect_loads.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I think the patch itself does not reveal the problem, but testcases definitely reveal the problem.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
comments in the patch: might reveal the problem
check-in comment: does not reveal the problem
tests: definitely reveal the issue

Which older supported branches are affected by this flaw?
Bug 1297338 which landed in FF52 introduced the problem.

If not all supported branches, which bug introduced the flaw?
See previous comment.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch would also work to fix the problem in FF52.

How likely is this patch to cause regressions; how much testing does it need?
We added two automated tests (one for top level loads one for iframe loads).
Attachment #8855455 - Flags: sec-approval?
I wouldn't mind taking this for 53 but we've run out of betas. For this to get sec-approval right now, it needs branch approvals for affected branches (and nominated patches for them) from release management. Otherwise, it isn't going to be approved to land before May 2, two weeks into the next cycle.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Yes, we can land this today for next monday's 53 RC build.
Flags: needinfo?(lhenry)
Comment on attachment 8855455 [details] [diff] [review]
bug_1353975_do_not_inherit_principal_for_top_level_redirect_loads.patch

Sec-approval+ based on comment 25 and talking with Al.
Attachment #8855455 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/96596bdeb7ecaa736e692b253ff7787fb35e69f3
Flags: needinfo?(rkothari)
Flags: needinfo?(jcristau)
Flags: in-testsuite?
Comment on attachment 8855455 [details] [diff] [review]
bug_1353975_do_not_inherit_principal_for_top_level_redirect_loads.patch

Approval Request Comment
Fixing a sec-critical bug, see comment 24.

[Feature/Bug causing the regression]:
Bug 1297338 - Introduce the concept of principalToInherit to loadinfo

[User impact if declined]:
Potential XSS attack.

[Is this code covered by automated tests?]:
Yes, two tests; one for iframe navigations, one for top-level navigations (which we are finalizing at the moment, not r+ yet but working)

[Has the fix been verified in Nightly?]:
Not yet.

[Needs manual test from QE? If yes, steps to reproduce]:
No, automated test coverage. But obviously having QA on a sec-critical bug can't hurt.

[List of other uplifts needed for the feature/fix]:
---

[Is the change risky?]:
Not risky.

[Why is the change risky/not risky?]:
Setting the principalToInherit to a freshly created NullPrincipal after a server side redirect does not influence regular resource loads.

[String changes made/needed]:
no
Attachment #8855455 - Flags: approval-mozilla-beta?
Attachment #8855455 - Flags: approval-mozilla-aurora?
Attachment #8855455 - Flags: approval-mozilla-esr52?
Gijs, thanks for your help.
Attachment #8855748 - Attachment is obsolete: true
Attachment #8855869 - Flags: review?(gijskruitbosch+bugs)

Updated

2 years ago
Attachment #8855869 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 31

2 years ago
Comment on attachment 8855869 [details] [diff] [review]
bug_1353975_data_redirect_toplevel_test.patch

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

::: docshell/test/browser/browser_principaltoinherit_data_redirect.js
@@ +26,5 @@
> +      ok(!loadingPrincipal, "top level loads do not have a loadingPrincipal");
> +      let triggeringPrincipal = loadInfo.triggeringPrincipal.URI.spec;
> +      is(triggeringPrincipal, TEST_PAGE, "triggeringPrincipal for data: URI correct");
> +      let principalToInherit = loadInfo.principalToInherit;
> +      ok(principalToInherit.isNullPrincipal, "principalToInherit for data: URI correct");

Oh, d'oh, last minute addition: can you also check ok(content.document.nodePrincipal.isNullPrincipal, "Document should have null principal") ?
(In reply to :Gijs from comment #31)
> Oh, d'oh, last minute addition: can you also check
> ok(content.document.nodePrincipal.isNullPrincipal, "Document should have
> null principal") ?

Added - good catch actually - thanks! Carrying over r+ from Gijs!
Attachment #8855869 - Attachment is obsolete: true
Attachment #8855875 - Flags: review+
We have two failing tests on inbound:

[1] test_allowedDomainsXHR.js which we can fix more or less easily, see inline patch underneath
[2] dom/security/test/cors/test_CrossSiteXHR.html which is timing out :-(

+++ b/js/xpconnect/tests/unit/test_allowedDomainsXHR.js
@@ -80,19 +80,17 @@ function run_test()
   // This request bounces to server 2 and then back to server 1.  Neither of
   // these servers support CORS, but if the expanded principal is used as the
   // triggering principal, this should work.
   cu.evalInSandbox('var createXHR = ' + createXHR.toString(), sb);
   var res = cu.evalInSandbox('var sync = createXHR("4444/redirect"); sync.send(null); sync', sb);
   do_check_true(checkResults(res));
 
   var principal = res.responseXML.nodePrincipal;
-  do_check_true(principal.isCodebasePrincipal);
-  var requestURL = "http://localhost:4444/simple";
-  do_check_eq(principal.URI.spec, requestURL);
+  do_check_true(principal.isNullPrincipal);
Aryx backed this out for the aforementioned failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c615bd0dfc44
> [1] test_allowedDomainsXHR.js which we can fix more or less easily, see inline patch underneath

Per IRC discussion, the test is right; the patch is what needs adjusting.  The same change as will fix test_CrossSiteXHR.html should fix this.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #24)
> > How easily could an exploit be constructed based on the patch?
> I think the patch itself does not reveal the problem, but testcases
> definitely reveal the problem.

The comment in the patch says exactly what the problem is and how to exploit it :-(

PLEASE strip that when we uplift this fix to aurora and beta. And when we re-land, for that matter.
Boris, I verified that:

* our two attached tests (iframe and toplevel) still work correctly
* dom/security/test/cors/test_CrossSiteXHR.html works again
* js/xpconnect/tests/unit/test_allowedDomainsXHR.js works again
Attachment #8855895 - Flags: review?(bzbarsky)
(In reply to Daniel Veditz [:dveditz] from comment #36)
> The comment in the patch says exactly what the problem is and how to exploit
> it :-(

I think that was my bad actually and not Ryan's fault, because in comment 24 I mentioned:
> check-in comment: does not reveal the problem

Anyway, we should obfuscate the commit message when re-landing.
Sounds like you are all working on re-landing this. Marking as a blocker for 53 release so that I can make sure we're ready for the RC build on monday.
Comment on attachment 8855895 [details] [diff] [review]
bug_1353975_do_not_inherit_principal_for_top_level_redirect_loads.patch

Please make sure there's a followup bug filed on a better fix for this, and reference that bug in the comment for the new code.  This bug may need to be security-sensitive for the moment, to avoid the commit message giving away the exploit.

I think the "says exactly what the problem is" bit is not the commit message, but the code comment in the patch.  Not saying anything about "data:" there is probably a good idea...
Attachment #8855895 - Flags: review?(bzbarsky) → review+
Yes, I meant
+    // avoid inheriting the wrong principal, e.g. in case when http
+    // redirects to data: URIs

tells anyone who sees it to play with redirecting to data: and see what "interesting" things happen.
Group: core-security → dom-core-security
Carrying over r+ from bz.
Attachment #8855455 - Attachment is obsolete: true
Attachment #8855895 - Attachment is obsolete: true
Attachment #8855455 - Flags: approval-mozilla-esr52?
Attachment #8855455 - Flags: approval-mozilla-beta?
Attachment #8855455 - Flags: approval-mozilla-aurora?
Attachment #8855902 - Flags: review+
Comment on attachment 8855902 [details] [diff] [review]
bug_1353975_do_not_inherit_principal_for_top_level_redirect_loads.patch

The patch slightly changed, here is the final one which is going to land.
Setting all the flags for this patch since the other one is obsolete now.

ckerschb : approval-mozilla-aurora?
ckerschb : approval-mozilla-beta?
RyanVM : approval-mozilla-esr52?
See comment 29

lizzard : sec-approval+
see comment 27
Attachment #8855902 - Flags: sec-approval+
Attachment #8855902 - Flags: approval-mozilla-esr52?
Attachment #8855902 - Flags: approval-mozilla-beta?
Attachment #8855902 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/cc68613d2a83
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8855902 [details] [diff] [review]
bug_1353975_do_not_inherit_principal_for_top_level_redirect_loads.patch

OK for beta uplift. Hoping to land this before the merge. If not, we will need to get it onto mozilla-release.
Attachment #8855902 - Flags: approval-mozilla-beta?
Attachment #8855902 - Flags: approval-mozilla-beta+
Attachment #8855902 - Flags: approval-mozilla-aurora?
Attachment #8855902 - Flags: approval-mozilla-aurora+
Comment on attachment 8855902 [details] [diff] [review]
bug_1353975_do_not_inherit_principal_for_top_level_redirect_loads.patch

fix this in 52.1.0esr as well
Attachment #8855902 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #40)
> Please make sure there's a followup bug filed on a better fix for this, and
> reference that bug in the comment for the new code.  This bug may need to be
> security-sensitive for the moment, to avoid the commit message giving away
> the exploit.

I filed Bug 1355055 as a follow up so we can think of replacing SEC_FORCE_INHERIT_PRINCIPAL with something better within docshell.
so this landed as https://hg.mozilla.org/releases/mozilla-beta/rev/9793272925b1 on beta (good news)

bad news is that i had to back this out from beta and aurora for bustage like 

https://treeherder.mozilla.org/logviewer.html#?job_id=90101849&repo=mozilla-aurora
Flags: needinfo?(ckerschb)
(In reply to Carsten Book [:Tomcat] from comment #50)
> so this landed as
> https://hg.mozilla.org/releases/mozilla-beta/rev/9793272925b1 on beta (good
> news)
> 
> bad news is that i had to back this out from beta and aurora for bustage
> like 

In older version it's still nsNullPrincipal, not NullPrincipal. This is the right patch for uplifting.
Flags: needinfo?(ckerschb)
Attachment #8856511 - Flags: review+
Comment on attachment 8856511 [details] [diff] [review]
bug_1353975_aurora_version.patch

Trying to land this before the beta merge and RC build today.
Attachment #8856511 - Flags: approval-mozilla-beta+
Attachment #8856511 - Flags: approval-mozilla-aurora+
Group: dom-core-security → core-security-release
Flags: qe-verify?
Whiteboard: [post-critsmash-triage]
Alias: CVE-2017-5466
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main53+][adv-esr52.1+]
(Reporter)

Comment 55

2 years ago
Thank you for fixing the bug surprisingly quickly.
I confirmed the bug doesn't reproduce in the latest version of Aurora v54.0a2.

Comment 56

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> (In reply to :Gijs from comment #3)
> > Setting flags based on bug 1317641.
> 
> I haven't debugged the problem yet, but I think the problem is that we have
> several security flags within the loadInfo which can appear together. In
> other words we should not do a == comparison but rather check if one of
> those flags is set by using &.
> 
> [1]
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 9935254c39eef9d14de419dd6163ff453cc1ce16#l1.18

Should we fix this in another bug? Still looks wrong to me based on your comment...
Flags: needinfo?(ckerschb)
(In reply to :Gijs from comment #56)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> > (In reply to :Gijs from comment #3)
> > > Setting flags based on bug 1317641.
> > 
> > I haven't debugged the problem yet, but I think the problem is that we have
> > several security flags within the loadInfo which can appear together. In
> > other words we should not do a == comparison but rather check if one of
> > those flags is set by using &.
> > 
> > [1]
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > 9935254c39eef9d14de419dd6163ff453cc1ce16#l1.18
> 
> Should we fix this in another bug? Still looks wrong to me based on your
> comment...

Sorry for not following up on that, in fact that works correctly because it calls ::GetSecuritymode() and not as I initially thought ::GetSecurityFlags(). GetSecurityMode() returns only one of the five actual security flags [1].

What we could update though is the line
>     nsSecurityFlags securityFlags = loadInfo->GetSecurityMode();
and replace securityFlags with securityMode to avoid the confusion.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#507
Flags: needinfo?(ckerschb)
This seems to have broken installing add-ons from non-whitelisted websites
Depends on: 1356292
I assume the fix at https://bugzilla.mozilla.org/show_bug.cgi?id=1356292#c28 doesn't undo the security fix?
Flags: needinfo?(dtownsend)
(In reply to Al Billings [:abillings] from comment #59)
> I assume the fix at https://bugzilla.mozilla.org/show_bug.cgi?id=1356292#c28
> doesn't undo the security fix?

As far as I know no. bz would probably know better than me though.
Flags: needinfo?(dtownsend) → needinfo?(bzbarsky)
Correct.  The code added in this bug is staying as-is.  The changed in bug 1356292 adjust the addon install code to not check the principalToInherit so that they are not affected by the changes made in this bug.
Flags: needinfo?(bzbarsky)
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.