Closed Bug 1387684 Opened 7 years ago Closed 7 years ago

Permanent false-positive img-src-self-unique-origin.html | application crashed [@ mozilla::detail::nsStringRepr::First() const] after Assertion failure: mLength > 0 (|First()| called on an empty string), at nsTSubstring.cpp:755

Categories

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

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: ckerschb)

References

Details

(Keywords: crash, intermittent-failure, Whiteboard: [stockwell fixed:product][domsecurity-active])

Crash Data

Attachments

(1 file, 3 obsolete files)

bug_1387684_self_data_uri.patch
4.91 KB, patch
dveditz
: review+
Details | Diff | Splinter Review 
Filed by: wkocher [at] mozilla.com

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

https://queue.taskcluster.net/v1/task/KYmJJw8mS5WLOo6JZUn2qQ/runs/0/artifacts/public/logs/live_backing.log

This appears to have come in with this merge from m-c to autoland: https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=e5e994cb9d84ea35d4034c0fd21bb7e870d5f586

Tracking it down might be tough since inbound doesn't run mac stylo tests due to capacity issues.
:ckerschb, this is a very high frequency failure (basically perma fail), could you help out with this next week or help find someone who knows about the test that might be able to help out?
Flags: needinfo?(ckerschb)
Whiteboard: [stockwell needswork]
Blocks: 1387871
(In reply to Joel Maher ( :jmaher) (UTC-9) from comment #1)
> :ckerschb, this is a very high frequency failure (basically perma fail),
> could you help out with this next week or help find someone who knows about
> the test that might be able to help out?

Thanks for bringing this to my attention. It's odd that we haven't encountered that exact use-case yet. The problem occurs when using 'self' in a meta CSP within a data: URI.
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Flags: needinfo?(ckerschb)
Priority: P5 → P2
Whiteboard: [stockwell needswork] → [stockwell needswork][domsecurity-active]
Attached patch bug_1387684_self_data_uri.patch (obsolete) — — Splinter Review 
Dan, first, it's odd that we don't have a testcase for that problem in our own test suite - I filed Bug 1387871 to add one, but I don't have time right now to do that, probably in the upcoming days.

Anyway, when using 'self' within a meta CSP inside a data: URI we use the document's URI as selfURI (See SetRequestContext() in the patch). Currently that translates into the data: URI from the iframe's src. We have to special case that, because we should use the URI of mLoadingPrincipal in that case, which is http://web-platform.test:8000/content-security-policy/img-src/img-src-self-uniq. Ultimately, when flipping the pref for data: URIs to become unique, opaque, origins, that loadingPrincipal will then be a nullPrincipal and will translate into something like: moz-nullprincipal:{bcda66a9-70a3-41dd-9416-af916a434fef}.

For the sake of completeness, the reason the test was crashing was because of that assertion [1]. Glad we added that :-). Now that assertion doesn't fire, but the test is timing out because we haven't implement violation events (see Bug 1302962) yet.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#641
Attachment #8894258 - Flags: review?(dveditz)
Of course in terms of Firefox behavior not asserting and crashing is the right thing to do, but in terms of the bug summary and the way we're starring, this ain't right: this is not the permaorange on stylo, all the WebRTC failures below this are, this is asserting just as frequently (every single run, orange or green) on every single platform.
Summary: Permafailing Mac Stylo img-src-self-unique-origin.html | application crashed [@ mozilla::detail::nsStringRepr::First() const] after Assertion failure: mLength > 0 (|First()| called on an empty string), at nsTSubstring.cpp:755 → Permanent false-positive img-src-self-unique-origin.html | application crashed [@ mozilla::detail::nsStringRepr::First() const] after Assertion failure: mLength > 0 (|First()| called on an empty string), at nsTSubstring.cpp:755
Comment on attachment 8894258 [details] [diff] [review]
bug_1387684_self_data_uri.patch

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

::: dom/security/nsCSPContext.cpp
@@ +701,5 @@
>      mSelfURI = doc->GetDocumentURI();
>      mLoadingPrincipal = doc->NodePrincipal();
> +
> +    bool selfIsData =
> +      (NS_SUCCEEDED(mSelfURI->SchemeIs("data", &isData)) && isData);

s/isData/selfIsData
See Also: → 1386938
I think we should also update frame-src-self-unique-origin.ini within this patch, seems like the same failure - see Bug 1386938.
Blocks: 1386938
Comment on attachment 8894258 [details] [diff] [review]
bug_1387684_self_data_uri.patch

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

::: dom/security/nsCSPContext.cpp
@@ +705,5 @@
> +      (NS_SUCCEEDED(mSelfURI->SchemeIs("data", &isData)) && isData);
> +    if (selfIsData) {
> +      // when loading a data: URI we have to use the
> +      // loadingPrincipal's URI as the selfURI otherwise
> +      // parsing 'self' translates into a data: URI.

Pretty sure this isn't what we want. In the old "inheriting" case we should be using the inherited CSP and the inherited 'self' (maybe we don't ever get here in that case). When data: is a unique origin then 'self' should never match anything according to the URL spec and comments in WPT tests (my naive assumption was that it should match exactly itself, but that's not useful).
Attachment #8894258 - Flags: review?(dveditz) → review-
As discussed with Dan over IRC, we need a slightly different path here. I will get that ready ASAP. Probably worth adding the test from Bug 1387871 to this bug.
Blocks: 1324406
Attached patch bug_1387684_self_data_uri.patch (obsolete) — — Splinter Review 
Dan, as discussed over IRC we need to special case unique opaque origins so that 'self' translates into something special, not matching anything.
As you posted on IRC:
> according to the URL spec (which I got to following links from the CSP2 spec) a unique origin isn't even supposed to match itself

I propose that a data: URI (when loaded as unique opaque origin) should translate
  from -> moz-nullprincipal:{4ebb3f7a-5bd3-4c56-82f5-4156522fb8ef}
  into -> unique-opaque-origin://4ebb3f7a-5bd3-4c56-82f5-4156522fb8ef

The above proposed solution would do exactly that. I agree it's a little hacky, but I added lots of comments in the code to explain that behavior.

Please note that I wrote a testcase within Bug 1387871 to verify correct behavior. I will flag you for review on that in a minute.
Attachment #8894258 - Attachment is obsolete: true
Attachment #8897812 - Flags: review?(dveditz)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
>  https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=06df7ebe60a09858b84c49d3e233c7c58e99c5bb

James, it seems the two wpt tests that this bug is fixing causes problems on TRY. Probably because I forgot to run some mach command to update the MANIFEST.json, right? I just ran |./mach wpt-manifest-update| locally. While this updates the MANIFEST.json in a lot of places, e.g.

    "cssom-view/elementFromPoint-002.html": [
-   "36b8a5f50e6489f9e25c3d09dc523007d442e2b3",
+   "36d7e75021f7f6ab8f89b2654ba3c8f818af16b8",
    "testharness"
   ],

it's not updating frame-src-self-unique-origin.html or img-src-self-unique-origin.html. I suppose because I just update those *.ini files, right? Anyway, just to make sure, what do I have to do in that case?
Flags: needinfo?(james)
So the MANIFEST.json doesn't contain the expected results. To update those look in the ini file testing/web-platform/meta/content-security-policy/img-src/img-src-self-unique-origin.html.ini and similar for the other file. There is a mach command to help with this (mach wpt-udpate), but for such a small number of changes it isn't worthwhile.

Do you know why those tests still time out?
Flags: needinfo?(james)
(In reply to James Graham [:jgraham] from comment #21)
> So the MANIFEST.json doesn't contain the expected results. To update those
> look in the ini file
> testing/web-platform/meta/content-security-policy/img-src/img-src-self-
> unique-origin.html.ini and similar for the other file. There is a mach
> command to help with this (mach wpt-udpate), but for such a small number of
> changes it isn't worthwhile.

Ok - thanks.

> Do you know why those tests still time out?

Yes, because we haven't implemented security policy violation events yet (See Bug 1302962). I update the *.ini files to indicate that:
https://hg.mozilla.org/try/rev/9a37b24b8feb370a8961aabc742831e2a9fc0513#l3.10
Comment on attachment 8897812 [details] [diff] [review]
bug_1387684_self_data_uri.patch

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

As we discussed on vidyo, I'm not keen on this approach.

::: dom/security/nsCSPContext.cpp
@@ +711,5 @@
> +      // within CSP_CreateHostSrcFromSelfURI() so that unique opaque origins are
> +      // treated as such, hence converting 'self'
> +      //   from -> moz-nullprincipal:{4ebb3f7a-5bd3-4c56-82f5-4156522fb8ef}
> +      //   into -> unique-opaque-origin://4ebb3f7a-5bd3-4c56-82f5-4156522fb8ef
> +      mLoadingPrincipal->GetURI(getter_AddRefs(mSelfURI));

I'm concerned about all the other cases where we're using the document URI rather than a principal. "data:" urls aren't the only case where the documentURI is a lie -- that's why we still use principals (their original use-case is long gone). about:srcdoc and about:blank are others that pop off the top of my head. And then there's the problem that data: isn't the only kind of "unique origin" there is (file:/// don't have hosts either). This special-casing here only solves part of the problem.

We also use mSelfURI for the "current document" in violation reports (in which case documentURI makes sense!). Sending "moz-nullprincipal" or "unique-opaque-origin" isn't going to help much. Not all that common though so we can just file a separate bug to fix that problem.

::: dom/security/nsCSPUtils.cpp
@@ +293,5 @@
> +                                             closeCurly - (openCurly + 1))));
> +     hostsrc->setGeneratedFromSelfKeyword();
> +     hostsrc->setScheme(NS_ConvertUTF8toUTF16("unique-opaque-origin"));
> +     return hostsrc;
> +  }

It would be better to add a boolean for "unique origin" to nsCSPHostSrc and then automatically fail all calls to permits(). Any self URL with a null host could be considered a unique origin, wouldn't need to do this conversion.
Attachment #8897812 - Flags: review?(dveditz) → review-
Attached patch bug_1387684_self_data_uri.patch (obsolete) — — Splinter Review 
(In reply to Daniel Veditz [:dveditz] from comment #25)
> It would be better to add a boolean for "unique origin" to nsCSPHostSrc and
> then automatically fail all calls to permits(). Any self URL with a null
> host could be considered a unique origin, wouldn't need to do this
> conversion.

As discussed on vidyo I agree with that approach. I updated the patch and added a boolean to nsCSPHostSrc. I also updated the testcase within Bug 1387871, to make sure a data URI iframe using the keyword 'self' can not load a data:image. Applying the patch in this bug and running the test within Bug 1387871 logs the following message to the console:

> Content Security Policy: The page’s settings blocked the loading of a resource at data:image/png;base64,iVBORw0KGgoAAAANSU... (“img-src data://”).

I think we can live with that console message.
Attachment #8897812 - Attachment is obsolete: true
Attachment #8899115 - Flags: review?(dveditz)
(In reply to Daniel Veditz [:dveditz] from comment #25)
> ::: dom/security/nsCSPUtils.cpp
> @@ +293,5 @@
> > +                                             closeCurly - (openCurly + 1))));
> > +     hostsrc->setGeneratedFromSelfKeyword();
> > +     hostsrc->setScheme(NS_ConvertUTF8toUTF16("unique-opaque-origin"));
> > +     return hostsrc;
> > +  }
> 
> It would be better to add a boolean for "unique origin" to nsCSPHostSrc and
> then automatically fail all calls to permits(). Any self URL with a null
> host could be considered a unique origin, wouldn't need to do this
> conversion.

Doesn't my patch from bug 1386938 already fix this?
Why does my bug was created first, and my patch was uploaded first, but my bug is duplicated without any reason, then in the end, my idea was shown here? 

Isn't fixing bugs is the most important thing to do?

Dan, Chris, could you comment?
Flags: needinfo?(dveditz)
Flags: needinfo?(ckerschb)
Comment on attachment 8899115 [details] [diff] [review]
bug_1387684_self_data_uri.patch

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

::: dom/security/nsCSPUtils.cpp
@@ +284,5 @@
> +  // Check for unique opaque origins
> +  bool selfIsData =
> +    (NS_SUCCEEDED(aSelfURI->SchemeIs("data", &selfIsData)) && selfIsData);
> +  if (selfIsData) {
> +    hostsrc->setIsUniqueOpageOrigin();

How come a sandbox iframe won't call setIsUniqueOpaqueOrigin(), we want to have different call paths for all kinds of unique opaque origins?

And does this even compile?
(In reply to Yoshi Huang [:allstars.chh] from comment #29)
> > +    hostsrc->setIsUniqueOpageOrigin();
> 
> How come a sandbox iframe won't call setIsUniqueOpaqueOrigin(), we want to
> have different call paths for all kinds of unique opaque origins?

Can you elaborate on this? What exactly do you mean?
Flags: needinfo?(ckerschb) → needinfo?(allstars.chh)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> Can you elaborate on this? What exactly do you mean?
Doesn't my patch from bug 1386938 already fix this?

(This is my 2nd time, and 1 needinfo? you to point this out)
Flags: needinfo?(allstars.chh) → needinfo?(ckerschb)
(In reply to Yoshi Huang [:allstars.chh] from comment #28)
> Doesn't my patch from bug 1386938 already fix this?
> Why does my bug was created first, and my patch was uploaded first, but my
> bug is duplicated without any reason, then in the end, my idea was shown here? 

I don't know, this is the first I've heard of the other bug. I was travelling to Oregon over the weekend and didn't see the ni? you added on me in the other bug on Friday
Flags: needinfo?(dveditz)
Comment on attachment 8899115 [details] [diff] [review]
bug_1387684_self_data_uri.patch

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

This looks like its fixing the test but I don't think it fixes the problem.

::: dom/security/nsCSPUtils.cpp
@@ +283,5 @@
>  
> +  // Check for unique opaque origins
> +  bool selfIsData =
> +    (NS_SUCCEEDED(aSelfURI->SchemeIs("data", &selfIsData)) && selfIsData);
> +  if (selfIsData) {

This doesn't address my primary objection in comment 25, that "data:" urls aren't the only ones that will cause problems. Does this work with about:srcdoc and about:blank+documentWrite() documents? Sandboxed iframes won't have a data: url but are unique origins. file:/// urls are unique origins...

I don't know if it's complete and accurate off the top of my head, but it seems that testing selfURI for a null host is a better determinant of being a unique origin than just checking for a 'data' scheme. But that's assuming the documentURI is even the right thing instead of using whatever the principal tells us.

@@ +633,5 @@
>      CSPUTILSLOG(("nsCSPHostSrc::permits, aUri: %s",
>                   aUri->GetSpecOrDefault().get()));
>    }
>  
> +  if (mInvalidated || mIsUniqueOpaqueOrigin) {

I like this part, I just think we're missing setting mIsUniqueOpaqueOrigin in many important cases.
Attachment #8899115 - Flags: review?(dveditz) → review-
Dan, as agreed over IRC, let's just check for an empty host which indicates we have to deal with an unique origin. Thanks for your feedback!
Attachment #8899115 - Attachment is obsolete: true
Flags: needinfo?(ckerschb)
Attachment #8900154 - Flags: review?(dveditz)
Comment on attachment 8900154 [details] [diff] [review]
bug_1387684_self_data_uri.patch

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

r=dveditz
Attachment #8900154 - Flags: review?(dveditz) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f96c5e184fbf
CSP: Special case 'self' for unique opaque origins. r=dveditz
https://hg.mozilla.org/mozilla-central/rev/f96c5e184fbf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Is there a user impact here that justifies Beta backport consideration or can
it ride the 57 train?
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox-esr52: --- → wontfix
Flags: needinfo?(ckerschb)
Flags: in-testsuite+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #40)
> Is there a user impact here that justifies Beta backport consideration or can
> it ride the 57 train?

I think it would be nice if we can backport it, but I let Dan decide.
Flags: needinfo?(ckerschb) → needinfo?(dveditz)
Whiteboard: [stockwell needswork][domsecurity-active] → [stockwell fixed:product][domsecurity-active]
This is intended to fix a test that's broken in nightly 57, and although there were broken edge-cases in the existing behavior that this fixes they aren't that common or crucial compared to the need for this fix in 57 because of bug 1324406. If we still had Aurora I'd be ok uplifting to that, but this close to the end of Beta: no way. Not a time to be dropping optional nice-to-haves.
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: