Closed
Bug 1138895
Opened 9 years ago
Closed 9 years ago
CSS data URL identified as certified app
Categories
(Firefox OS Graveyard :: Runtime, defect)
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)
2.28 KB,
patch
|
geekboy
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(ptheriault)
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
I had this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e388f18832c
Comment 3•9 years ago
|
||
(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.)
Updated•9 years ago
|
Flags: needinfo?(sstamm)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
Looks like if I just remove caching, it's good locally ...
Assignee | ||
Comment 6•9 years ago
|
||
reftests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20e4203efb75 gaia unit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef7a30851081
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 8•9 years ago
|
||
(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)
Assignee | ||
Comment 9•9 years ago
|
||
(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
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
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)
Comment 12•9 years ago
|
||
(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)?
Assignee | ||
Comment 13•9 years ago
|
||
(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)
Comment 14•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 17•9 years ago
|
||
reftests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5c8e3a58330
Assignee | ||
Comment 18•9 years ago
|
||
B2G Desktop Gu: https://treeherder.mozilla.org/#/jobs?repo=try&revision=135d87b6db21
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8573202 -
Attachment is obsolete: true
Attachment #8573202 -
Flags: feedback?(sstamm)
Assignee | ||
Updated•9 years ago
|
Attachment #8574291 -
Flags: review?(sstamm)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → lissyx+mozillians
Assignee | ||
Comment 20•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cd5b127d9b0
Assignee | ||
Comment 21•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8574291 -
Attachment is obsolete: true
Attachment #8574291 -
Flags: review?(sstamm)
Assignee | ||
Comment 22•9 years ago
|
||
Assignee | ||
Comment 23•9 years ago
|
||
(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
Assignee | ||
Updated•9 years ago
|
Attachment #8574412 -
Flags: review?(sstamm)
Assignee | ||
Comment 24•9 years ago
|
||
B2G Desktop Gu failure is unrelated: https://bugzilla.mozilla.org/show_bug.cgi?id=1140862
Comment 25•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 26•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/785891c8e997
Keywords: checkin-needed
Updated•9 years ago
|
Whiteboard: [systemsfe]
Target Milestone: --- → 2.2 S8 (20mar)
Updated•9 years ago
|
Flags: needinfo?(ptheriault)
You need to log in
before you can comment on or make changes to this bug.
Description
•