Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

"Content-type" header is passed directly into result #38

Closed
neftaly opened this issue Feb 22, 2015 · 9 comments · Fixed by #47
Closed

"Content-type" header is passed directly into result #38

neftaly opened this issue Feb 22, 2015 · 9 comments · Fixed by #47

Comments

@neftaly
Copy link
Contributor

neftaly commented Feb 22, 2015

We need to parse the content-type header before generating the SRI attribute.

request.result = {
    'content-type': 'text/javascript; charset=UTF-8'
   ...
}
sri.generateHash (request.result);
//=> "<script src="file.js" integrity="type:text/javascript; charset=UTF-8 sha256-..."></script>"

This is a security issue; both XSS and the ability to spoof hashes.

request.result = {
    'content-type': 'text/javascript sha512-fAkEhAsH'
   ...
}
sri.generateHash (request.result);
//=> "<script src="file.js" integrity="type:text/javascript sha512-fAkEhAsH sha256-rEaLhAsH"></script>"

I'm mad at myself for not noticing this earlier.

@neftaly neftaly added the bug label Feb 22, 2015
@neftaly neftaly added this to the Initial public release milestone Feb 22, 2015
@neftaly
Copy link
Contributor Author

neftaly commented Feb 22, 2015

Can you include options (i.e. encoding) in the "type" definition of the current spec?

@fmarier
Copy link
Contributor

fmarier commented Feb 22, 2015

I guess we need to:

  1. split the string on semicolons (;)
  2. only use the first string returned in the first step and discard all others
  3. split the string on slash (/)
  4. ensure that both sides match [A-Za-z0-9_.-]+ (or better yet, use a built-in validator in node or hapi if they exist)

@fmarier fmarier self-assigned this Feb 22, 2015
@neftaly
Copy link
Contributor Author

neftaly commented Feb 22, 2015

I would force string termination after anything that looked like a delimiter to the browser SRI interpreter (your regex...).

@fmarier
Copy link
Contributor

fmarier commented Feb 22, 2015

Yeah, at the very least, we should have a blacklist to detect spaces and closing quotes, but whitelist-based approaches (only look at what you expect) are generally better.

@neftaly
Copy link
Contributor Author

neftaly commented Feb 23, 2015

Just a note, I plan to audit every I/O in the XHR handler, to ensure that dodgy values can't be injected elsewhere.

@neftaly
Copy link
Contributor Author

neftaly commented Feb 25, 2015

Regarding the spec, is there a reason "type:" is included at all? Now that it's not a per-hash ni-URI parameter, I don't see any actual use for it that can't already be handled by the HTML type attribute.

@fmarier
Copy link
Contributor

fmarier commented Feb 25, 2015

I'm not an expert on the HTML type attribute but I believe it's for a different purpose. I think it's for telling the browser "treat this resource as this content type regardless of what the server says it is". SRI on the other hand says "block this resource unless it comes with the right content-type header from the server".

@neftaly
Copy link
Contributor Author

neftaly commented Feb 25, 2015

I can't find the issue (was originally pre-CSP2-style-change IIRC), but I remember reading somewhere that the reason for content-type matching was to prevent something like an application/javascript file with some hidden VBS being parsed as application/vbscript, at the servers -- not the users -- behest.

Wouldn't this do the same thing, whilst simplifying the spec, and removing any ambiguity? My concern is that ni-URIs provided a hammer to solve a problem that could now be fixed with a spanner.

I know this is the wrong place to ask, but w3c/webappsec is imposing, and I'm unsure of the etiquette :)

@neftaly
Copy link
Contributor Author

neftaly commented Feb 25, 2015

Nevermind, I'm blind! I'll explain a little further in a new srihash.org issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants