Loading…
SRI: Clarify how to handle non-eligible and invalid metadata #317
9 participants
Just to comment regarding "implementationability", what I've suggested above is what I've implemented so far in Firefox.
An argument against failing closed for 2 and 3 of the cross-domain examples is that we'll be stuck with the CORS requirement even if we (somehow) succeed in making it optional (as per #338).
In other words, the forward-compatible approach is probably to fail open (i.e. load the script w/o checking integrity) in all 6 examples. Of course, devtools warnings would be appropriate.
I think it might make sense to fail open it all cases (including the CORS ineligible cases) where there isn't a straight integrity failure. This would also be consistent, as right now I can't really explain why we fail open in the syntax case but closed in the eligibility case. What do you all think?
Also, just as a note, we have test cases covering I think all of the discussed cases here, if you want to use them/check them out: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/
If we fail open in the CORS ineligible case, an attacker that controls the CDN can just disable CORS and then modify the JavaScript files. I strongly believe we need to fail close for the CORS ineligible case.
A cross origin fetch with integrity metadata should just fail because of missing cors, maybe?
@mozfreddyb: I don't think that attack is at issue. What you're describing would already fail today because the request was made with the crossorigin attribute (so it's a CORS enabled fetch), but the response doesn't include CORS headers, so the fetch would fail, and it wouldn't even make it to the integrity check.
The other case is that the developer forgot the crossorigin attribute, in which case the integrity can never be checked because no CORS check is ever done. In that case, isn't it safe to fail over since even if the response has CORS headers, the fetch wasn't CORS enabled, so it's not safe to do the integrity check?
@devd: I'm confused. Isn't what you described the status quo?
I also think there's a greater point here that @mozfreddyb was getting at. Errors caused by the client (such as syntax errors in the integrity attribute) should fail open, as it could be a future extension of SRI. However, errors from the server (e.g. missing CORS headers or mismatching content to the integrity) should fail closed as they could be an attack, since in our threat model the attacker has control of the server.
Yup, I was basically trying to say what Joel just said, just doing a pretty terrible job at it. Sorry about that
Haha. Well, glad you and I are in agreement :-)
should we close this with the PR that Joel just merged?
For the record, @mikewest is not the biggest fan of this change (see discussion on https://codereview.chromium.org/1166003004/). In particular, he's concerned that it will be confusing to developers, and they'll assume that adding the integrity attribute just "worked," even if we have a console message.
I also had the same concern, but the backwards compat argument won me over: we can always do something about it if we see that it is being misunderstood widely.
Yup, I just wanted to have that conversation put on the record over here.
I'm also not a fan, but I don't see how to get backwards compatibility without it :(
Well, it only makes this "backwards compatible" if we intend to go backwards :-) I think Mike's argument is that we won't; CORS is a requirement. I actually agree with him on that point, but I just don't think it's the consensus viewpoint, and I don't want my position to "win" because we locked ourselves into that position.
In any case, if everyone is sad about it, maybe we should think more deeply on it. I'm all ears!
Actually, re-reading that codereview more carefully, I like @mikewest's suggestion of automatically adding crossorigin="anonymous" for the author when it's missing.
Pros:
- SRI is enabled instead of silently (despite console messages) disabled
- the load is blocked when the server-side header is missing
- it's backwards-compatible
Cons:
- authors may not bother to add
crossorigin="anonymous"(so we should spam the devtool console)
Yes, that's extremely aggressive. You can make the argument that if we require crossorigin=anonymous now, and take away that requirement in the future, it's almost backwards compatible, in the sense that browsers update relatively quickly these days, and it will disappear within a few months.
But if we include crossorigin=anonymous automatically now, then if we change that requirement in the future, we're in big trouble, because we'll break deployed SRI (that didn't know about the CORS requirement, and just added the integrity attribute without crossorigin=anonymous). That would be a big surprise. So if we do that, there's really no backing out at that point.
Like I said, it seems unlikely to me that we'd back out of the CORS requirement in the future, so I don't think this is totally crazy, although I do find the idea of magically including a CORS request a bit surprising. I'd rather it just blocked and then made developers read the console and add the crossorigin=anonymous attribute manually. Heck, it would be good education :-)
If we make CORS not mandatory anytime in the future, automatically adding cross-origin=anonymous takes away that flexibility
I think we may be thinking about two different "compatibility" cases:
- right now, you have to add a ACAO header on the server to make SRI work and in the future, you'd like to remove this header from the server
- right now, you can't use SRI with resources that aren't served with ACAO headers and in the future, browsers may relax this restriction and allow this
In the first case, you're right, automatically turning loads into CORS loads will break the ability to drop the header on the server without breaking compatibility with old clients.
In the second case however, it doesn't matter because old clients will continue to fail like they always have. That's the compatibility case I'm more interested in personally.
I like the idea of CORS being implicit. I don't think we will block ourselves, because frankly, I'm more or less convinced that we will never be able to not require CORS for SRI.
I agree that not-sending credentials can be surprising to users but it's better than using with-credentials. But oddities are really not uncommon in the web, if developers don't fully understand what they are doing :/
User Agents could alert the user that they did something for the author and recommend them to include the crossorigin attribute in their HTML, because the explicitness adds more clarity.
- We can skip the implicit
crossorigin=anonymousbehavior for same-origin requests. - As long as authors can override the behavior by setting
crossoriginexplicitly, it seems fine to default to not sending credentials.
In @fmarier's case (2), I disagree that we can go back. If the client implicitly sends crossorigin=anonymous today, and the server serves up corresponding CORS headers in response, it will all pass well and good when the response comes in.
But if user agents suddenly drop the implicit crossorigin=anonymous, then what had previously been an implicit but valid CORS request, will suddenly fail when the response comes in. And this will happen to applications all of a sudden that used to work. I cannot imagine Chrome ever doing this, as it would break every application that relied on the implicit crossorigin=anonymous attribute, so from my perspective, yes, we are locked in at that point.
In addition, I find all of the suggested behaviors quite surprising, especially @mikewest's latest suggestion in (1) where the headers would change based on if it's same origin or not. I suppose it doesn't matter, in the general sense, but it just doesn't smell right to not only implicitly tie attributes together, but to do it dependently based on the origin. I'd feel better about it if anyone knows of any similar attribute behavior that I'm just not thinking of.
As I've stated before, I'd prefer not to go this route initially, as it's easy to add in implicit crossorigin=anonymous later, but we certainly can't take it away. In any case, if we're considering doing this, I feel quite strongly that we need consensus on the mailing list, to make sure we're not missing any corner cases.
Looking again at my second example (server without ACAO header), whether or not we add a crossorigin attribute doesn't actually help, like @metromoxie points out:
- On a V1 user agent, the missing ACAO header means that a non-CORS load will fail open (load script with warning), while a CORS load will fail (blocked). So automatically adding a
crossoriginattribute would make things worse. - On a hypothetical V2 user agent, the missing ACAO header wouldn't matter for a non-CORS load and that would be successful (hypothetically). A CORS-load would continue to fail (be blocked).
Perhaps it's better to just fail open for now and see what we can do in V2 once we see SRI used in the wild.
Again, I'd lean towards the latter (implicitly adding this, when using integrity cross-origin) since I find it rather unlikely that we will be able to lift the CORS requirement for SRI ever.
If we can not find an agreement by tomorrow morning (EU time; i.e. ~16hrs from now), I propose we take this to the mailing list.
I feel very strongly that implicitly adding crossorigin=anonymous requires mailing list discussion. That's a pretty big change that I don't think is okay for the editors to quietly add in the background. I'm not a big fan of that anyway, as I mentioned above, but if consensus on the mailing list is to go with it, then I wouldn't force the issue.
For the simpler case, of failing closed without a CORS request, I think that doesn't require list discussion, but I want to make sure we acknowledge that we can't go back from that decision.
To summarize my perspective on the 3 options:
- Fail open when the request is cross origin and not CORS enabled. I don't think this requires any further discussion since it would be backwards compatible to fail closed in the future.
- Fail closed when the request is cross origin and not CORS enabled. I don't think this requires any further discussion because it was already discussed and was consensus on the mailing list, but we should just be aware that we can't go back from this.
- Attach crossorigin=anonymous on all requests, unless there is an overriding crossorigin attribute already present. I think this requires further discussion on the mailing list because it would be a change from the consensus API, and I want to make sure we're not missing any side effects. And, as with step 2, we should acknowledge that we can't go back.
Sure. I'll write something up tonight/in the morning.
So, what would we gain from automatically adding the crossorigin attribute on all cross-domain requests?
I believe I was wrong in thinking that it would help future compatibility. I can see that it would be helpful to authors who forget to add the attribute, but I'm not sure it's a good idea to train them to rely on implicit CORS loads when an explicit one would work just fine.
We can certainly take this to the list, but it's not clear to me anymore why we'd want to do this (as opposed to failing open, the status quo).
The concern is that if we don't do this, developers will think integrity is being enforced when it is not.
Sure, but that's also the case if they misspell "sha256" (e.g. SHA256, SHA-256) or use a base64url encoding instead of base64.
At the end of the day, developers who care about integrity protection should pay attention to the devtool console.
yup; it is very likely that we will end up on that state; the question is whether this is something we should bring up on the mailing list. I tend to agree that we should confirm on the list.
I'm going to send out an email. I think that we don't have clear consensus here, and I want to make sure the community is on the same page. Feel free to respond to my email that I'll send out momentarily with any additions or corrections.
Thanks for bringing this up on the mailing list, Joel!
Based on the lack of discussion on WebAppSec, other than @annevk's comments supporting 'fail open', I suggest we stick with 'fail open.' We should keep our eyes open for how developer's are using the integrity attribute, and if we see issues in the future, we should consider going to a 'fail closed' model. Any disagreement?
Sounds good to me too. We can always change course later.
agreed
Reopening this as we are discussing it at the F2F...
Attempt to summarize discussion of this at the Berlin F2F, nobody has come up with a plausible future in which we could enable integrity checking on opaque responses, so the risk of developers not realizing they are failing open seems to be much larger than constraining ourselves for future extensibility nobody thinks will really be possible
I specifically noted that you can't change from fail-open to fail-closed in the future; it's an observable behavior change that has security impacts. It also seems unlikely that you can go the other way (closed -> open) for fear of adding silent breakage to previously secure sites.
Based on the discussion we had, I would support failing closed when:
- we are loading a cross-origin resource
- the
integrityattribute is non-empty (it could be valid or invalid) - the
crossoriginattribute is missing (i.e. it's not a CORS load)
That would lock us into CORS for the forseeable future, but it would prevent SRI from being silently disabled when developers forget CORS.
So when the request is same-origin or crossorigin is present, integrity can still be ignored? That seems fairly magical. What principles did you use to arrive there?
So when the request is same-origin or
crossoriginis present,integritycan still be ignored? That seems fairly magical. What principles did you use to arrive there?
We fail open when the integrity attribute only contains invalid tokens in order to allow for future expansion of the syntax.
If you're willing to fail-closed for the above, you might as well make integrity imply crossorigin=anonymous. There is yet to be a plausible argument why CORS could go away in the future. And if that were the case, you could just pick a new attribute.
If we're going this route, Chrome is now in a bit of a pickle. I'm pretty sad we didn't discuss this further on the mailing list earlier when I sent out an email discussing this topic, because now Chrome has shipped with fail-open.
We can change it, but that's going to break Chrome users (although I highly doubt there are many yet as we haven't made posts or announcements about it yet). Thus, any thoughts about how to soften the blow here would be greatly appreciated.
To the more specifics of this topic, as discussed above, I am OK failing closed, but I personally dislike implicitly tying crossorigin=anonymous to requests (see many above comments in this thread). I think this will be somewhat magical, and I think it's better to just fail and make clear console warnings.
We can change it, but that's going to break Chrome users (although I highly doubt there are many yet as we haven't made posts or announcements about it yet). Thus, any thoughts about how to soften the blow here would be greatly appreciated.
How far has it ridden the trains yet? @slightlyoff suggested uplifting a pref change to turn if off by default if it's too late to change the code to fail closed. I'm planning to land it pref'ed OFF in Firefox until we resolve this.
Also, in terms of breakage, it should be fairly limited as we're talking about breaking people who were misusing it already.
Though I may have argued for implicit crossorigin=anonymous, I now agree that implicit crossorigin=anonymous is a bit 'too magical'. I hope that good tooling and good feedback in the developer console will get us there without adding too many quirks.
@fmarier @slightlyoff, we don't have a pref to turn this off anymore (that's what I took away when we shipped SRI). It's possible that we could backport a patch to change it since it's going to Beta uses next week, but I doubt it. I suspect given the likely low rate of breakage we'll just ship the feature for 6 weeks as is, and try to make it as clear in our announcements as possible that it will change.
Just to finalize this, before we start editing the spec as appropriate, we're saying that a lack of CORS for cross origin requests should result in a network error? @mozfreddyb and I seem to be firmly in the camp of requiring the crossorigin attribute explicitly; would anyone like to make the case for it being implicit?
@metromoxie I straddle both groups regarding the attribute:
- Implied
anonymouswould reduce developer effort and be simpler to implement and reduce implementation errors of setting a misspelled attribute. It also has the side benefit of auto choosinganonymousfor them rather thanwith-credentialswhich reduces exposure of those details. - Explicit would train new developers that they need to talk to sysadmins etc to add the CORS headers and clearly documents the mode they are in.
It's possible that we could backport a patch to change it since it's going to Beta uses next week, but I doubt it.
If that's possible it would be ideal since you'd be able to eliminate all sources of breakage.
we're saying that a lack of CORS for cross origin requests should result in a network error?
Correct.
@mozfreddyb and I seem to be firmly in the camp of requiring the crossorigin attribute explicitly; would anyone like to make the case for it being implicit?
I am also in that camp, but I think we should take it to the list.
Since I brought it up on the list last time, would one of you mind emailing? Perhaps you'll elicit more of a response than I did :-)
@metromoxie sent 
I persuaded myself against the implied attribute hah.
I also noted about the unknown algo case, I would prefer some control over this as there isn't specific control for when the user agent doesn't recognise any algo in the list.
We need some closure here. I just uploaded #437 which I think implements the consensus we came to here. I would really, really love to finalize this ASAP since it will probably require a merge on the Chrome end to our Beta branch, which isn't particularly fun.
Note that effectively what you're saying here is that specifying integrity="" has the side effect of setting mode to "same-origin" (rather than its "no-cors" default). Seems not too different from saying integrity="" has the side effect of setting mode to "cors", but up to you folks I suppose.
@annevk the discussed setup would be conditionally setting crossorigin="anonymous" when not same-origin; rather than conditionally requiring crossorigin to be set.
I think you have coached me back to sitting on the fence again; I like the idea of less attributes certainly.
integrity="" is a no-op, as per step 2.1 of Modifications to Fetch, so it's not equivalent to setting CORS to same-origin.
@metromoxie I meant specifying it with a value. I use ="" to indicate it's an attribute.
I think we need to do a follow-up to #314 but I'd like us to agree on the expected behavior before I do that.
What I'm interested in is how to deal with non-eligible resources that have empty, valid or invalid metadata, as well as eligible resources with invalid metadata.
Let's consider these non-CORS examples:
<script src="http://elsewhere.example.com/foo.js" integrity=""><script src="http://elsewhere.example.com/foo.js" integrity="rot13-base64hash"><script src="http://elsewhere.example.com/foo.js" integrity="invalid">Here, I would suggest that the first one should be loaded because it's equivalent to the pre-SRI web. The presence of an empty
integrityattribute shouldn't change anything for authors. This is what I fixed in #314.The last two on the other hand, should probably be blocked because they represent an attempt by the author to use SRI badly and by refusing to load their resources we ensure that they notice immediately and fix the problem. Any objections?
Now, let's look at the eligible cases (same-origin):
<script src="foo.js" integrity=" "><script src="foo.js" integrity="rot13-base64hash"><script src="foo.js" integrity="invalid">Here, I would suggest that the first two be loaded. The first example is obvious since everything is fine. In the second case, we already decided to "fail open" to be forward compatible with future hash algorithms.
The third case is different because we now have a mechanism to add arbitrary per-hash options. We could decide that this is how future versions of the spec will need to extend the syntax and block subresources that consist only of invalid metadata (as opposed to valid but unsupported hash algorithm). This would allow authors to easily catch mistakes like
integrity="sha256bash64hash"(i.e. forgetting the dash). Or we could fail open in this case too. Thoughts?