Closed Bug 1394554 (CVE-2017-7841) Opened 7 years ago Closed 7 years ago

Firefox still allows loading data URL on top

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- disabled
firefox57 --- fixed

People

(Reporter: s.h.h.n.j.k, Assigned: ckerschb)

References

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: [domsecurity-active][adv-main57-])

Attachments

(3 files, 4 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36

Steps to reproduce:

1. Enable pref security.data_uri.block_toplevel_data_uri_navigations to true
2 Go to https://test.shhnjk.com/location.php?url=data:text/html,<script>alert(1)</script>


Actual results:

Data URL is loaded on top


Expected results:

It shouldn't. As per fix of https://bugzilla.mozilla.org/show_bug.cgi?id=1331351
Are we not catching redirects or something?
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Security
Flags: needinfo?(ckerschb)
Product: Firefox → Core
(In reply to Daniel Veditz [:dveditz] from comment #1)
> Are we not catching redirects or something?

oh yeah, redirects are not handled correctly, because the code lives within docshell. We need to move it to the contentSecuritymanager which can handle redirects. Since this feature is experimental and lives behind a pref, I don't think this bug needs to be hidden. If we can unhide it, we should also mark it blocking Bug 1331351.
Flags: needinfo?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> (In reply to Daniel Veditz [:dveditz] from comment #1)
> > Are we not catching redirects or something?
> 
> oh yeah, redirects are not handled correctly, because the code lives within
> docshell. We need to move it to the contentSecuritymanager which can handle
> redirects. Since this feature is experimental and lives behind a pref, I
> don't think this bug needs to be hidden. If we can unhide it, we should also
> mark it blocking Bug 1331351.

Oh but that load should still go through docshell if I am not mistaken. I will take a closer look at that in the upcoming days. Potentially we have to modify the loadinfo arguments for that load. The initial load of the https://bla load uses a systemprincipal as the triggeringPricnipal (because the URL load was explicitly triggered by the user) which potentially is then copied for the data: URI load. What should happen is that we use a triggeringPrincipal of the https page for the data: URI load.
Assignee: nobody → ckerschb
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Whiteboard: [domsecurity-active]
Here is an automated testcase to highlight the problem, and then there is also Bug 1395948 which we have to fix to make toplevel data: URI blocking work correctly.
Hey smaug, I looked at Bug 1395948, and while I agree that we are blocking data: URI navigations too late in the regular load case, we are also facing the problem described in this bug, where we are not catching redirects. I guess we have to move the check into the contentSecuritymanager which is also consulted after redirects, but that makes the blocking the regular load case happen even later. I don't have a strong opinion, but was wondering if you would prefer two 2 separate checks:
* one early within the regular docshell loading process (basically fixing Bug 1395948)
* one within the contentSecurityManager - catching redirects.

What do you think?
Flags: needinfo?(bugs)
I prefer two checks, similar what we need for content policies.
Flags: needinfo?(bugs)
Hey Smaug, please note that this patch relies on the changes within Bug 1395948.

For this bug: Since we also have to account for redirects when blocking top level data: URI navigations, I created a separate function AllowTopLevelNavigationToDataURI() which can be consulted from docshell as well as from within the contentSecurityManager when encounting a redirect. There are two cases (and I will upload automated tests in a minute for each in a minute):

a) A link click which loads a toplevel page which then gets redirected to a data: URI, that case would be handled by just adding the additional check to the redirect handler, because the triggeringPrincipal would still be a codebasePrincipal.

b) If someone pastes a http: URL into the address bar which then gets redirected to a data: URI. In that case the triggerignPricnipal would remain a systemprincipal and would allow the data: URI redirect to happen. I guess in that case we *always* want to block, hence I think it makes sense to explicitly pass a NullPrincipal to AllowTopLevelNavigationToDataURI(). If we go down that route, that we always block redirects to a data: URI then we don't have to explicitly store the 'loadFromExternal' flag in the loadInfo. If you agree to that then I would remove the 'loadFromExternal' bits from the patch.

What do you think?
Attachment #8904002 - Attachment is obsolete: true
Attachment #8904458 - Flags: feedback?(bugs)
Attachment #8904460 - Flags: review?(bugs)
Attachment #8904459 - Flags: review?(bugs) → review+
Comment on attachment 8904460 [details] [diff] [review]
bug_1394554_browser_redirect_tests.patch

"data: URI navigation after redirect from system should be blocked"
'from system' sounds a bit odd. 'from server' perhaps?

Should there be another test for <meta> redirect?
Attachment #8904460 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #10)
> Should there be another test for <meta> redirect?

Yes, there definitely should - I'll add one.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> b) If someone pastes a http: URL into the address bar which then gets
> redirected to a data: URI. In that case the triggerignPricnipal would remain
> a systemprincipal and would allow the data: URI redirect to happen. I guess
> in that case we *always* want to block, hence I think it makes sense to
> explicitly pass a NullPrincipal to AllowTopLevelNavigationToDataURI().
But the initial load from system principal should work, no? I guess you mean passing NullPrincipal in redirect case only?
Comment on attachment 8904458 [details] [diff] [review]
bug_1394554_block_data_navigation_after_redirect.patch

Hmm, I guess blocking redirect to data: always makes sense. Otherwise some bit.ly or such url could be used to trick one to load data: url.
So perhaps we don't need the loadFromeExternal flag, if we always block data url loading for redirects.
Attachment #8904458 - Flags: feedback?(bugs)
(In reply to Olli Pettay [:smaug] from comment #12)
> But the initial load from system principal should work, no? I guess you mean
> passing NullPrincipal in redirect case only?

Yes, only talking about the redirect case here. The initial load should work.
Updated to include test for meta redirect. Carrying over r+ from smaug.
Attachment #8904460 - Attachment is obsolete: true
Attachment #8904512 - Flags: review+
As discussed, no need for passing around the 'loadFromExternal' flag since toplevel redirects to a data: URI will always be blocked.
Attachment #8904458 - Attachment is obsolete: true
Attachment #8904513 - Flags: review?(bugs)
Comment on attachment 8904513 [details] [diff] [review]
bug_1394554_block_data_navigation_after_redirect.patch

>+  // Let's block all toplevel document navigations to a data: URI.
>+  // In all cases where the toplevel document is navigated to a
>+  // data: URI the triggeringPrincipal is a codeBasePrincipal, or
>+  // a NullPrincipal. In other cases, e.g. typing a data: URL into
>+  // the URL-Bar, the triggeringPrincipal is a SystemPrincipal;
>+  // we don't want to block those loads. Only exception, loads coming
>+  // from an external applicaton (e.g. Thunderbird) don't load
application

>   if (loadInfo && loadInfo->GetEnforceSecurity()) {
A bit confusing to have the check for GetEnforceSecurity() blocking 
AllowTopLevelNavigationToDataURI, but should be in practice ok.

>+    // Redirecting to a toplevel data: URI is not allowed, hence we pass
>+    // a NullPrincipal as the TriggeringPrincipal to
>+    // AllowTopLevelNavigationToDataURI() which definitely blocks any
>+    // data: URI load.
>+    nsCOMPtr<nsILoadInfo> newLoadInfo = aNewChannel->GetLoadInfo();
>+    nsCOMPtr<nsIURI> uri;
>+    rv = NS_GetFinalChannelURI(aNewChannel, getter_AddRefs(uri));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIPrincipal> nullTriggeringPrincipal = NullPrincipal::Create();
>+    if (!nsContentSecurityManager::AllowTopLevelNavigationToDataURI(
>+          uri,
>+          newLoadInfo->GetExternalContentPolicyType(),
>+          nullTriggeringPrincipal,
>+          false)) {
>+        // logging to console happens within AllowTopLevelNavigationToDataURI
>+        aOldChannel->Cancel(NS_ERROR_DOM_BAD_URI);
>+      return NS_OK;
Why you return NS_OK here? Return NS_ERROR_DOM_BAD_URI, and I guess there shouldn't be need to cancel the old channel.
Attachment #8904513 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #17)
> >   if (loadInfo && loadInfo->GetEnforceSecurity()) {
> A bit confusing to have the check for GetEnforceSecurity() blocking 
> AllowTopLevelNavigationToDataURI, but should be in practice ok.

In the end it doesn't really matter as you mentioned. Anyway, semantically it feels odd, I refactored that line to always check if we have a loadinfo.

> >+    if (!nsContentSecurityManager::AllowTopLevelNavigationToDataURI(
> >+          uri,
> >+          newLoadInfo->GetExternalContentPolicyType(),
> >+          nullTriggeringPrincipal,
> >+          false)) {
> >+        // logging to console happens within AllowTopLevelNavigationToDataURI
> >+        aOldChannel->Cancel(NS_ERROR_DOM_BAD_URI);
> >+      return NS_OK;
> Why you return NS_OK here? Return NS_ERROR_DOM_BAD_URI, and I guess there
> shouldn't be need to cancel the old channel.

Good catch - updated - thanks.

Carrying over r+ from smaug.
Attachment #8904513 - Attachment is obsolete: true
Attachment #8904541 - Flags: review+
Blocks: 1331351
Group: dom-core-security
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb932a1656cd
Block toplevel data: URI navigations after redirect. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb3040f8887
Test block data: URI toplevel navigations after redirect. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d80fa4d12362
Test block data: URI toplevel navigations after redirect. r=smaug
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main57+]
Alias: CVE-2017-7841
Whiteboard: [domsecurity-active][adv-main57+] → [domsecurity-active][adv-main57-]
You need to log in before you can comment on or make changes to this bug.