-
Notifications
You must be signed in to change notification settings - Fork 672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[css-values] Security concerns regarding attr() #5092
Comments
Please note that @mikewest explicitly referred to the known concerns about third-party CSS in the intent thread and explained how the proposed expansion in expressive power of One specific problematic case are sites which allow user-controlled
If Note that this is just one example. Another consideration is the fact that many websites' Content Security Policy rules are more lax when it comes to permitting styles than scripts, so making CSS-based exfiltration of DOM contents easier will allow bypassing existing CSPs. From a security perspective, I'd strongly favor allowlisting attributes permitted in |
Above links are all 404 for me - the discussion appears to be the one at https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/FGCgsKmylhw |
It's not limited to
It's not implemented for (non-pseudo) elements yet in any UAs as far as I know, but at least Gecko supports it on I don't think that changes the security aspects though, since it would still be considered generated content; same as for pseudos. |
Thanks, @faceless2 -- I updated the link in my reply above. |
div {
content: url(file.png);
} is valid CSS, and already works on Gecko and Blink (the distinction is between "content-replacement" and "content-list" in the definition of the "content" property - the former (which maps to But as div::before {
content: url(attr(secret));
content: attr(secret url);
} are presumably the kind of cases causing concern in the original posts. Both should be valid according to the spec, but this syntax doesn't work in any browser yet. As there's no ability to concatenate in CSS, these would become a relative URL resulting in a request to the server that supplied the HTML (or, perhaps, the stylesheet, depending on the outcome of #5079). So you couldn't send a request just anywhere. Personally I've no opinions on what to do about this, other to say that |
Does |
@Crissov It's the attribute in the current DOM, which can come from original HTML source or set in JS. But e.g. if you change the |
Re @arturjanc
There's a compatibility concern, since How about disallowing |
That's a great illustration of the security issue this feature runs into: currently, browsers (at least Chrome and Firefox) resolve relative URLs based on the location of the stylesheet, not the loading document. So any CSS injection could just use Currently, attackers can still query against the contents of attributes with CSS3 selectors, but that's a single-bit information leak, requiring thousands of requests (and recursively adding new stylesheets to the page) to exfiltrate something like a CSRF token. This makes such attacks more difficult to conduct in practice, and makes them more time-consuming, increasing the chance that a user would navigate away from the page before the attacker can leak the secret. Giving I don't think disallowing this on certain elements is sufficient, given the large amount of data modern applications have in |
So I think #5079 should be resolved how Anne suggests, which would conveniently also remove the ability to exfiltrate arbitrary data to an arbitrary origin via However, when we introduce a string concatenation function, and a I think that, separately, we should pursue what Mike West suggested in the ItS thread, and block certain sensitive attributes from being visible to CSS at all - |
There's no way to pull an attribute value from an element other than the one you're styling, and I've been unable to apply any sort of style to I presume this was by design, but I'm not sure where it's specified. |
You can absolutely display script and style; you might be overlooking that |
And "hide sensitive attributes from CSS entirely" is now #5136. |
I was about to reply - confidently - that I didn't put them in the head, until I recalled I was testing them as with an online thingy like codepen. So now I'm not so confident :-) Please ignore my previous. |
I like the idea from #5136, let's continue the discussion about restricting CSS attribute access there. The one thing I'd like to stress here is that exfiltration via Basically, I'm worried that there's a fairly large set of existing CSS features that would enable leaks in existing applications when |
For other folks interested in this issue, we recently had a discussion about a safe path forward for As a brief summary, IMHO preventing the use of |
The opt-in sounds pretty good to me. Should this be very strictly on the referenced element itself, or would it be okay to put it on an ancestor and have it apply to an entire subtree (and, if placed on html, the entire document)? Tons of attribute repetition isn't great if the referenced elements occur a bunch on the page. |
Note that it’s not possible to use any CSS function inside |
Good point, but the concerns still apply to |
There's probably no single right answer here. There's a risk that developers will add the attribute on html and accidentally allow CSS access to sensitive values throughout the DOM (it's difficult to reason about this because it requires understanding the meaning of every value in every attribute in the DOM). At the same time, this would be safe by default, i.e. only applications that add the attribute would allow such access, and we could add a spec/documentation note explaning the considerations. Personally, I'd be okay with inheriting this bit from an ancestor element given that this design wouldn't result in a security regression for existing applications (at most, it's a sharp edge that developers would need to take into account when adding the attribute). |
Support attr()'s new syntax function according to CSS Values 5 spec [0]: attr() = attr( <attr-name> <attr-type>? , <declaration-value>?) This CL only implements the basic attr() support and does not include security concerns handling, described in [1]. [0] https://drafts.csswg.org/css-values-5/#attr-notation [1] w3c/csswg-drafts#5092 Bug: 40320391 Change-Id: I6703ea6a6e59cec7579dce0df6e411de238361f6
Support attr()'s new syntax function according to CSS Values 5 spec [0]: attr() = attr( <attr-name> <attr-type>? , <declaration-value>?) This CL only implements the basic attr() support and does not include security concerns handling, described in [1]. Android Binary Size (arm64 high end) (TrichromeLibrary64.apk) has increased by 72,820 bytes with this commit, see detailed SuperSize HTML Diff [2]. This is unavoidable change to support attr()'as extended capabilities. [0] https://drafts.csswg.org/css-values-5/#attr-notation [1] w3c/csswg-drafts#5092 [2] https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchrome-supersize%2Foneoffs%2F5d767292566fc8a703add425aec266384201f481_0c795997696df0aefa7166aadf631be05995aac3.sizediff Bug: 40320391 Change-Id: I6703ea6a6e59cec7579dce0df6e411de238361f6 Binary-Size: See commit description.
Support attr()'s new syntax function according to CSS Values 5 spec [0]: attr() = attr( <attr-name> <attr-type>? , <declaration-value>?) This CL only implements the basic attr() support and does not include security concerns handling, described in [1]. Android Binary Size (arm64 high end) (TrichromeLibrary64.apk) has increased by 72,820 bytes with this commit, see detailed SuperSize HTML Diff [2]. This is unavoidable change to support attr()'as extended capabilities. [0] https://drafts.csswg.org/css-values-5/#attr-notation [1] w3c/csswg-drafts#5092 [2] https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchrome-supersize%2Foneoffs%2F5d767292566fc8a703add425aec266384201f481_0c795997696df0aefa7166aadf631be05995aac3.sizediff Bug: 40320391 Change-Id: I6703ea6a6e59cec7579dce0df6e411de238361f6 Binary-Size: See commit description. Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5660057 Reviewed-by: Anders Hartvoll Ruud <[email protected]> Commit-Queue: Munira Tursunova <[email protected]> Cr-Commit-Position: refs/heads/main@{#1323782}
Support attr()'s new syntax function according to CSS Values 5 spec [0]: attr() = attr( <attr-name> <attr-type>? , <declaration-value>?) This CL only implements the basic attr() support and does not include security concerns handling, described in [1]. Android Binary Size (arm64 high end) (TrichromeLibrary64.apk) has increased by 72,820 bytes with this commit, see detailed SuperSize HTML Diff [2]. This is unavoidable change to support attr()'as extended capabilities. [0] https://drafts.csswg.org/css-values-5/#attr-notation [1] w3c/csswg-drafts#5092 [2] https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchrome-supersize%2Foneoffs%2F5d767292566fc8a703add425aec266384201f481_0c795997696df0aefa7166aadf631be05995aac3.sizediff Bug: 40320391 Change-Id: I6703ea6a6e59cec7579dce0df6e411de238361f6 Binary-Size: See commit description. Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5660057 Reviewed-by: Anders Hartvoll Ruud <[email protected]> Commit-Queue: Munira Tursunova <[email protected]> Cr-Commit-Position: refs/heads/main@{#1323782}
Automatic update from web-platform-tests Support new syntax of attr() Support attr()'s new syntax function according to CSS Values 5 spec [0]: attr() = attr( <attr-name> <attr-type>? , <declaration-value>?) This CL only implements the basic attr() support and does not include security concerns handling, described in [1]. Android Binary Size (arm64 high end) (TrichromeLibrary64.apk) has increased by 72,820 bytes with this commit, see detailed SuperSize HTML Diff [2]. This is unavoidable change to support attr()'as extended capabilities. [0] https://drafts.csswg.org/css-values-5/#attr-notation [1] w3c/csswg-drafts#5092 [2] https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchrome-supersize%2Foneoffs%2F5d767292566fc8a703add425aec266384201f481_0c795997696df0aefa7166aadf631be05995aac3.sizediff Bug: 40320391 Change-Id: I6703ea6a6e59cec7579dce0df6e411de238361f6 Binary-Size: See commit description. Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5660057 Reviewed-by: Anders Hartvoll Ruud <[email protected]> Commit-Queue: Munira Tursunova <[email protected]> Cr-Commit-Position: refs/heads/main@{#1323782} -- wpt-commits: 29c9f52f5ab65d0c55638b2caa5d128876d6ad56 wpt-pr: 47001
Automatic update from web-platform-tests Support new syntax of attr() Support attr()'s new syntax function according to CSS Values 5 spec [0]: attr() = attr( <attr-name> <attr-type>? , <declaration-value>?) This CL only implements the basic attr() support and does not include security concerns handling, described in [1]. Android Binary Size (arm64 high end) (TrichromeLibrary64.apk) has increased by 72,820 bytes with this commit, see detailed SuperSize HTML Diff [2]. This is unavoidable change to support attr()'as extended capabilities. [0] https://drafts.csswg.org/css-values-5/#attr-notation [1] w3c/csswg-drafts#5092 [2] https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchrome-supersize%2Foneoffs%2F5d767292566fc8a703add425aec266384201f481_0c795997696df0aefa7166aadf631be05995aac3.sizediff Bug: 40320391 Change-Id: I6703ea6a6e59cec7579dce0df6e411de238361f6 Binary-Size: See commit description. Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5660057 Reviewed-by: Anders Hartvoll Ruud <[email protected]> Commit-Queue: Munira Tursunova <[email protected]> Cr-Commit-Position: refs/heads/main@{#1323782} -- wpt-commits: 29c9f52f5ab65d0c55638b2caa5d128876d6ad56 wpt-pr: 47001
Support attr()'s new syntax function according to CSS Values 5 spec [0]: attr() = attr( <attr-name> <attr-type>? , <declaration-value>?) This CL only implements the basic attr() support and does not include security concerns handling, described in [1]. Android Binary Size (arm64 high end) (TrichromeLibrary64.apk) has increased by 72,820 bytes with this commit, see detailed SuperSize HTML Diff [2]. This is unavoidable change to support attr()'as extended capabilities. [0] https://drafts.csswg.org/css-values-5/#attr-notation [1] w3c/csswg-drafts#5092 [2] https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchrome-supersize%2Foneoffs%2F5d767292566fc8a703add425aec266384201f481_0c795997696df0aefa7166aadf631be05995aac3.sizediff Bug: 40320391 Change-Id: I6703ea6a6e59cec7579dce0df6e411de238361f6 Binary-Size: See commit description. Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5660057 Reviewed-by: Anders Hartvoll Ruud <[email protected]> Commit-Queue: Munira Tursunova <[email protected]> Cr-Commit-Position: refs/heads/main@{#1323782}
We have a prototype of the attr()-tainting proposal in Blink now. The proposal generally looks fine, and has proven to be implementable, so I suggest that we resolve to accept it, with two clarifications:
|
Ooh, okay, I was assuming that a per-token tainting was too complicated, but if instead it's easier, I'm definitely fine with that. |
The CSS Working Group just discussed
The full IRC log of that discussion<TabAtkins> on that note, I think the right behavior for scoped @Property is that *the host element* will refuse to inherit from its parent, if its shadow registers a custom property.<khush> moonira 2 open questions left for attr security. invalidation time and second is painting custom props <TabAtkins> q+ <khush> for the first we propose making security violation. if we have a grammar that allows doing type, that doesn't cause security issues we can't say for sure <khush> until after the substitution <khush> in the current spec, attr is already a computed value time. there's no substition value <khush> we just be consistent with security violation at computed value time <khush> if attr is subsitutued into custom prop value. affected part of custom prop become tainted, not the whole custom prop value <khush> moonira that's the proposal <astearns> ack TabAtkins <khush> TabAtkins i agree with both of these. url taining only causes invalid at computed value time, not parse time. makes it more diff to do that <khush> indivisual tainiting is amazing! at the moment, spec says entire value is tainted if any part has attr. i assumed it's easier. if doing it for individual tokens is possible, which means we can combine sec and not security sensitive tokens in the same prop. that's great! <khush> astearns 2 separate resolutions? <emilio> q+ <astearns> ack emilio <khush> emilio curious about how you do partial tainting? <khush> q+ <khush> if that token gets used for a security sensitive spot then it's invalid, color is fine <khush> q- <khush> emilio feels sketchy <khush> astearns we could resolve on what is proposed. and make it more strict? <khush> TabAtkins presence of attr taints the entire custom prop which keeps chaining <khush> emilio what is the computed value? does that contain attribute value? <khush> TabAtkins computed value is post substitution <khush> emilio i like the current spec more. i can see why it's more useful. <khush> emilio feels unfortunate to track token tainiting lacking clear use-cases <khush> TabAtkins both need to taint token, you need to tell that URL came from a tainted variable <khush> emilio if i were to implement this, you would mark img as tainted. <khush> the whole custom prop --img is tainted. rather than partial tainting <khush> feels simpler to spec, reason about <khush> TabAtkins on the spec side its only slightly simpler <khush> implementation wise also feels same, you have to care about token ranges <khush> one is example using bar and the other one is a URL <chrishtr> q+ <khush> emilio very curious about use-case, feels more complex than current spec and unwarranted <khush> iank_ i'll show you the chrome implementation <iank_> https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_attr_value_tainting.h (i think) <astearns> ack chrishtr <iank_> moonira is that correct? <khush> chrishtr the second part of the proposal, is the reason you suggested it is because it's more natural to implement <khush> moonira yes, it's easier to implement that way <khush> moonira it makes sense. the example in the issue, there it makes sense <khush> moonira not sure <khush> astearns what if we resolve on first part. and left the second part open so we could decide whether there is enough author justification <emilio> https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_attr_value_tainting.cc;l=20;drc=f522344e45882da4c7f7cb1b3a0a7bd747d654bb seems pretty hacky to me fwiw, I'd rather keep a tainted bit on the value you substitute around, but I haven't thought it through <khush> emilio +1 <khush> astearns PROPOSED RESOLUTION: change the spec such that violating attr security restriction will make it invalid at computed value time <khush> astearns for now the tainting whole custom prop stays as is <khush> can decide whether to make it partial later <khush> RESOLVED change the spec such that violating attr security restriction will make it invalid at computed value time <khush> RESOLVED: change the spec such that violating attr security restriction will make it invalid at computed value time |
No, it is definitely substantially more complicated. (May vary between implementations). However, per-value tainting means stuff will stop working just because you add indirection via a custom property:
No, it doesn't. But there seems to be agreement that it does. Let's clarify the spec that way, then. |
Edits checked in as 9fad470 |
This may have been the intended meaning of the spec text already, but I think we should clearly state the outcome of w3c#5092: partial tainting of custom properties was proposed in that issue, discussed, objected to, and therefore *not* accepted. This PR hopefully makes it clear that attr()-tainting applies to whole substitution values, for now.
This may have been the intended meaning of the spec text already, but I think we should clearly state the outcome of w3c#5092: partial tainting of custom properties was proposed in that issue, discussed, objected to, and therefore *not* accepted. This PR hopefully makes it clear that attr()-tainting applies to whole substitution values, for now.
This may have been the intended meaning of the spec text already, but I think we should clearly state the outcome of #5092: partial tainting of custom properties was proposed in that issue, discussed, objected to, and therefore *not* accepted. This PR hopefully makes it clear that attr()-tainting applies to whole substitution values, for now.
The CSS values spec basically says there's no security concerns:
In the Blink Intent to Implement and Ship: Advanced attr() thread, multiple concerns have been raised that
attr()
can be used as a tool for data exfiltration of sensitive data like passwords,nonce
, etc.And it's a much easier-to-use weapon compared to attribute selectors, which has to exfiltrate attribute value character-by-character in an iterative/recursive manner.
Other than "try harder to block CSS injection", do we have other ideas to address the security concerns? For example, blacklisting certain attributes (e.g.,
nonce
,value
, etc.), or even whitelisting attributes allowed inattr()
(as suggested by @mikewest here)?The text was updated successfully, but these errors were encountered: