Closed
Bug 1406278
Opened 7 years ago
Closed 7 years ago
Use the subject principal as the triggering principal for loads of script-generated content
Categories
(Core :: DOM: Security, enhancement)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 1 open bug)
Details
Attachments
(12 files)
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
In order to allow extension content scripts partial immunity to page CSP, and to give them permission to load their own resources into content pages without allowing content to do the same, we need to be able to map loads of the content they inject to the principal of the script that injected them. This bug only addresses basic injection of content generated by DOM APIs. There will be follow-ups for parser-generated content and inline stylesheets and scripts. There are probably some things I've missed, but I'm intentionally not addressing <embed> or <object> elements, since I don't want to encourage extensions to use them.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8915823 [details] Bug 1406278: Part 1 - Pass subject principal to SetAttribute and friends. https://reviewboard.mozilla.org/r/187034/#review192092 r=me with the various nits below fixed. ::: dom/base/AnonymousContent.h:43 (Diff revision 1) > > void GetTextContentForElement(const nsAString& aElementId, > DOMString& aText, > ErrorResult& aRv); > > void SetAttributeForElement(const nsAString& aElementId, Why do we need the argument here? This method is always called from system code, because this entire API is chromeonly... I don't even understand how this compiles, given that the only caller of this function is binding code and that would pass nsIPrincipal& anyway. I guess I'm OK with doing that, just to make it clear what's going on, instead of hardcoding SystemPrincipal() or nullptr here. But the signature does need to match. ::: dom/base/Attr.h:89 (Diff revision 1) > virtual JSObject* WrapNode(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > > // XPCOM GetName() is OK > - // XPCOM GetValue() is OK > > - void SetValue(const nsAString& aValue, ErrorResult& aRv); > + void GetValue(nsString& val, nsIPrincipal& aPrincipal) Just don't give aPrincipal a name if it's not used, please. ::: dom/base/Attr.h:94 (Diff revision 1) > - void SetValue(const nsAString& aValue, ErrorResult& aRv); > + void GetValue(nsString& val, nsIPrincipal& aPrincipal) > + { > + GetValue(val); > + } > + > + void SetValue(const nsAString& aValue, nsIPrincipal* aPrincipal, ErrorResult& aRv); Should be nsIPrincipal&, no? I guess you need both, so you can call it from the XPCOM side too. Or we could nix the XPCOM side first... ::: dom/base/Element.h:926 (Diff revision 1) > void GetAttribute(const nsAString& aName, DOMString& aReturn); > void GetAttributeNS(const nsAString& aNamespaceURI, > const nsAString& aLocalName, > nsAString& aReturn); > void SetAttribute(const nsAString& aName, const nsAString& aValue, > - ErrorResult& aError); > + nsIPrincipal* aPrincipal, ErrorResult& aError); nsIPrincipal& here.... Again, we probably need both overloads. ::: dom/base/Element.h:930 (Diff revision 1) > void SetAttribute(const nsAString& aName, const nsAString& aValue, > - ErrorResult& aError); > + nsIPrincipal* aPrincipal, ErrorResult& aError); > void SetAttributeNS(const nsAString& aNamespaceURI, > const nsAString& aLocalName, > const nsAString& aValue, > + nsIPrincipal* aPrincipal, And here, nsIPrincipal& in addition to the pointer version. ::: dom/base/Element.h:1487 (Diff revision 1) > * not previously unset). Otherwise it will be null. > * @param aParsedValue parsed new value of attribute. Replaced by the > * old value of the attribute. This old value is only > * useful if either it or the new value is StoresOwnData. > + * @param aSubjectPrincipal > + * the principal of the scripted caller responsible or "responsible for" ::: dom/base/Element.h:1592 (Diff revision 1) > * @param aOldValue the value that the attribute had previously. If null, > * the attr was not previously set. This argument may not have the > * correct value for SVG elements, or other cases in which the > * attribute value doesn't store its own data > + * @param aSubjectPrincipal the principal of the scripted caller responsible > + * for setting the attribute, or null if no scripted caller. Document that it will be null if the attr is being unset? ::: dom/base/nsIContent.h:398 (Diff revision 1) > * > * @param aNameSpaceID the namespace of the attribute > * @param aName the name of the attribute > * @param aPrefix the prefix of the attribute > * @param aValue the value to set > + * @param aSubjectPrincipal the principal of the scripted caller responsible It's worth documenting, here and in SetAttrAndNotify/AfterSetAttr, that this argument may NOT be reliable. In particular it can be null even when we _do_ have a scripted caller (e.g. if the attribute is reflected via a DOM property and the reflection code doesn't pass through the principal). So callees should NOT assume that null means no scripted caller unless they have audited all codepaths that call SetAttr/SetAttrAndNotify for the relevant attribute. I'm not sure about the aSubjectPrincipal naming given all of the above. aMaybeScriptedCallerPrincipal is too long, though.... aMaybeScriptPrincipal? Really not sure. ::: dom/html/HTMLImageElement.h:429 (Diff revision 1) > + nsIPrincipal* aSubjectPrincipal, > bool aValueMaybeChanged, bool aNotify); > > bool mInDocResponsiveContent; > RefPtr<ImageLoadTask> mPendingImageLoadTask; > + nsCOMPtr<nsIPrincipal> mSrcTriggeringPrincipal; This is unused; why is it here?
Attachment #8915823 -
Flags: review?(bzbarsky) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8915824 [details] Bug 1406278: Part 2a - Rename LoadingPrincipal to TriggeringPrincipal in imgLoader. https://reviewboard.mozilla.org/r/187036/#review192098 r=me, but should probably update nsContentUtils::LoadImage/CanLoadImage too, here or in a followup.
Attachment #8915824 -
Flags: review?(bzbarsky) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8915825 [details] Bug 1406278: Part 2b - Use subject principal as triggering principal in <img> "src" attribute. https://reviewboard.mozilla.org/r/187040/#review192104 r- because of the srcset/picture concern below. Apart from that and the relative URL issue, the rest is just nits. ::: dom/base/nsContentUtils.h:3062 (Diff revision 1) > * the value 'loadingprincipal' is also successfully deserialized, otherwise > * return false. > */ > static bool > GetLoadingPrincipalForXULNode(nsIContent* aLoadingNode, > - nsIPrincipal** aLoadingPrincipal); > + nsIPrincipal* aDefaultPrincipal, Needs documentation. In particular, that this can be null and then aLoadingNode->NodePrincipal() is used as the fallback. ::: dom/base/nsImageLoadingContent.h:106 (Diff revision 1) > * @param aForce If true, make sure to load the URI. If false, only > * load if the URI is different from the currently loaded URI. > * @param aNotify If true, nsIDocumentObserver state change notifications > * will be sent as needed. > * @param aImageLoadType The ImageLoadType for this request > + * @param aTriggeringPrincipal Optional parameter specifying the triggering Please document what happens if null is passed. ::: dom/base/nsImageLoadingContent.h:141 (Diff revision 1) > * @param aLoadStart If true, dispatch "loadstart" event. > * @param aDocument Optional parameter giving the document this node is in. > * This is purely a performance optimization. > * @param aLoadFlags Optional parameter specifying load flags to use for > * the image load > + * @param aTriggeringPrincipal Optional parameter specifying the triggering Document what null means. ::: dom/html/HTMLImageElement.cpp:421 (Diff revision 1) > > // Mark channel as urgent-start before load image if the image load is > // initaiated by a user interaction. > mUseUrgentStartForChannel = EventStateManager::IsHandlingUserInput(); > > + mSrcTriggeringPrincipal = aSubjectPrincipal; So I just realized a possible footgun we're introducing here. This exercise is premised on the fact that if an extension sets the src attribute then it controls what's being loaded and we should do the load. But if an extension sets the src attribute to a _relative_ url, then the extension and the page _together_ control the resulting URL. I wonder whether it's possible or desirable to restrict the CSP override to only the absolute URL case. ::: dom/html/HTMLImageElement.cpp:1007 (Diff revision 1) > > // If we have a srcset attribute or are in a <picture> element, > // we always use the Imageset load type, even if we parsed no > // valid responsive sources from either, per spec. > rv = LoadImage(src, aForce, aNotify, > HaveSrcsetOrInPicture() ? eImageLoadType_Imageset This is also pretty worrying to me. If the `<img>` is in a `<picture>`, or has a srcset, then mSrcTriggeringPrincipal has absolutely nothing to do with the URL that ends up loaded. For the moment, we should ensure that mSrcTriggeringPrincipal is used only when the URL came from this element's "src" attribute. ::: image/test/browser/browser_docshell_type_editor.js:112 (Diff revision 1) > // restore appType of rootDocShell before moving on to the next test > rootDocShell.appType = defaultAppType; > resolve(); > } > doc.body.appendChild(image); > - image.src = "chrome://test1/skin/privileged.png"; > + image.wrappedJSObject.src = "chrome://test1/skin/privileged.png"; Please add a comment about why the wrappedJSObject is necessary for this test to make sense? ::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:30 (Diff revision 1) > > async function testImageLoading(src, expectedAction) { > let imageLoadingPromise = new Promise((resolve, reject) => { > let cleanupListeners; > let testImage = document.createElement("img"); > - testImage.setAttribute("src", src); > + testImage.wrappedJSObject.setAttribute("src", src); Please add a comment about why this wrappedJSObject is needed. ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:66 (Diff revision 1) > + * the first leaf node encountered. > + * @param {string} test.src > + * The relative URL to use as the source of the element. Each > + * load of this URL will have a separate set of query parameters > + * appended to it, based on the values in `opts`. > + * @param {string} [test.srcAttr = "src"] I'm not sure what `= "src"` means here. The function doesn't do any defaulting if test.srcAtr is missing... Similar for missing test.liveSrc. ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:68 (Diff revision 1) > + * @param {string} [test.srcAttr = "src"] > + * The attribute in which to store the element's source URL. This comment is duplicated. ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:90 (Diff revision 1) > + * > + * - An element present in the initial page HTML. > + * - An element injected by a page script belonging to web > + * content. > + * - An element injected by an extension content script. > + * @returns {object} Structure of the return value? ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:226 (Diff revision 1) > + > + // Build the full element with a property setter. > + opts = overrideOpts({source: `${baseOpts.source}-prop`}); > + for (let test of tests) { > + let {elem, srcElem, src} = createElement(test, opts); > + srcElem[test.srcAttr] = src; This assumes that the DOM property always has the same name as the attribute... I guess that's OK for now; in practice it might be true for all the elements we care about. ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:263 (Diff revision 1) > + window.wrappedJSObject.elem = srcElem; > + window.wrappedJSObject.eval(`elem.setAttribute(${uneval(test.srcAttr)}, ${uneval(src)})`); > + } > + > + // Build the full element, then let content inject. > + // the corresponding property. Stray line here? ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:314 (Diff revision 1) > + * context. > + * @returns {string} > + */ > +function getInjectionScript(tests, opts) { > + return ` > + ${getElementData}; Do you need the trailing ';' here? ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:315 (Diff revision 1) > + * @returns {string} > + */ > +function getInjectionScript(tests, opts) { > + return ` > + ${getElementData}; > + ${createElement}; And here? ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:329 (Diff revision 1) > + * expected. > + * > + * @param {Array<object>} tests > + * A list of tests, as understood by {@see getElementData}. > + * @param {Array<string>} sources > + * A list source names. Each test is expected to be loaded once "of source names" ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:381 (Diff revision 1) > + /** > + * A list of tests to run in each context, as understood by > + * {@see getElementData}. > + */ > + const TESTS = [ > + { We should have srcset/picture tests here to make sure we handle that stuff right...
Attachment #8915825 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8915823 [details] Bug 1406278: Part 1 - Pass subject principal to SetAttribute and friends. https://reviewboard.mozilla.org/r/187034/#review192092 > Why do we need the argument here? This method is always called from system code, because this entire API is chromeonly... > > I don't even understand how this compiles, given that the only caller of this function is binding code and that would pass nsIPrincipal& anyway. I guess I'm OK with doing that, just to make it clear what's going on, instead of hardcoding SystemPrincipal() or nullptr here. But the signature does need to match. I guess we probably don't need it, but it seemed to make sense to treat it the same as other setAttribute APIs. It compiles because NotNull<nsIPrincipal> converts to either `nsIPrincipal*` or `nsIPrincipal&` > Should be nsIPrincipal&, no? > > I guess you need both, so you can call it from the XPCOM side too. Or we could nix the XPCOM side first... I actually initially had both `nsIPrincipal*` and `nsIPrincipal&` versions of this method, but since `NotNull<nsIPrincipal>` converts to either, it wouldn't compile since the calls were ambiguous. Same for the SetAttribute methods. > This is unused; why is it here? Oops. This was part of a later patch. Not sure how it wound up in this one.
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8915825 [details] Bug 1406278: Part 2b - Use subject principal as triggering principal in <img> "src" attribute. https://reviewboard.mozilla.org/r/187040/#review192104 > So I just realized a possible footgun we're introducing here. > > This exercise is premised on the fact that if an extension sets the src attribute then it controls what's being loaded and we should do the load. > > But if an extension sets the src attribute to a _relative_ url, then the extension and the page _together_ control the resulting URL. > > I wonder whether it's possible or desirable to restrict the CSP override to only the absolute URL case. Hm. That's a good point. I guess malicious content could do something interesting with <base href>... > This is also pretty worrying to me. If the `<img>` is in a `<picture>`, or has a srcset, then mSrcTriggeringPrincipal has absolutely nothing to do with the URL that ends up loaded. For the moment, we should ensure that mSrcTriggeringPrincipal is used only when the URL came from this element's "src" attribute. `mSrcTriggeringPrincipal` only applies to the `src` attribute (hence the name). I addressed `srcset` an <source srcset> in later patches. > I'm not sure what `= "src"` means here. The function doesn't do any defaulting if test.srcAtr is missing... > > Similar for missing test.liveSrc. I guess it's slightly misleading. The defaults are handled in the body of the test function, but I didn't have a better place to document them. > This assumes that the DOM property always has the same name as the attribute... I guess that's OK for now; in practice it might be true for all the elements we care about. Yeah, for all the cases I'm currently testing, they always have the same name. I can always add a `srcProp` property if we need to test any that aren't. > We should have srcset/picture tests here to make sure we handle that stuff right... They're tested in later patches :)
Comment 18•7 years ago
|
||
> It compiles because NotNull<nsIPrincipal> converts to either `nsIPrincipal*` or `nsIPrincipal&` Ah. Hrm... So the fact that NonNull is used here is a binding implementation detail; the API contract is nsIPrincipal&. Probably better to not depend on the implementation detail if we can avoid it. I wonder whether we should have avoided adding the "operator T*()" in bug 949456... > but since `NotNull<nsIPrincipal>` converts to either, it wouldn't compile since the calls were ambiguous. Ugh. :( I guess it worries me a bit to so easily lose track of whether things are null or not. Curses on C++. > `mSrcTriggeringPrincipal` only applies to the `src` attribute (hence the name). Ah, I see. I hadn't realized the picture/srcset cases had been peeled off before those lines. OK, that looks reasonable. Thoughts on the <base> situation?
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #18) > Thoughts on the <base> situation? Yeah, I was thinking about adding a nsContentUtils helper that takes the node, subject principal, and attribute value, and only returns the triggering principal if the value is an absolute URL.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8915825 [details] Bug 1406278: Part 2b - Use subject principal as triggering principal in <img> "src" attribute. https://reviewboard.mozilla.org/r/187040/#review192412 ::: dom/base/nsContentUtils.h:609 (Diff revisions 1 - 2) > + * Returns the triggering principal which should be used for the given URL > + * attribute value with the given subject principal. > + * > + * If the attribute value is not an absolute URL, the subject principal will > + * be ignored, and the node principal of aContent will be used instead. > + * If aContent is non-null, this function will always return a principal. Do any callers pass null? If not, just require aContent to be non-null. ::: dom/base/nsContentUtils.cpp:2354 (Diff revisions 1 - 2) > + } > + > + // If the attribute value is empty, it's not an absolute URL, so don't bother > + // with more expensive checks. > + if (!aAttrValue.IsEmpty() && > + net_IsAbsoluteURL(NS_ConvertUTF16toUTF8(aAttrValue))) { I'm not sure this will do what we want. net_IsAbsoluteURL is meant for use from nsStandardURL and will return false for anything that is not a plausible absolute _nsStandardURL_. In particular, it would return false for data: URIs or blob: URIs. I was going to suggest we just check whether net_ExtractURLScheme succeeds, but the problem is that "http:abc" can be a relative URL! Of course it can also be an absolute URL.... I guess we could sort of explicitly test for: 1. net_ExtractURLScheme succeeds. 2. Either the resulting scheme differs from the base URL of aContent, or net_IsAbsoluteURL tests true, or the url scheme doesn't end up using nsStandardURL. Not sure how to test this last bit offhand. Or something like that...
Attachment #8915825 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 33•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8915825 [details] Bug 1406278: Part 2b - Use subject principal as triggering principal in <img> "src" attribute. https://reviewboard.mozilla.org/r/187040/#review192412 > Do any callers pass null? If not, just require aContent to be non-null. ResponsiveImageSelector passes Content(), which isn't guaranteed to be non-null. I think it always is in the cases where we pass it a triggering principal, but it calls GetAttrTriggeringPrincipal regardless of whether the triggering principal is null. > I'm not sure this will do what we want. net_IsAbsoluteURL is meant for use from nsStandardURL and will return false for anything that is not a plausible absolute _nsStandardURL_. In particular, it would return false for data: URIs or blob: URIs. > > I was going to suggest we just check whether net_ExtractURLScheme succeeds, but the problem is that "http:abc" can be a relative URL! Of course it can also be an absolute URL.... > > I guess we could sort of explicitly test for: > > 1. net_ExtractURLScheme succeeds. > 2. Either the resulting scheme differs from the base URL of aContent, or net_IsAbsoluteURL tests true, or the url scheme doesn't end up using nsStandardURL. Not sure how to test this last bit offhand. > > Or something like that... Hm. That's a good point. So, the two possibilities I have for that are: 1) Whitelist non-StandardURL schemes we want to support (possibly just blob: and data:), or 2) Return false if `net_ExtractURLScheme` fails. Return true if `net_IsAbsoluteURL` succeeds. Return true if the scheme has the URI_NORELATIVE, according to the IO service. Which would at least restrict the more expensive checks to unusual corner cases. I'd prefer option 2.
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8915826 [details] Bug 1406278: Part 2c - Use subject principal as triggering principal in <img> "srcset" attribute. https://reviewboard.mozilla.org/r/187044/#review192436
Attachment #8915826 -
Flags: review?(bzbarsky) → review+
Comment 35•7 years ago
|
||
Option 2 makes sense to me.
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8915827 [details] Bug 1406278: Part 3 - Use subject principal as triggering principal in <script> "src" attribute. https://reviewboard.mozilla.org/r/187046/#review192478 ::: dom/html/HTMLScriptElement.cpp:247 (Diff revision 2) > if (nsGkAtoms::async == aName && kNameSpaceID_None == aNamespaceID) { > mForceAsync = false; > } > + if (nsGkAtoms::src == aName && kNameSpaceID_None == aNamespaceID) { > + mSrcTriggeringPrincipal = nsContentUtils::GetAttrTriggeringPrincipal( > + this, aValue->GetStringValue(), aSubjectPrincipal); aValue can be null here on UnsetAttr, no? Please deal with that. ::: dom/script/ScriptLoader.cpp:754 (Diff revision 2) > new ModuleLoadRequest(aRequest->mElement, aRequest->mJSVersion, > aRequest->mCORSMode, aRequest->mIntegrity, this); > > childRequest->mIsTopLevel = false; > childRequest->mURI = aURI; > + childRequest->mTriggeringPrincipal = aRequest->mTriggeringPrincipal; This is a bit suspect, in that the URI involved is coming from the module, not from the scripted caller that started the toplevel module load... But I guess we were treating it all as effectively having the node's principal as the triggering principal before, so this is no worse.
Attachment #8915827 -
Flags: review?(bzbarsky) → review+
Comment 37•7 years ago
|
||
mozreview-review |
Comment on attachment 8915828 [details] Bug 1406278: Part 4 - Use subject principal as triggering principal in <iframe>/<frame> "src" attribute https://reviewboard.mozilla.org/r/187048/#review192484 r=me ::: dom/base/nsFrameLoader.h:116 (Diff revision 2) > > void LoadFrame(mozilla::ErrorResult& aRv); > > void LoadURI(nsIURI* aURI, mozilla::ErrorResult& aRv); > > + nsresult LoadURI(nsIURI* aURI, nsIPrincipal* aPrincipal); aTriggeringPrincipal? And document that null is allowed and what it means? ::: dom/base/nsFrameLoader.h:329 (Diff revision 2) > * flags from our owning content's owning document with a logical OR, this > * ensures that we can only add restrictions and never remove them. > */ > void ApplySandboxFlags(uint32_t sandboxFlags); > > - void GetURL(nsString& aURL); > + void GetURL(nsString& aURL, nsIPrincipal** aPrincipal); aTriggeringPrincipal ::: dom/base/nsFrameLoader.h:389 (Diff revision 2) > nsresult ReallyLoadFrameScripts(); > > // Updates the subdocument position and size. This gets called only > // when we have our own in-process DocShell. > void UpdateBaseWindowPositionAndSize(nsSubDocumentFrame *aIFrame); > - nsresult CheckURILoad(nsIURI* aURI); > + nsresult CheckURILoad(nsIURI* aURI, nsIPrincipal* aPrincipal); aTriggeringPrincipal and again document the semantics. ::: dom/html/HTMLIFrameElement.h:55 (Diff revision 2) > > // Web IDL binding methods > - // The XPCOM GetSrc is fine for our purposes > + void GetSrc(nsString& aSrc, nsIPrincipal&) > - void SetSrc(const nsAString& aSrc, ErrorResult& aError) > { > - SetHTMLAttr(nsGkAtoms::src, aSrc, aError); > + GetSrc(aSrc); This is going to conflict with bug 1405792, fwiw.
Attachment #8915828 -
Flags: review?(bzbarsky) → review+
Comment 38•7 years ago
|
||
mozreview-review |
Comment on attachment 8915829 [details] Bug 1406278: Part 5a - Use subject principal as triggering principal in <audio>/<video> "src" attribute. https://reviewboard.mozilla.org/r/187050/#review192486 ::: dom/html/HTMLMediaElement.cpp:4470 (Diff revision 2) > { > if (aNameSpaceID == kNameSpaceID_None) { > if (aName == nsGkAtoms::src) { > mSrcMediaSource = nullptr; > + mSrcAttrTriggeringPrincipal = nsContentUtils::GetAttrTriggeringPrincipal( > + this, aValue->GetStringValue(), aSubjectPrincipal); aValue is null on attr removal. We may need tests for the removal case here and elsewhere, unless it's already tested.
Attachment #8915829 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 39•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8915827 [details] Bug 1406278: Part 3 - Use subject principal as triggering principal in <script> "src" attribute. https://reviewboard.mozilla.org/r/187046/#review192478 > This is a bit suspect, in that the URI involved is coming from the module, not from the scripted caller that started the toplevel module load... > > But I guess we were treating it all as effectively having the node's principal as the triggering principal before, so this is no worse. Yeah, I was on the fence about this. In the end, I came to the conclusion that, for the purposes of CSP (and other security checks), I think we need to treat the load of the whole module graph as being triggered by the same origin, or there's no point in applying it to module loads at all. Otherwise, any imports will be subject to page CSP, which means they would probably fail any time the original module load would have failed. Which means we would only effectively grant CSP overrides to modules scripts that don't import any other module (which would be essentially equivalent to non-module scripts).
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8915830 [details] Bug 1406278: Part 5b - Use subject principal as triggering principal in <source> "src" attribute for <audio>/<video>. https://reviewboard.mozilla.org/r/187052/#review192492 ::: dom/html/HTMLSourceElement.cpp:134 (Diff revision 2) > } > > } else if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::media) { > UpdateMediaList(aValue); > } else if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::src) { > + mSrcTriggeringPrincipal = nsContentUtils::GetAttrTriggeringPrincipal( aValue is null on removeAttribute.
Attachment #8915830 -
Flags: review?(bzbarsky) → review+
Comment 41•7 years ago
|
||
mozreview-review |
Comment on attachment 8915831 [details] Bug 1406278: Part 6 - Use subject principal as triggering principal in <source> "srcset" attribute for <picture>. https://reviewboard.mozilla.org/r/187054/#review192494 ::: dom/html/HTMLImageElement.cpp:1045 (Diff revision 2) > // We're currently using this node as our responsive selector > // source. > nsCOMPtr<nsIPrincipal> principal; > if (aSourceNode == this) { > principal = mSrcsetTriggeringPrincipal; > + } else if (aSourceNode->IsHTMLElement(nsGkAtoms::source)) { } else if (auto* source = HTMLSourceElement::FromContent(aSourceNode)) { so you don't check the tag twice. ::: dom/html/HTMLSourceElement.cpp:105 (Diff revision 2) > nsIPrincipal* aSubjectPrincipal, > bool aNotify) > { > + if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::srcset) { > + mSrcsetTriggeringPrincipal = nsContentUtils::GetAttrTriggeringPrincipal( > + this, aValue->GetStringValue(), aSubjectPrincipal); aValue is null on removeAttribute.
Attachment #8915831 -
Flags: review?(bzbarsky) → review+
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8915832 [details] Bug 1406278: Part 7 - Use subject principal as triggering principal in <input> "src" attribute. https://reviewboard.mozilla.org/r/187056/#review192496 ::: dom/html/HTMLInputElement.h:714 (Diff revision 2) > > - void GetSrc(nsAString& aValue) > + void GetSrc(nsAString& aValue, nsIPrincipal&) > { > GetURIAttr(nsGkAtoms::src, nullptr, aValue); > } > - void SetSrc(const nsAString& aValue, ErrorResult& aRv) > + void SetSrc(const nsAString& aValue, nsIPrincipal& aPrincipal, ErrorResult& aRv) aTriggeringPrincipal? Probably similar in earlier patches too, actually... ::: dom/html/HTMLInputElement.cpp:1387 (Diff revision 2) > UpdateValueMissingValidityStateForRadio(false); > } > > + if (aName == nsGkAtoms::src) { > + mSrcTriggeringPrincipal = nsContentUtils::GetAttrTriggeringPrincipal( > + this, aValue->GetStringValue(), aSubjectPrincipal); aValue is null on unset. ::: dom/html/HTMLInputElement.cpp:1391 (Diff revision 2) > + mSrcTriggeringPrincipal = nsContentUtils::GetAttrTriggeringPrincipal( > + this, aValue->GetStringValue(), aSubjectPrincipal); > + if (aNotify && mType == NS_FORM_INPUT_IMAGE) { > + if (aValue) { > + // Mark channel as urgent-start before load image if the image load is > + // initaiated by a user interaction. "initiated" (yes, I know you just moved this).
Attachment #8915832 -
Flags: review?(bzbarsky) → review+
Comment 43•7 years ago
|
||
mozreview-review |
Comment on attachment 8915833 [details] Bug 1406278: Part 8a - Cleanup some uses of CallQueryInterface. https://reviewboard.mozilla.org/r/187060/#review192498
Attachment #8915833 -
Flags: review?(bzbarsky) → review+
Comment 44•7 years ago
|
||
mozreview-review |
Comment on attachment 8915834 [details] Bug 1406278: Part 8b - Use subject principal as triggering principal in style <link> "href" attribute. https://reviewboard.mozilla.org/r/187062/#review192500 ::: dom/base/nsStyleLinkElement.h:96 (Diff revision 2) > mozilla::dom::ShadowRoot *aOldShadowRoot, > bool aForceUpdate = false); > > void UpdateStyleSheetScopedness(bool aIsNowScoped); > > - virtual already_AddRefed<nsIURI> GetStyleSheetURL(bool* aIsInline) = 0; > + virtual already_AddRefed<nsIURI> GetStyleSheetURL(bool* aIsInline, nsIPrincipal** aTriggeringPrincipal = nullptr) = 0; Why the default on the second arg? There is only one caller, and it's this class, and it will be pasing non-null.... Just document that it needs to not be null. ::: dom/base/nsStyleLinkElement.h:140 (Diff revision 2) > bool* aIsAlternate, > bool aForceUpdate); > > RefPtr<mozilla::StyleSheet> mStyleSheet; > protected: > + nsCOMPtr<nsIPrincipal> mTriggeringPrincipal; Why is this on nsStyleLinkElement if it's only used by HTMLLinkElement? Seems like it should live on HTMLLinkElement. ::: dom/html/HTMLLinkElement.h:93 (Diff revision 2) > - // XPCOM GetHref is fine. > + void GetHref(nsString& aHref, nsIPrincipal&) > - void SetHref(const nsAString& aHref, ErrorResult& aRv) > { > - SetHTMLAttr(nsGkAtoms::href, aHref, aRv); > + GetHref(aHref); > + } > + void SetHref(const nsAString& aHref, nsIPrincipal& aPrincipal, ErrorResult& aRv) aTriggeringPrincipal? ::: dom/html/HTMLLinkElement.cpp:314 (Diff revision 2) > } > } > > + if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::href) { > + mTriggeringPrincipal = nsContentUtils::GetAttrTriggeringPrincipal( > + this, aValue->GetStringValue(), aSubjectPrincipal); aValue is null on attr removal. ::: dom/html/HTMLLinkElement.cpp:442 (Diff revision 2) > nsAutoString href; > GetAttr(kNameSpaceID_None, nsGkAtoms::href, href); > if (href.IsEmpty()) { > return nullptr; > } > + if (aTriggeringPrincipal) { Can be assumed non-null here. ::: dom/html/HTMLLinkElement.cpp:443 (Diff revision 2) > GetAttr(kNameSpaceID_None, nsGkAtoms::href, href); > if (href.IsEmpty()) { > return nullptr; > } > + if (aTriggeringPrincipal) { > + auto prin = mTriggeringPrincipal; I'd rather have explicit nsCOMPtr<nsIPrincipal> here, because then it's a lot clearer what's going on. ::: dom/html/HTMLStyleElement.cpp:190 (Diff revision 2) > UpdateStyleSheetInternal(nullptr, nullptr); > } > > already_AddRefed<nsIURI> > -HTMLStyleElement::GetStyleSheetURL(bool* aIsInline) > +HTMLStyleElement::GetStyleSheetURL(bool* aIsInline, nsIPrincipal** aTriggeringPrincipal) > { Should really do `*aTriggeringPrincipal = nullptr;`. ::: dom/svg/SVGStyleElement.cpp:260 (Diff revision 2) > > //---------------------------------------------------------------------- > // nsStyleLinkElement methods > > already_AddRefed<nsIURI> > -SVGStyleElement::GetStyleSheetURL(bool* aIsInline) > +SVGStyleElement::GetStyleSheetURL(bool* aIsInline, nsIPrincipal** aTriggeringPrincipal) Again, should set `*aTriggeringPrincipal.`. ::: dom/xml/XMLStylesheetProcessingInstruction.cpp:103 (Diff revision 2) > { > mOverriddenBaseURI = aNewBaseURI; > } > > already_AddRefed<nsIURI> > -XMLStylesheetProcessingInstruction::GetStyleSheetURL(bool* aIsInline) > +XMLStylesheetProcessingInstruction::GetStyleSheetURL(bool* aIsInline, nsIPrincipal** aTriggeringPrincipal) And here. ::: layout/style/Loader.h:259 (Diff revision 2) > * be notified. In addition to loading the sheet, this method will insert it > * into the stylesheet list of this CSSLoader's document. > * > * @param aElement the element linking to the the stylesheet. May be null. > * @param aURL the URL of the sheet. > + * @param aTriggeringPrincipal the triggering principal for the load. Document that this can be null, in which case the NodePrincipal() of aElement, if present, or our document (if no aElement) is used? ::: layout/style/Loader.cpp:1953 (Diff revision 2) > > NS_ENSURE_TRUE(mDocument, NS_ERROR_NOT_INITIALIZED); > > - nsIPrincipal* principal = > - aElement ? aElement->NodePrincipal() : mDocument->NodePrincipal(); > + nsIPrincipal* principal = aTriggeringPrincipal; > + if (!principal) { > + principal = aElement ? aElement->NodePrincipal() : mDocument->NodePrincipal(); Does this fit in 80 chars? ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:456 (Diff revision 2) > { > element: ["input", {type: "image"}], > src: "input.png", > }, > { > + element: ["link", {rel: "stylesheet"}], Should be added to the empty element list, no?
Attachment #8915834 -
Flags: review?(bzbarsky) → review+
Comment 45•7 years ago
|
||
mozreview-review |
Comment on attachment 8915832 [details] Bug 1406278: Part 7 - Use subject principal as triggering principal in <input> "src" attribute. https://reviewboard.mozilla.org/r/187056/#review192502 ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:452 (Diff revision 2) > element: ["img", {}], > src: "imgset.png", > srcAttr: "srcset", > }, > { > + element: ["input", {type: "image"}], input should be in the empty tag list, right?
Assignee | ||
Comment 46•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8915834 [details] Bug 1406278: Part 8b - Use subject principal as triggering principal in style <link> "href" attribute. https://reviewboard.mozilla.org/r/187062/#review192500 > Why is this on nsStyleLinkElement if it's only used by HTMLLinkElement? Seems like it should live on HTMLLinkElement. Mainly under the assumption that it would be needed in other style elements in the future. I was planning to handle capturing the triggering principal for HTML <style> nodes in a follow-up bug soon, and probably SVG style nodes too. I'm undecided about <?xml-stylesheet?> processing instructions at this point, though. > Should really do `*aTriggeringPrincipal = nullptr;`. I tend to assume that everyone should be using `getter_AddRefs` (which guarantees the pointer value is null before the call) for ref-counted out params, or they're doing something wrong. But I suppose it can't hurt to be defensive.
Assignee | ||
Comment 47•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8915829 [details] Bug 1406278: Part 5a - Use subject principal as triggering principal in <audio>/<video> "src" attribute. https://reviewboard.mozilla.org/r/187050/#review192486 > aValue is null on attr removal. We may need tests for the removal case here and elsewhere, unless it's already tested. It's probably already tested, but I didn't do a full try run after I made this change, just a few targetted tests. I'll add null checks to all of these.
Comment 48•7 years ago
|
||
> I was planning to handle capturing the triggering principal for HTML <style> nodes In what sense? Those don't do a load. Or did you mean a principal for the sheet that's different from the document itself? I think storing that on the nsStyleLinkElement in the same member as the <link> thing, but with different semantics, would be a bit odd. > I'm undecided about <?xml-stylesheet?> processing instructions at this point, though. Those would be the one reason to have this on the superclass.
Assignee | ||
Comment 49•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #48) > > I was planning to handle capturing the triggering principal for HTML <style> nodes > > In what sense? Those don't do a load. Or did you mean a principal for the > sheet that's different from the document itself? I think storing that on > the nsStyleLinkElement in the same member as the <link> thing, but with > different semantics, would be a bit odd. They don't do a load, but we still need the principal that created them for two reasons: 1) So that they can be loaded into sites with CSPs that would otherwise prohibit inline styles. 2) So that we can use it as the triggering principal for content loaded by the sheet. I agree that it's a bit odd, but nsStyleLinkElement is already pretty overloaded to deal with either inline or remote stylesheets, but not both. And GetStyleSheetURL already returns an inline and principal out param. It seems to make sense that for inline sheets, it would return the principal of the sheet, and from the same member.
Assignee | ||
Comment 50•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #49) > They don't do a load, but we still need the principal that created them for > two reasons: > > 1) So that they can be loaded into sites with CSPs that would otherwise > prohibit inline styles. > 2) So that we can use it as the triggering principal for content loaded by > the sheet. (Essentially, I've been thinking of the inline case as a load that inherits its triggering principal)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 61•7 years ago
|
||
mozreview-review |
Comment on attachment 8915825 [details] Bug 1406278: Part 2b - Use subject principal as triggering principal in <img> "src" attribute. https://reviewboard.mozilla.org/r/187040/#review192884 r=me on the changes to the absolute url bits. ::: dom/base/nsContentUtils.h:629 (Diff revisions 2 - 3) > > /** > + * Returns true if the given string is guaranteed to be treated as an absolute > + * URL, rather than a relative URL. In practice, this means any complete URL > + * as supported by nsStandardURL, or any string beginning with a valid scheme > + * which is known to the IO service, and has the URI_NORELATIVE flag. Maybe document that in corner cases where a url may end up being treated as relative or absolute depending on circumstances this function will return false? ::: dom/base/nsContentUtils.cpp:2377 (Diff revisions 2 - 3) > + // so no need to check with the IO service. > + if (net_IsAbsoluteURL(aURL)) { > + return true; > + } > + > + nsCOMPtr<nsIIOService> ios = services::GetIOService(); Just use nsContentUtils::GetIOService() or sIOService directly.
Attachment #8915825 -
Flags: review?(bzbarsky) → review+
Comment 62•7 years ago
|
||
> but we still need the principal that created them for two reasons
Ah, fair. OK.
Assignee | ||
Comment 63•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf42aab381823d6595cee406c3ba77e934568dd4 Bug 1406278: Part 1 - Pass subject principal to SetAttribute and friends. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/a679728a98e9e3ab8dde8f839ff0baaa8628dd4c Bug 1406278: Part 2a - Rename LoadingPrincipal to TriggeringPrincipal in imgLoader. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/9fca7bef9dfe04ab94ef14021dd14a76721af651 Bug 1406278: Part 2b - Use subject principal as triggering principal in <img> "src" attribute. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/cf763570260af394cd7375a287954bb2c091236f Bug 1406278: Part 2c - Use subject principal as triggering principal in <img> "srcset" attribute. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/cf383eff981f365975f777158e5e5cf6e388d32c Bug 1406278: Part 3 - Use subject principal as triggering principal in <script> "src" attribute. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7305ab6283ee4bd9ebf2a352c276b72c93b269 Bug 1406278: Part 4 - Use subject principal as triggering principal in <iframe>/<frame> "src" attribute r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/e894f7b9168b33eebda126c7b4b431a833807710 Bug 1406278: Part 5a - Use subject principal as triggering principal in <audio>/<video> "src" attribute. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4afd1e97b7856b0ff604fb7b867d555cecd40c Bug 1406278: Part 5b - Use subject principal as triggering principal in <source> "src" attribute for <audio>/<video>. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/19b365ebc32cd5895080ab0a572fc0cacdbcaefe Bug 1406278: Part 6 - Use subject principal as triggering principal in <source> "srcset" attribute for <picture>. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/1784e206cc103a87b366b83f7ce6bf9c1d4ade9f Bug 1406278: Part 7 - Use subject principal as triggering principal in <input> "src" attribute. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/e4308fbed9c584d58ef67693ba4797680e6d6669 Bug 1406278: Part 8a - Cleanup some uses of CallQueryInterface. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0c2767cf57ed606e9bed54a15d6bdadae12579 Bug 1406278: Part 8b - Use subject principal as triggering principal in style <link> "href" attribute. r=bz
Comment 64•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf42aab38182 https://hg.mozilla.org/mozilla-central/rev/a679728a98e9 https://hg.mozilla.org/mozilla-central/rev/9fca7bef9dfe https://hg.mozilla.org/mozilla-central/rev/cf763570260a https://hg.mozilla.org/mozilla-central/rev/cf383eff981f https://hg.mozilla.org/mozilla-central/rev/8e7305ab6283 https://hg.mozilla.org/mozilla-central/rev/e894f7b9168b https://hg.mozilla.org/mozilla-central/rev/ac4afd1e97b7 https://hg.mozilla.org/mozilla-central/rev/19b365ebc32c https://hg.mozilla.org/mozilla-central/rev/1784e206cc10 https://hg.mozilla.org/mozilla-central/rev/e4308fbed9c5 https://hg.mozilla.org/mozilla-central/rev/ec0c2767cf57
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•