Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1880283002:
Disallow redirects in SDCH dictionary fetches. (Closed)
|
Created:
4 years, 8 months ago by Randy Smith (Not in Mondays) Modified:
4 years, 7 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, helen li Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisallowredirects in SDCH dictionary fetches.
BUG=601234
R=eroman@chromium.org
Committed: https://crrev.com/a6980cd62b2118af7062163494c8a8bf5076ec20
Cr-Commit-Position: refs/heads/master@{#388264}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Shifted to disallowing redirects. #Patch Set 3 : Merge to p388233. #
Messages
Total messages: 31 (10 generated)
Eric: Willing to take a look at this? I figure you've been glancing at SDCH, and Matt deserves a break :-}.
hah, no good deed goes unpunished i guess :)
On 2016/04/12 21:07:46, eroman wrote: > hah, no good deed goes unpunished i guess :) Yeah, you had to go and put yourself on my radar screen :-}.
eroman@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:302: if (redirect_url_.GetOrigin() != current_request_->url().GetOrigin()) { +rsleevi for security perspective, although mkwst may be a better person to ask for this as he has been cleaning up our origin checks. I would be *very* cautious about sprinkling more GetOrigin() calls into //net. The specific meaning of these origin comparisons is subtle and depends on the context -- i.e. see SecurityContext::canDisplay() vs SecurityContext::canAccess() vs SecurityContext::canRequest(). I don't have access to the bug so not sure how to evaluate this. Can you add more details on what exactly the goals are ? For instance some considerations: * This will disallow redirects between subdomains * This will disallow redirects to data:URLs (I believe). Kind of weird, but not really a same origin violation. * Can't redirect from http --> https * Can't redirect to dictionary hosted on third party site (i.e. CDN). https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:303: current_request_->Cancel(); I think this is redundant -- calling ResetRequest() below cancels too right? https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:305: next_state_ = STATE_SEND_REQUEST; Shouldn't there be a return statement here, and error out?
rsleevi@chromium.org changed reviewers: + mkwst@chromium.org
CC'ing Mike as master of CORS. Note: Public code review, private bug. https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:301: // be necessary when/if SDCH is restricted to HTTPS. The way this comment is worded, I think you're confused about cross-origin (origin = scheme, host, port). That is "cross-port" and "cross-scheme" are redundant with "cross-origin", and the latter comment doesn't really explain why it would be necessary, which is perhaps the most useful thing. https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:302: if (redirect_url_.GetOrigin() != current_request_->url().GetOrigin()) { On 2016/04/13 00:12:38, eroman wrote: > +rsleevi for security perspective, although mkwst may be a better person to ask > for this as he has been cleaning up our origin checks. > > I would be *very* cautious about sprinkling more GetOrigin() calls into //net. > The specific meaning of these origin comparisons is subtle and depends on the > context -- i.e. see SecurityContext::canDisplay() vs > SecurityContext::canAccess() vs SecurityContext::canRequest(). > > I don't have access to the bug so not sure how to evaluate this. Can you add > more details on what exactly the goals are ? > > For instance some considerations: > * This will disallow redirects between subdomains > * This will disallow redirects to data:URLs (I believe). Kind of weird, but > not really a same origin violation. > * Can't redirect from http --> https > * Can't redirect to dictionary hosted on third party site (i.e. CDN). Right, Mike is definitely the better reviewer here, because the whole policy regarding redirects is mediated in Blink land and is much more closely tied to access policies and the effective security origin than the literal security origin (and CORS). I'd like to get Mike's feedback, but my initial reaction is that this probably isn't quite right either. This is where it'd really be helpful if SDCH had a spec (and I'd really like to push on that; we shouldn't keep working on SDCH or keeping it enabled if we don't have a clear spec, preferably with one that explains how things like this work - is this a Fetch()-enabled request, or something lower? As implemented, it doesn't fit into any of the other web platform feature explainers, including link-rel bits)
https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:302: if (redirect_url_.GetOrigin() != current_request_->url().GetOrigin()) { 1. I agree with Ryan that a spec which explained how fetching worked for SDCH would be useful. I'd be happy to help put something together if you're interested in doing so (with the obvious caveats that I don't actually know anything about SDCH). 2. Is there actually value in thinking about any of this? That is, how much pain would it cause to simply reject redirect responses when fetching SDCH dictionaries? 3. Based on the discussion in the bug, it sounds like the dictionary we fetch from `https://victim.test` after redirection from `https://malicious.test` will only be used on `https://victim.test`, and cannot provide `https://malicious.test` with information about the dictionary's contents. Is that accurate? If so, then there's not much to worry about in terms of information leakage.
Description was changed from ========== Disallow cross-origin redirects in SDCH dictionary fetches. BUG=601234 R=eroman@chromium.org ========== to ========== Disallow cross-origin redirects in SDCH dictionary fetches. BUG=601234 R=eroman@chromium.org ==========
On 2016/04/13 10:54:07, Mike West wrote: > https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... > File net/url_request/sdch_dictionary_fetcher.cc (right): > > https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... > net/url_request/sdch_dictionary_fetcher.cc:302: if (redirect_url_.GetOrigin() != > current_request_->url().GetOrigin()) { > 1. I agree with Ryan that a spec which explained how fetching worked for SDCH > would be useful. I'd be happy to help put something together if you're > interested in doing so (with the obvious caveats that I don't actually know > anything about SDCH). I think a more precise spec would be a great idea. Unfortunately, I don't have time to work on it. Helen Li (cc'd) is planning to do some SDCH work later this quarter; that might be something she could work on. > 2. Is there actually value in thinking about any of this? That is, how much pain > would it cause to simply reject redirect responses when fetching SDCH > dictionaries? I don't actually know. I'm reluctant to do that just because I know there are a lot of sites using SDCH and I don't know how they're using it, so I'd rather not make that change without a finch trial to see how much breakage it causes, or at least some UMA and some cycles at this end to track the results of the UMA as it roles out. So I'd rather do something simple and restrictive in the cross-origin blocking space, if such a thing exists. > 3. Based on the discussion in the bug, it sounds like the dictionary we fetch > from `https://victim.test` after redirection from `https://malicious.test` will > only be used on `https://victim.test`, and cannot provide > `https://malicious.test` with information about the dictionary's contents. Is > that accurate? If so, then there's not much to worry about in terms of > information leakage. I think the OP on the bug provided a scenario which has a very small danger to it, but yes, agreed, it's not that big a deal. (If it's relevant to your judgement about the CL how severe the vulnerability is, let's have that discussion on the bug.)
On 2016/04/13 at 15:22:34, rdsmith wrote: > > 2. Is there actually value in thinking about any of this? That is, how much pain > > would it cause to simply reject redirect responses when fetching SDCH > > dictionaries? > > I don't actually know. I'm reluctant to do that just because I know there are a lot of sites using SDCH and I don't know how they're using it, so I'd rather not make that change without a finch trial to see how much breakage it causes, or at least some UMA and some cycles at this end to track the results of the UMA as it roles out. So I'd rather do something simple and restrictive in the cross-origin blocking space, if such a thing exists. Or we could break it and see if anyone complains. :) It doesn't sound like we're planning to merge this back to M50/51, so we have plenty of time to deal with fallout. If you're reluctant to do that, and you'd prefer to only break cross-origin redirects, then the code you've written does that and I think it's reasonable to do so. I have some small ideas about how to adjust things, but I'll add those as comments to the patch.
https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:301: // be necessary when/if SDCH is restricted to HTTPS. How are we not already restricting SDCH to HTTPS? I thought that was a requirement to begin with? Also +1 to Ryan's comments. Cross-origin includes cross-port (and cross-scheme). https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:302: if (redirect_url_.GetOrigin() != current_request_->url().GetOrigin()) { On the narrow question of this code: I'd suggest switching to `url::Origin(redirect_url_).IsSameOriginWith(url::Origin(current_request_->url()))`. (Yes, it is terribly confusing that `GURL::GetOrigin()` returns a `GURL` and not a `url::Origin`. I'm working on it.). This check seems fine, since what we care about here is the origin of the URL that you tried to load, and the origin of the URL you've been directed to. As Eric noted, this will exclude a few things you might care about: `https://www.example.com` couldn't redirect to `https://sdch.example.com`, nor to `http://[anything]`, nor to `data:` (though I think we might block that anyway?). On the broader question of what behavior we'd like to create: I think it would be simpler for us to simply block redirects, as opposed to only blocking cross-origin redirects. I don't know enough about your user base, but I suspect that would be doable; there doesn't seem to be a ton of benefit to allowing `example.com` to point to a different resource on its own origin (whereas I could imagine benifits for them pointing to their CDN or something).
On 2016/04/13 15:49:31, Mike West wrote: > https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... > File net/url_request/sdch_dictionary_fetcher.cc (right): > > https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... > net/url_request/sdch_dictionary_fetcher.cc:301: // be necessary when/if SDCH is > restricted to HTTPS. > How are we not already restricting SDCH to HTTPS? I thought that was a > requirement to begin with? Nope--SDCH has been around for a very long while. It used to be *HTTP*-only; one of the first things I did when I owned it was drive the finch trial that allowed us to turn it on on HTTPS. We've agreed in theory that we should switch to HTTPS-only, but no one's had the bandwidth to send out the Intent-to-Deprecate and drive the process. > Also +1 to Ryan's comments. Cross-origin includes cross-port (and cross-scheme). I'm good with that. > https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... > net/url_request/sdch_dictionary_fetcher.cc:302: if (redirect_url_.GetOrigin() != > current_request_->url().GetOrigin()) { > On the narrow question of this code: I'd suggest switching to > `url::Origin(redirect_url_).IsSameOriginWith(url::Origin(current_request_->url()))`. > (Yes, it is terribly confusing that `GURL::GetOrigin()` returns a `GURL` and not > a `url::Origin`. I'm working on it.). This check seems fine, since what we care > about here is the origin of the URL that you tried to load, and the origin of > the URL you've been directed to. As Eric noted, this will exclude a few things > you might care about: `https://www.example.com` couldn't redirect to > `https://sdch.example.com`, nor to `http://[anything]`, nor to `data:` (though I > think we might block that anyway?). I think the scheme for dictionaries needs to be HTTP/HTTPS, yes. > On the broader question of what behavior we'd like to create: I think it would > be simpler for us to simply block redirects, as opposed to only blocking > cross-origin redirects. I don't know enough about your user base, but I suspect > that would be doable; there doesn't seem to be a ton of benefit to allowing > `example.com` to point to a different resource on its own origin (whereas I > could imagine benifits for them pointing to their CDN or something). So in the abstract I have no objection; my reluctance is all around the time/attention required to track the metrics and handle the rollout. But your point is fair that there's a pretty clear use case that the change I've made will break, so I'm probably chasing a chimera. I'll just block redirects. New patch set coming soon.
Description was changed from ========== Disallow cross-origin redirects in SDCH dictionary fetches. BUG=601234 R=eroman@chromium.org ========== to ========== Disallowredirects in SDCH dictionary fetches. BUG=601234 R=eroman@chromium.org ==========
New patchset up that just blocks redirects. I believe I've responded to all comments (and most are moot) but the responses were a combination of in-source and top level, so if there's some issue you think applies to the new system of just blocking redirects, you might want to make sure I responded to it. PTAL? https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:303: current_request_->Cancel(); On 2016/04/13 00:12:38, eroman wrote: > I think this is redundant -- calling ResetRequest() below cancels too right? Yeah, I think you're right. Good point. https://codereview.chromium.org/1880283002/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:305: next_state_ = STATE_SEND_REQUEST; On 2016/04/13 00:12:38, eroman wrote: > Shouldn't there be a return statement here, and error out? Ooops. Yes, there should. That suggests my tests were incomplete, too. I think the issue's moot now, but thanks for catching it.
LGTM, even though my LGTM is useless because I don't own any of this code!
On 2016/04/13 18:21:35, Mike West wrote: > LGTM, even though my LGTM is useless because I don't own any of this code! I think only one of us needs to own the code, and I presume you're a chromium committer, so life is good. Thank you!!
LGTM For whomever comes along and does source archaelogy on this, I think that before we introduce the capability to handle redirects or cross-origin fetches, we need to be able to spec and explain SDCH in relation to Fetch() and the overall platform. Both Mike and I are happy to collaborate on that (based on the comments on this review), but that should be a precondition for any changes in behaviour, to avoid any future bugs or unexpected nasty surprises.
The CQ bit was checked by rdsmith@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880283002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880283002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by rdsmith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1880283002/#ps40001 (title: "Merge to p388233.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880283002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880283002/40001
Message was sent while issue was closed.
Description was changed from ========== Disallowredirects in SDCH dictionary fetches. BUG=601234 R=eroman@chromium.org ========== to ========== Disallowredirects in SDCH dictionary fetches. BUG=601234 R=eroman@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Disallowredirects in SDCH dictionary fetches. BUG=601234 R=eroman@chromium.org ========== to ========== Disallowredirects in SDCH dictionary fetches. BUG=601234 R=eroman@chromium.org Committed: https://crrev.com/a6980cd62b2118af7062163494c8a8bf5076ec20 Cr-Commit-Position: refs/heads/master@{#388264} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a6980cd62b2118af7062163494c8a8bf5076ec20 Cr-Commit-Position: refs/heads/master@{#388264}
Message was sent while issue was closed.
On Tuesday, April 12, 2016 at 3:02:59 PM UTC-6, rds...@chromium.org wrote: > > Reviewers: eroman > CL: https://codereview.chromium.org/1880283002/ > > Message: > Eric: Willing to take a look at this? I figure you've been glancing at > SDCH, > and Matt deserves a break :-}. > > Description: > Disallow cross-origin redirects in SDCH dictionary fetches. > > BUG=601234 > R=er...@chromium.org <javascript:> > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+199, -39 lines): > M net/url_request/sdch_dictionary_fetcher.h > M net/url_request/sdch_dictionary_fetcher.cc > M net/url_request/sdch_dictionary_fetcher_unittest.cc > M net/url_request/url_request.h > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On Tuesday, April 12, 2016 at 3:02:59 PM UTC-6, rds...@chromium.org wrote: > > Reviewers: eroman > CL: https://codereview.chromium.org/1880283002/ > > Message: > Eric: Willing to take a look at this? I figure you've been glancing at > SDCH, > and Matt deserves a break :-}. > > Description: > Disallow cross-origin redirects in SDCH dictionary fetches. > > BUG=601234 > R=er...@chromium.org <javascript:> > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+199, -39 lines): > M net/url_request/sdch_dictionary_fetcher.h > M net/url_request/sdch_dictionary_fetcher.cc > M net/url_request/sdch_dictionary_fetcher_unittest.cc > M net/url_request/url_request.h > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
