Closed
Bug 1496034
Opened 2 years ago
Closed 2 years ago
Apply bz's comment to FeaturePolicy
Categories
(Core :: DOM: Security, enhancement)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
Details
(Whiteboard: [domsecurity-backlog1] [domsecurity-active][wptsync upstream])
Attachments
(1 file, 2 obsolete files)
94.36 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•2 years ago
|
||
Here the main changes: 1. nsIPrincipal instead of nsString for origin 2. mFeaturePolicy is always created in HTMLIFrameElement in order to avoid pref changes. We do something similar in nsIDocument. 3. mFeaturePolicy is traversed/unlinked by HTMLIFrameElement and not its parent class. 4. NullPrincipal() is used for opaque origin, instead of an empty string. 5. WhiteList renamed AllowList to match the spec syntax. 6. comments
Attachment #9013970 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•2 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog1] [domsecurity-active]
![]() |
||
Comment 2•2 years ago
|
||
Comment on attachment 9013970 [details] [diff] [review] comments.patch >+++ b/dom/base/nsDocument.cpp The documentation I asked for in nsIDocument::Policy is not there. >+++ b/dom/html/HTMLIFrameElement.cpp >+ // We always need a featurePolicy, also if not exposed. "even if not exposed" >+ mFeaturePolicy = new FeaturePolicy(this); This is going to refcount us in the constructor... probably OK, I guess, since we have no subclasses. >+HTMLIFrameElement::GetFeaturePolicyDefaultOrigin(nsIPrincipal** aPrincipal) const This function still needs documentation explaining what it's trying to compute, in spec terms. Not having that makes it hard to review, esp in edge cases. For example, why do we want to create a separate nullprincipal in the sandboxed case instead of still using our document's principal? Totally unclear. Needs either spec references or documentation. >+ if (!GetURIAttr(nsGkAtoms::src, nullptr, getter_AddRefs(nodeURI))) { We should just document that if false is returned nodeURI will be null. Also that nodeURI can be null even if true is returned. >+ nsresult rv = nsContentUtils::GetUTFOrigin(nodeURI, origin); "origin" is unused after this. Why is this call here at all? Please document or remove. I'm a little unclear on how this stuff works for various edge cases of iframes, both in the spec and in our impl. Some obvious questions: 1) What happens with an <iframe srcdoc> that uses 'self' in its policy? 2) What happens with <iframe src="data:...">, where the frame doc will have a nullprincipal but we will end up with a codebase principal, I think, from GetFeaturePolicyDefaultOrigin? Will 'self' work? Should it, per spec? > HTMLIFrameElement::RefreshFeaturePolicy() > if (OwnerDoc()->GetSandboxFlags() ^ SANDBOXED_ORIGIN) { This still looks wrong to me, and if correct still needs a clear explanation for why it's done this way. >+++ b/dom/html/nsGenericHTMLFrameElement.cpp OK, so we no longer CC mFeaturePolicy from this class, but the member and getter still live on this class? Shouldn't those be on HTMLIFrameElement too? >+++ b/dom/security/featurepolicy/Feature.cpp >+ if (BasePrincipal::Cast(principal)->FastEquals(aPrincipal)) { Spec has a "same origin-domain" check, no? Or is this not the check that the spec is doing? >+ return BasePrincipal::Cast(mDefaultOrigin)->FastEquals(aOrigin); Here too? >+++ b/dom/security/featurepolicy/FeaturePolicyUtils.h >+ // Feature allowed of the document's origin. Not quite sure what this means. Maybe "Feature allowed for documents that are same-origin with this one" or something? >+ // Iterator. Sure, but what do the arguments to the callback mean, for example? It worries me that switching from strings to principals did not affect any test results. The obvious conclusion is that test coverage is poor. Could we add some tests that get fixed by these changes, please?
Attachment #9013970 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 3•2 years ago
|
||
> 1) What happens with an <iframe srcdoc> that uses 'self' in its policy? In this case, 'src' attribute is not set and the parent document's origin will be used. I'll add a test for this. > 2) What happens with <iframe src="data:...">, where the frame doc will have > a nullprincipal but we will end up with a codebase principal, I think, from > GetFeaturePolicyDefaultOrigin? Will 'self' work? Should it, per spec? There are 3 FeaturePolicy objects involved here: 1. the parent document's policy - origin: document's origin. 2. the iframe policy - origin: data URL's origin. 3. the iframe document policy - origin: nullprincipal. only '*' features should be allowed here. in my interpretation of the spec, data: URL should be able to allow only features marked as '*'. I'll add a test here as well.
Assignee | ||
Comment 4•2 years ago
|
||
I wrote some WPTs to check sandboxed, srcdoc, data: URL iframes. They seem to behave fine with this code, if I read the spec correctly.
Attachment #9013970 -
Attachment is obsolete: true
Attachment #9014413 -
Flags: review?(bzbarsky)
![]() |
||
Comment 5•2 years ago
|
||
> In this case, 'src' attribute is not set Why? Someone can write: <iframe srcdoc="foo" src="bar"> and that will ignore src and render the srcdoc. What does the spec say to do with feature policy in this case? What does our implementation do? We should test both cases, with and without src, here. And we should test dynamic changes (e.g. srcdoc being set dynamically on an iframe with existing loaded src and src being set dynamically on an iframe with existing loaded srcdoc; the latter case should be a complete no-op). > in my interpretation of the spec, data: URL should be able to allow only features marked as '*'. OK, but is that reasonable behavior? I guess blob: will work ok because we can in fact create a useful codebase principal from a blob url?
Assignee | ||
Comment 6•2 years ago
|
||
> Why? Someone can write: <iframe srcdoc="foo" src="bar"> and that will > ignore src and render the srcdoc. What does the spec say to do with feature > policy in this case? What does our implementation do? The spec says nothing about srcdoc. I filed a spec bug: https://github.com/WICG/feature-policy/issues/223 > We should test both cases, with and without src, here. And we should test > dynamic changes (e.g. srcdoc being set dynamically on an iframe with > existing loaded src and src being set dynamically on an iframe with > existing loaded srcdoc; the latter case should be a complete no-op). Ok. > > in my interpretation of the spec, data: URL should be able to allow only features marked as '*'. > > OK, but is that reasonable behavior? data: URLs are considered cross-origin in general. I think this is the right behavior. > I guess blob: will work ok because we can in fact create a useful codebase > principal from a blob url? Right.
QA Contact: ckerschb
Assignee | ||
Comment 7•2 years ago
|
||
Attachment #9014413 -
Attachment is obsolete: true
Attachment #9014413 -
Flags: review?(bzbarsky)
Attachment #9014724 -
Flags: review?(bzbarsky)
Updated•2 years ago
|
QA Contact: ckerschb
![]() |
||
Comment 8•2 years ago
|
||
Comment on attachment 9014724 [details] [diff] [review] comments.patch >+++ b/dom/base/nsDocument.cpp > nsIDocument::Policy() const > { > MOZ_ASSERT(StaticPrefs::dom_security_featurePolicy_enabled()); Note that this assert will fail if the pref is changed after initial document setup. I don't think we should be asserting this. >+++ b/dom/html/HTMLIFrameElement.cpp >+HTMLIFrameElement::GetFeaturePolicyDefaultOrigin(nsIPrincipal** aPrincipal) const The docs don't match the implemenentation in the srcdoc case. The function now always returns NS_OK. Why do we need the nsresult return type at all? Why not return already_AddRefed<nsIPrincipal>? You could then take early returns in special cases like srcdoc. Do we have tests that would catch the behavior change from us no longer checking sandboxing flags here? > data: URLs are considered cross-origin in general. I think this is the right behavior. Sure, but why shouldn't 'self' be the nonce origin for the data: case? Anyway, this seems like a spec issue that we should just file. Do we have a test for us no longer checking sandboxing flags here? >+++ b/dom/security/featurepolicy/Feature.cpp >+Feature::AllowListContains(nsIPrincipal* aPrincipal) const >+ if (BasePrincipal::Cast(principal)->Subsumes(aPrincipal, >+ BasePrincipal::DontConsiderDocumentDomain)) { The spec _does_ consider document.domain, doesn't it? Again, there should be tests for this stuff... >+++ b/dom/security/featurepolicy/FeaturePolicy.cpp > FeaturePolicy::AllowsFeatureInternal(const nsAString& aFeatureName, >+ return BasePrincipal::Cast(mDefaultOrigin)->Subsumes(aOrigin, >+ BasePrincipal::DontConsiderDocumentDomain); Again, I think the spec considers document.domain... Again, tests?
Attachment #9014724 -
Flags: review?(bzbarsky) → review-
![]() |
||
Comment 9•2 years ago
|
||
Oh, and in general this is looking pretty good! Thank you for the expanded test coverage!
![]() |
||
Comment 10•2 years ago
|
||
Comment on attachment 9014724 [details] [diff] [review] comments.patch Just talked about this with Andrea on IRC. We agreed to get this landed with all but the document.domain comments addressed and file followups for the document.domain bits and adding more test coverage and so forth. r+ with the other bits addressed.
Attachment #9014724 -
Flags: review- → review+
Comment 11•2 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/402b42c38c70 Apply bz's comments to FeaturePolicy, r=bz
Comment 12•2 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa33c65b6222 Remove FeaturePolicy assertion in getter methods, r=bz
Comment 13•2 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6fe44fb6369 Improve HTMLIFrameElement::GetFeaturePolicyDefaultOrigin, r=bz
Comment 14•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/402b42c38c70 https://hg.mozilla.org/mozilla-central/rev/fa33c65b6222 https://hg.mozilla.org/mozilla-central/rev/a6fe44fb6369
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13491 for changes under testing/web-platform/tests
Whiteboard: [domsecurity-backlog1] [domsecurity-active] → [domsecurity-backlog1] [domsecurity-active][wptsync upstream]
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/13491 * continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/440777378?utm_source=github_status&utm_medium=notification)
Whiteboard: [domsecurity-backlog1] [domsecurity-active][wptsync upstream] → [domsecurity-backlog1] [domsecurity-active][wptsync upstream error]
Whiteboard: [domsecurity-backlog1] [domsecurity-active][wptsync upstream error] → [domsecurity-backlog1] [domsecurity-active][wptsync upstream]
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•