Open Bug 1316087 Opened 8 years ago Updated 2 years ago

Do not fall back to systemPrincipal within LoadURI when load is of TYPE_SUBDOCUMENT

Categories

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

defect

Tracking

()

People

(Reporter: ckerschb, Unassigned)

References

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(2 files)

In that case we should just bail out and do not allow the load.
Depends on: 1308889
Priority: -- → P2
Whiteboard: [domsecurity-active]
Hey Tanvi, as discussed in the meeting we should deny the load if there is no valid triggeringPrincipal for TYPE_SUBDOCUMENT loads. https://treeherder.mozilla.org/#/jobs?repo=try&revision=88dc3fc62bee840cdf78deb41e00459b85de7475
Attachment #8808749 - Flags: review?(tanvi)
Attachment #8808749 - Flags: review?(tanvi) → review+
(In reply to Christoph Kerschbaumer [:ckerschb (limited availability)] from comment #1) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=88dc3fc62bee840cdf78deb41e00459b85de7475 Unfortunately, try doesn't look happy.
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
(In reply to Tanvi Vyas [:tanvi] from comment #2) > Unfortunately, try doesn't look happy. So this one isn't that easy unfortunately. I looked into it yesterday and today (attaching a WIP patch of what I've tried so far). Apparently using isFrame() itself is not sufficient to determine whether we are performing a load of TYPE_SUBDOCUMENT or not. In more detail, loads coming from LoadURIWithOptions to not provide a triggeringPrincipal on the loadInfo but can perform a TYPE_DOCUMENT or a TYPE_SUBDOCUMENT load.
So, can we use a docshell member variable that tells us whether we fallback to system? We can create a docshell member variable "boolean triggerFellBackToSystem" and set it to false by default. Then here we can set it to true: https://dxr.mozilla.org/mozilla-central/rev/f8ba9c9b401f57b0047ddd6932cb830190865b38/docshell/base/nsDocShell.cpp#1524 Then in InternalLoad, in this if, we can fail if triggerFellBackToSystem is true for TYPE_SUBDOCUMENT: https://dxr.mozilla.org/mozilla-central/rev/f8ba9c9b401f57b0047ddd6932cb830190865b38/docshell/base/nsDocShell.cpp#9835 We could also potentially use this for https://bugzilla.mozilla.org/show_bug.cgi?id=1316256. When we call Content Policies, we can use set requestingPrincipal = nullptr if triggerFellBackToSystem is true. I'm not sure how future proof this is though. It only works as long as the fallbacks we are interested in live in nsDocShell. If we fallback to system outside of docshell, we will have to pass this information into docshell and set the member variable accordingly. Boris, what do you think?
Flags: needinfo?(bzbarsky)
We do not want to have member variables on docshell that are associated with a particular load, because that way lies insanity due to loads racing.... Even the ones we already have are a big pain to maintain. What happened with the simple approach? Is there chrome code doing direct loads using loadURI/loadURIWithOptions in subframes?
Flags: needinfo?(bzbarsky)
Blocks: 1182569
This bug may be easy to do after bug 1316256, where we can look at the aCPPrincipal and compare it to the triggeringPrincipal. If we have TYPE_SUBDOCUMENT and aCPPrincipal does not equal triggeringPrincipal, it means that we fell back to system principal for a frame load; I think. We can doublecheck that logic once 1316256 is ready.
I strongly feel that the thing we're adding in bug 1316256 should just be a temporary measure until we can provide Thunderbird with a sane API for doing what they want to do: provide a CSP of their choice for a non-HTTP channel.
What is the path forward on this bug? Bug 1316256 ended up not fixing this.
Flags: needinfo?(ckerschb)
Flags: needinfo?(bzbarsky)
I thought our conclusion was that we have lots of tests and whatnot and possibly extensions "legitimately" doing this (in the sense that they are navigating a subframe via the nsIWebNavigation API, and then there is no way to pass in any principals).
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9) > I thought our conclusion was that we have lots of tests and whatnot and > possibly extensions "legitimately" doing this (in the sense that they are > navigating a subframe via the nsIWebNavigation API, and then there is no way > to pass in any principals). Within 'Bug 1316275 - Try to explicitly pass aTriggeringPrincipal to nsDocShell::LoadURI' I tried to extend the argument list within a TriggeringPrincipal so callsites using nsIWebNavigation to load a URI could pass the right principal. But it's also complicated and smaug isn't really in favor of it [1]. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1316275#c4
Flags: needinfo?(ckerschb)
Assignee: ckerschb → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
Whiteboard: [domsecurity-active] → [domsecurity-backlog1]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: