Skip to content
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

Apply integrity checks to inline script and style blocks. #86

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mikewest
Copy link
Member

@mikewest mikewest commented Dec 16, 2019

Closes #44.

If this looks like a reasonable approach, I'll put up patches to HTML and CSP as well, and try to find someone to fix Chromium's implementation.

/cc @annevk @mozfreddyb @devd @fmarier @metromoxie


Preview | Diff

@mozfreddyb
Copy link
Collaborator

Makes sense.

IMHO this should have an informal note / redirection that CSP modified the allow behavior depending on policy.

Did you file an issue in CSP to write down the blocking of correct but disallowed (absent from policy) metadata?

@mikewest
Copy link
Member Author

IMHO this should have an informal note / redirection that CSP modified the allow behavior depending on policy.

CSP didn't modify SRI's behavior at all, AFAIR. It layers on top of SRI, imposing some additional restrictions prior to SRI's enforcement. (Or is that what you mean?)

Did you file an issue in CSP to write down the blocking of correct but disallowed (absent from policy) metadata?

As currently defined, it fails the bits in step 1.3 of https://w3c.github.io/webappsec-csp/#script-pre-request, which happens during Fetch, prior to SRI enforcement. So we'd expect a CSP error failing the request rather than an SRI error. I'm pretty sure that's what Chrome's console reports, and that we fire a securitypolicyviolation event.

Is there more that CSP should do here?

@mozfreddyb
Copy link
Collaborator

IMHO this should have an informal note / redirection that CSP modified the allow behavior depending on policy.

CSP didn't modify SRI's behavior at all, AFAIR. It layers on top of SRI, imposing some additional restrictions prior to SRI's enforcement. (Or is that what you mean?)

Yes. SRI will allow, but the resource matching the integrity attribute still has to pass a CSP (if existant). Maybe that's not noteworthy in itself. But what's layered on top of what isn't immediately obvious to developers, is it? I'm not sure how to make the order in which things are processed (layered) more explicit.

Did you file an issue in CSP to write down the blocking of correct but disallowed (absent from policy) metadata?

As currently defined, it fails the bits in step 1.3 of https://w3c.github.io/webappsec-csp/#script-pre-request, which happens during Fetch, prior to SRI enforcement. So we'd expect a CSP error failing the request rather than an SRI error. I'm pretty sure that's what Chrome's console reports, and that we fire a securitypolicyviolation event.

Is there more that CSP should do here?

No, that makes sense. Nevermind then.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case for this feature?

this algorithm after the CSP check in step 13.

ISSUE: This needs to be wired up to HTML's [=update a style block=] algorithm by inserting a call
to this algorithm after the CSP check in step 5.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you'll also need to define it as a valid attribute for the style element if this is actually a thing we want to encourage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I thought we'd done that already, but of course it's only valid for <link>.

jonathanKingston pushed a commit to jonathanKingston/webappsec-subresource-integrity that referenced this pull request Jan 4, 2020
@devd devd closed this in #90 Jan 7, 2020
devd pushed a commit that referenced this pull request Jan 7, 2020
@annevk
Copy link
Member

annevk commented Jan 7, 2020

This was not fixed by that PR.

@annevk annevk reopened this Jan 7, 2020
@mikewest
Copy link
Member Author

mikewest commented Jan 7, 2020

What's the use case for this feature?

Inline integrity information can provide some defense in depth against injection attacks (as an injection into a script block wouldn't automatically mean code execution if the block had an integrity attribute; you'd need another injection point). This protection can be enhanced via CSP's existing support for listing the set of hashes which are allowed to execute on a given page, but it seems valuable in itself.

Adding this support might also allow us to more cleanly layer CSP's behavior on top of SRI for inline blocks, just as we have for externalized scripts. That isn't user-facing at all, but might clean up the spec a bit.

There was some discussion of/agreement on this in https://www.w3.org/2016/05/16-webappsec-minutes.html#item02, and on and off since then.

@annevk
Copy link
Member

annevk commented Jan 7, 2020

And the injection point would filter HTML somehow and only allow injection of content already covered by the integrity attribute value? Especially that last part seemed rather curious to me, but maybe it's valid in certain complicated setups.

@mikewest
Copy link
Member Author

mikewest commented Jan 7, 2020

And the injection point would filter HTML somehow and only allow injection of content already covered by the integrity attribute value? Especially that last part seemed rather curious to me, but maybe it's valid in certain complicated setups.

I admit that the use case from a security perspective is somewhat contrived (@arturjanc might have actual data here from Google's products). It assumes an injection point like let name = "{{user data goes here}}";, and that developers are going to be better about filtering </script><script> from user input than "; alert(1) //.

Apple had performance reasons in the minutes linked above (that are unfortunately a bit difficult to tease out of that record), and I think there are spec-sanity reasons to do so (I find it somewhat confusing that CSP can allow inline script by hash, but that it does so completely independently of SRI).

Given that Chrome's shipping implementation is already halfway here, I'd prefer to simply make that work the way outlined here than remove it.

@annevk
Copy link
Member

annevk commented Jan 7, 2020

The thing I wanted to point is that the integrity value would have to match the eventual {{user data goes here}} value, right? At which point you'd likely compute the integrity value based on that, at which point it's unsafe.

@mikewest
Copy link
Member Author

mikewest commented Jan 7, 2020

Yes. If a developer is parsing the data on the server and generating an integrity attribute on the fly, then all bets are off.

An alternative way of looking at that is that calculating a hash to match the eventual {{user data goes here}} would be hard, and tagging script blocks with integrity information would make it difficult for future developers to develop insecure practices. Security through annoyance. :)

@annevk
Copy link
Member

annevk commented Jan 8, 2020

For my understanding, the expected usage pattern is that {{user data goes here}} is a finite number of different strings and you'd fill the integrity attribute on the surrounding script element with an equal number of hashes?

@arturjanc
Copy link

I'm likely missing some context here, but I'm also not entirely clear on how this could be used as an injection defense:

  • If an inline script has no variable/interpolated parts then developers could calculate and add the integrity attribute, but it would be a no-op because no injection is possible in this case.
  • If an inline script has variable parts, then in order to set the correct integrity attribute the server would have to dynamically calculate its value after interpolation, which -- if there is an injection -- would include the attacker's injected string, allowing their injection to execute.

The way I see this potentially working is if there was an integrity attribute that covered the parse tree of the script, but not its full contents. Then, if I have something like <script>var foo = "${foo}" </script> in my HTML templates, I could calculate the hash of the structure of the script ahead of time and expect that the hash remains the same regardless of the contents of $foo. (And if there is an injection, the structure of the script would change and then the signature wouldn't match.)

Alas, that's not really what SRI does... :)

@ghost
Copy link

ghost commented Feb 14, 2020

As arturjanc stated, maybe I'm missing some context here... but how exactly does this replace the functionality of require-sri-for? I ask because sub-resource integrity checking via CSP is not something that will go quietly into the night. While there is plenty of mixed information regarding the status of require-sri-for including within the spec itself - the current editor's draft @ https://w3c.github.io/webappsec-subresource-integrity/ (Jan 4 2020) still acknowledges its existence / living status in the current CSP specs as of that draft, which is 4 years ahead of the "current" spec. While the concept for inline integrity via hashes - just like using them in the head/s of HTML which is current practice using, e.g. <style sha256="sha256-hV6tTXtg/3ifbDe......=>p{font:xxx;}</style> , is one that will certainly be helpful, is this meant as the require-sri-for replacement in CSP? If not, what is? Simply abandoning it is not a "thing" that should be done. Moreover, it's not good security practice.

Base automatically changed from master to main February 16, 2021 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend SRI to support integrity metadata on inline script/style blocks
4 participants