Closed Bug 1138895 Opened 9 years ago Closed 9 years ago

CSS data URL identified as certified app

Categories

(Firefox OS Graveyard :: Runtime, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
2.2 S8 (20mar)
Tracking Status
firefox39 --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 2 obsolete files)

Per bug 1138454 comment 1, while investigating the issue, I've noticed that the CSS data URL part that was getting blocked by CSP got identified as a certified app (https://hg.mozilla.org/mozilla-central/annotate/0b3c520002ad/dom/security/nsCSPService.cpp#l149), and that was the root cause of the blockage.

Now, removing this code with the patch in bug 1138454 will fix the problem I'm interested in, that is, getting the test green. But after speaking with Vivien of this, he fears that there may be a security-related bug there if this data URL gets identified as a certified app.
Flags: needinfo?(mozilla)
Flags: needinfo?(ptheriault)
No longer blocks: 1094369
So, in bug 1030936 we already tried to remove the fast path a while ago, but it caused test breakage on TBPL. Just chatted with Sid, he is going to answer. Hence I am forwarding the needinfo to him.
Flags: needinfo?(mozilla) → needinfo?(sstamm)
(In reply to Alexandre LISSY :gerard-majax from comment #0)
> But after speaking with
> Vivien of this, he fears that there may be a security-related bug there if
> this data URL gets identified as a certified app.

I believe this has to do with the way caching is implemented for the fast path, and is not a concern once we remove the fast path.

   152   aContentLocation->GetPrePath(contentOrigin);
   153   if (aRequestPrincipal && !mAppStatusCache.Get(contentOrigin, &status)) {
   154     aRequestPrincipal->GetAppStatus(&status);
   155     mAppStatusCache.Put(contentOrigin, status);
   156   }

For all loaded content URLs, we grab the origin, then look to see if the origin already has an affiliated app status.  Once a certified app loads a data: URI, then all data: URIs are marked/cached as "in a certified app".  Removing the app status should remove this horrendous flaw.

(FWIW, the error probably came about as the loading strategies changed and as we rewrote CSP and fixed many errors outside this code.)
Flags: needinfo?(sstamm)
So, Sid, I'm not sure to properly get what you suggest. Can we safely drop all this code, or there are still things blocking us ?

What do you exactly mean by « removing the app status » ? Removing the caching ?
Flags: needinfo?(sstamm)
Looks like if I just remove caching, it's good locally ...
(In reply to Alexandre LISSY :gerard-majax from comment #6)
> reftests:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=20e4203efb75
> gaia unit:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef7a30851081

So, just removing caching seems to make both Mulet and B2G Desktop Gu happy.
Keywords: checkin-needed
(In reply to Alexandre LISSY :gerard-majax from comment #4)
> So, Sid, I'm not sure to properly get what you suggest. Can we safely drop
> all this code, or there are still things blocking us ?
> 
> What do you exactly mean by « removing the app status » ? Removing the
> caching ?

Sorry, that was not what I meant to type.  I meant "removing the fast path".  Sorry for the confusion.

Ideally we should remove the whole fast path (not just the caching for the fast path), but it looks like that's not gonna work today (bug 1138454 got backed out).
Flags: needinfo?(sstamm)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #8)
> (In reply to Alexandre LISSY :gerard-majax from comment #4)
> > So, Sid, I'm not sure to properly get what you suggest. Can we safely drop
> > all this code, or there are still things blocking us ?
> > 
> > What do you exactly mean by « removing the app status » ? Removing the
> > caching ?
> 
> Sorry, that was not what I meant to type.  I meant "removing the fast path".
> Sorry for the confusion.
> 
> Ideally we should remove the whole fast path (not just the caching for the
> fast path), but it looks like that's not gonna work today (bug 1138454 got
> backed out).

Yes :). But if we just skip the caching for data URL, it should be okay. At least it does the trick locally.

I have those try pending:
 - for Gu: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4b0302e0c15
 - for reftests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a19008957b74
Comment on attachment 8573202 [details] [diff] [review]
Bug 1138454 - Remove CSP fast path hack for certified apps r=ckerschb

Would you be against this ? That is fixing both R2 for Mulet and not regressing B2G Desktop Gu
Attachment #8573202 - Flags: feedback?(sstamm)
(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Comment on attachment 8573202 [details] [diff] [review]
> Bug 1138454 - Remove CSP fast path hack for certified apps r=ckerschb
> 
> Would you be against this ? That is fixing both R2 for Mulet and not
> regressing B2G Desktop Gu

I don't like the one-off for data URLs because it's possible we'll have the same issue for other types of URLs and have to fix this bug again.  Can you instead whitelist http/https and app URLs (or is that too cumbersome)?
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12)
> (In reply to Alexandre LISSY :gerard-majax from comment #11)
> > Comment on attachment 8573202 [details] [diff] [review]
> > Bug 1138454 - Remove CSP fast path hack for certified apps r=ckerschb
> > 
> > Would you be against this ? That is fixing both R2 for Mulet and not
> > regressing B2G Desktop Gu
> 
> I don't like the one-off for data URLs because it's possible we'll have the
> same issue for other types of URLs and have to fix this bug again.  Can you
> instead whitelist http/https and app URLs (or is that too cumbersome)?

I'm not even sure I get what you have in mind :(.
Flags: needinfo?(sstamm)
Sorry that was not very clear.

(But really this caching is dangerous in general.  What we *really* need is a fixed Gu and then to remove the whole fast path.)

What I was suggesting is: instead of *not* caching data URIs, could you instead *cache* only URLs that are HTTP, HTTPS or app URIs?  So instead of just looking for a data scheme, you'd have to look for one of three.

But thinking more about this, I think we are doing the caching wrong.  We are caching the app status with the wrong URI:  We should be storing the app status with the *requesting* URI (this is the document that has the CSP), not the content that's being loaded (which is being subject to its parent document's CSP).

Does it fix the problem if instead of avoiding data: URIs in the cache, you instead simply change the caching to use the origin from aRequestOrigin instead of aContentLocation?
Flags: needinfo?(sstamm)
Thanks, I'll test this :)
Flags: needinfo?(lissyx+mozillians)
Seems ok locally.
Flags: needinfo?(lissyx+mozillians)
Blocks: 1138454
No longer depends on: 1138454
Attachment #8573202 - Attachment is obsolete: true
Attachment #8573202 - Flags: feedback?(sstamm)
Attachment #8574291 - Flags: review?(sstamm)
Assignee: nobody → lissyx+mozillians
(In reply to Alexandre LISSY :gerard-majax from comment #20)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cd5b127d9b0

Looks like we have some xpcshell tests failures. Reproduced locally, aRequestOrigin may be null.
Attachment #8574291 - Attachment is obsolete: true
Attachment #8574291 - Flags: review?(sstamm)
(In reply to Alexandre LISSY :gerard-majax from comment #22)
> Created attachment 8574412 [details] [diff] [review]
> Use proper origin for CSP fast path cache r=geekboy

full: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91524adde8a0
mulet reftest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76cfcc0ccb9b
Attachment #8574412 - Flags: review?(sstamm)
B2G Desktop Gu failure is unrelated: https://bugzilla.mozilla.org/show_bug.cgi?id=1140862
Comment on attachment 8574412 [details] [diff] [review]
Use proper origin for CSP fast path cache r=geekboy

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

looks good.
Attachment #8574412 - Flags: review?(sstamm) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/785891c8e997
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [systemsfe]
Target Milestone: --- → 2.2 S8 (20mar)
Flags: needinfo?(ptheriault)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: