Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2970913002:
Implement CORS preflights for Expect-CT reports (Closed)
DescriptionImplement CORS preflights for Expect-CT reports
Expect-CT reports are sent with a Content-Type header of
application/expect-ct-report+json. This Content-Type (nor "application/json") is
not CORS safelisted, meaning that Chrome arguably ought to send CORS preflights
to make sure the designated report collection server has opted in to receive
Expect-CT reports. Otherwise, web content would be able to trigger reports to
arbitrary endpoints with a non-safelisted Content-Type header.
Therefore, this CL implements CORS preflight requests before sending
reports. The wrinkle is that Expect-CT is checked at connection setup time,
before being associated with a particular URLRequest, much less an initiating
origin, so we cannot construct a proper Origin header to include in the
preflight request. Instead, we set the Origin header to "null", and expect
`Access-Control-Allow-Origin: *` or `Access-Control-Allow-Origin: null` in
response. While this is a bit weird, it is safe because reports are sent without
credentials and it requires the server to opt in to receiving reports.
See https://lists.w3.org/Archives/Public/ietf-http-wg/2017AprJun/0168.html for
more discussion on the CORS issues and background on why we have settled on
sending `Origin: null` preflights.
https://fetch.spec.whatwg.org/#cors-preflight-fetch describes the preflights
implemented in this CL.
BUG=679012
Review-Url: https://codereview.chromium.org/2970913002
Cr-Commit-Position: refs/heads/master@{#484849}
Committed: https://chromium.googlesource.com/chromium/src/+/70adceb53e4a7c7d1cbfa080ab6fd4d6ae2878c6
Patch Set 1 #
Total comments: 23
Patch Set 2 : meacer comments #
Total comments: 4
Patch Set 3 : meacer nits #
Messages
Total messages: 26 (13 generated)
estark@chromium.org changed reviewers: + meacer@chromium.org
meacer, can you please review? This is the last major bit of implementation work for the Expect-CT header, which I'm planning to ship in M61, so ideally I'd like to get this landed before Friday. Thanks!
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/07/05 05:47:33, estark (OOO until Jul 10) wrote: > meacer, can you please review? This is the last major bit of implementation work > for the Expect-CT header, which I'm planning to ship in M61, so ideally I'd like > to get this landed before Friday. Thanks! I read the threads (quite a journey down the living quarters of rabbits) and I'm still not sure what exactly we are fixing, so please bear with me. Is the concern that Expect-CT reports (and possibly others) violate CORS principles? Or is it that without CORS check web content can send arbitrary report requests? Or both? If it's the former, there seems to be disagreement that browser initiated requests should be considered in scope of SOP. I don't want to trigger a rehash of the whole discussion, but it seems the ietf thread reached a consensus without taking those concerns into consideration? (I'm probably missing something here, but still) If the latter, I'm not sure how ACAO:* or ACAO:null prevents sites from sending arbitrary reports. * would obviously still allow any site, and null would allow data URLs as data urls have null origins. That seems to be defying the purpose of the CL, or am I missing something here too?
And the CL review https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:34: bool FindHeaderValues(net::URLRequest* request, nit: Rename to RequestHasHeaderWithValue or simply HasHeaderValues? https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:36: const std::set<std::string>& expected_values) { nit: allowed_values instead of expected_values? expected_values slightly implies that all values are expected. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:39: std::vector<std::string> response_values = base::SplitString( nit: const https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:211: int response_code = request->GetResponseCode(); nit: const https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:222: RecordUMAOnFailure(preflight.report_uri, request->status().error(), There is a use after free here: preflight is a reference to the value keyed by request, and the line above erases that value. Same in line 232-233. You can move the erase line below this line, but that's still error prone as someone can add a line below that accesses preflight. Maybe add a comment as well? Alternatively, copy (or rather std::move) preflight instead. You can then move |inflight_preflights_.erase(request)| right after line 209 and remove the duplicate calls. But then you can't simply copy an InFlightPreflight, you'll need a move constructor etc etc. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:274: inflight_preflights_[raw_request].report_uri = report_uri; optional nit: To save two extra map lookups you can add a constructor to InFlightPreflight with these 3 params and then simply build it here. nflight_preflights_[raw_request] = InflightPreFlight(url_request, ...); The added benefit is that you can also make the members of InflightPreFlight all const members. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_ct_reporter.h (right): https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.h:53: struct InFlightPreflight { nit: I think the name is a tad confusing but unfortunately I don't have better suggestion. SoaringPreflightRequest? (No) Feel free to keep as is. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc (right): https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:74: void WaitForReport(const GURL& report_uri, const base::Closure& callback) { This isn't actually waiting for a report, is it? Perhaps pass the runloop as a const ref and run it directly from here. But even then I'm confused about how this method would work. The test cases below create a runloop, call WaitForReport and then run the run loop. So - If the Send() happens before WaitForReport() is called, the run loop would never run because it's quit closure will run before the run loop runs. That's Working as intended. - But if the Send() happens after WaitForReport(), the test will timeout because the runloop will start, and the quit closure will never run. Seems like what you want is to have the runloop as a member of this class, then run its quit closure when a Send() happens. Then WaitForReport() can simply start the runloop if latest_report_uri_.is_empty(). https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:80: return; nit: not necessary https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:371: const net::HostPortPair host_port, Pass by ref https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:635: net::HostPortPair host_port("example.test", 443); nit: const (or perhaps inline these in lines 641, 667 and 693)
On 2017/07/05 20:50:11, meacer wrote: > On 2017/07/05 05:47:33, estark (OOO until Jul 10) wrote: > > meacer, can you please review? This is the last major bit of implementation > work > > for the Expect-CT header, which I'm planning to ship in M61, so ideally I'd > like > > to get this landed before Friday. Thanks! > > I read the threads (quite a journey down the living quarters of rabbits) and I'm > still not sure what exactly we are fixing, so please bear with me. > > Is the concern that Expect-CT reports (and possibly others) violate CORS > principles? Or is it that without CORS check web content can send arbitrary > report requests? Or both? > > If it's the former, there seems to be disagreement that browser initiated > requests should be considered in scope of SOP. I don't want to trigger a rehash > of the whole discussion, but it seems the ietf thread reached a consensus > without taking those concerns into consideration? (I'm probably missing > something here, but still) > > If the latter, I'm not sure how ACAO:* or ACAO:null prevents sites from sending > arbitrary reports. * would obviously still allow any site, and null would allow > data URLs as data urls have null origins. That seems to be defying the purpose > of the CL, or am I missing something here too? Thanks for going down the IETF rabbit-hole. :) The concern is the former, that Expect-CT reports actually violate the CORS spec and threat model. Here are two things that might help explain better: (1) Expect-CT reports aren't really browser-initiated requests. If you visit https://evil.com, it can set an Expect-CT header with a report-uri pointing to https://victim.com and subsequently intentionally violate CT policy -- that would trigger a request to https://victim.com which victim.com might not be expecting. So these requests are not really browser-initiated because they can be triggered by web content. (2) The threat model of CORS is that existing servers shouldn't be made vulnerable. So the specific concern here would be this: suppose there is an intranet server https://victim-corp that implicitly trusts data received in a `Content-Type: blah` request -- say, maybe the server crashes upon receiving such a request unless the request has some particular body. This would be considered "safe" behavior for a server, according to the CORS spec, because arbitrary web content can't trigger requests to https://victim-corp with `Content-Type: blah` headers, unless the server explicitly opts in to receiving them by responding positively to a CORS preflight. The principle of CORS is that the web platform shouldn't just go ahead and add an ability for web content to trigger `Content-Type: blah` requests (even if the web content can't control the body or read the response) without the server opting it, because that would make the existing https://victim-corp server vulnerable. Without a preflight, this is exactly what Expect-CT would be doing, except it's 'application/expect-ct-report' instead of 'blah'. One could argue that this is a silly threat model: that https://victim.corp is just plain buggy and it's not the web platform's responsibility to worry about making it vulnerable. But this is the threat model of CORS, and if we don't think that Expect-CT reports need to send preflights, then that's a larger issue that we should take up with the CORS spec. (I happen to think we should take it up with the CORS spec, but I don't want to block Expect-CT on it; I'd rather send preflights to start with and then later stop sending preflights if we can convince other browser vendors that the Content-Type preflight requirement should be dropped from CORS.) So, the intent of the preflight is to obtain permission from the report collection server that it is okay receiving requests with `Content-Type: application/expect-ct-report` headers. There isn't an expectation that report collection servers would say "yes" to some origins and "no" to others; it's just a way to ensure that the server we are sending reports to expects to receive Expect-CT reports at all.
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:34: bool FindHeaderValues(net::URLRequest* request, On 2017/07/05 23:48:59, meacer wrote: > nit: Rename to RequestHasHeaderWithValue or simply HasHeaderValues? Done. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:36: const std::set<std::string>& expected_values) { On 2017/07/05 23:48:59, meacer wrote: > nit: allowed_values instead of expected_values? expected_values slightly implies > that all values are expected. Done. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:39: std::vector<std::string> response_values = base::SplitString( On 2017/07/05 23:48:59, meacer wrote: > nit: const Done. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:211: int response_code = request->GetResponseCode(); On 2017/07/05 23:48:59, meacer wrote: > nit: const Done. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:222: RecordUMAOnFailure(preflight.report_uri, request->status().error(), On 2017/07/05 23:48:58, meacer wrote: > There is a use after free here: preflight is a reference to the value keyed by > request, and the line above erases that value. Same in line 232-233. > > You can move the erase line below this line, but that's still error prone as > someone can add a line below that accesses preflight. Maybe add a comment as > well? > > Alternatively, copy (or rather std::move) preflight instead. You can then move > |inflight_preflights_.erase(request)| right after line 209 and remove the > duplicate calls. But then you can't simply copy an InFlightPreflight, you'll > need a move constructor etc etc. Done. Moved the erase below and added comments. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.cc:274: inflight_preflights_[raw_request].report_uri = report_uri; On 2017/07/05 23:48:59, meacer wrote: > optional nit: To save two extra map lookups you can add a constructor to > InFlightPreflight with these 3 params and then simply build it here. > > nflight_preflights_[raw_request] = InflightPreFlight(url_request, ...); > > The added benefit is that you can also make the members of InflightPreFlight all > const members. Done. Changed to a unique_ptr in the map to avoid a copy. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_ct_reporter.h (right): https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter.h:53: struct InFlightPreflight { On 2017/07/05 23:48:59, meacer wrote: > nit: I think the name is a tad confusing but unfortunately I don't have better > suggestion. > > SoaringPreflightRequest? (No) > > Feel free to keep as is. Renamed to PreflightInProgress https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc (right): https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:74: void WaitForReport(const GURL& report_uri, const base::Closure& callback) { On 2017/07/05 23:48:59, meacer wrote: > This isn't actually waiting for a report, is it? > > Perhaps pass the runloop as a const ref and run it directly from here. > > But even then I'm confused about how this method would work. > > The test cases below create a runloop, call WaitForReport and then run the run > loop. So > - If the Send() happens before WaitForReport() is called, the run loop would > never run because it's quit closure will run before the run loop runs. That's > Working as intended. > - But if the Send() happens after WaitForReport(), the test will timeout because > the runloop will start, and the quit closure will never run. > > Seems like what you want is to have the runloop as a member of this class, then > run its quit closure when a Send() happens. Then WaitForReport() can simply > start the runloop if latest_report_uri_.is_empty(). I changed it to create and run the run loop inside the function, but other than that I don't understand: why would the test timeout if Send() happens after WaitForReport()? AFAICT this is basically following the canonical usage pattern of creating a run loop and running it, with an asynchronous task that calls the QuitClosure(): https://cs.chromium.org/chromium/src/base/run_loop.h?sq=package:chromium&dr=S... (Anecdotally, I saw in my local runs that WaitForReport() happens before Send(), and the test passes, doesn't time out.) https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:80: return; On 2017/07/05 23:48:59, meacer wrote: > nit: not necessary Done. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:371: const net::HostPortPair host_port, On 2017/07/05 23:48:59, meacer wrote: > Pass by ref Done. https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:635: net::HostPortPair host_port("example.test", 443); On 2017/07/05 23:48:59, meacer wrote: > nit: const (or perhaps inline these in lines 641, 667 and 693) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/06 06:05:44, estark (OOO until Jul 10) wrote: > On 2017/07/05 20:50:11, meacer wrote: > > On 2017/07/05 05:47:33, estark (OOO until Jul 10) wrote: > > > meacer, can you please review? This is the last major bit of implementation > > work > > > for the Expect-CT header, which I'm planning to ship in M61, so ideally I'd > > like > > > to get this landed before Friday. Thanks! > > > > I read the threads (quite a journey down the living quarters of rabbits) and > I'm > > still not sure what exactly we are fixing, so please bear with me. > > > > Is the concern that Expect-CT reports (and possibly others) violate CORS > > principles? Or is it that without CORS check web content can send arbitrary > > report requests? Or both? > > > > If it's the former, there seems to be disagreement that browser initiated > > requests should be considered in scope of SOP. I don't want to trigger a > rehash > > of the whole discussion, but it seems the ietf thread reached a consensus > > without taking those concerns into consideration? (I'm probably missing > > something here, but still) > > > > If the latter, I'm not sure how ACAO:* or ACAO:null prevents sites from > sending > > arbitrary reports. * would obviously still allow any site, and null would > allow > > data URLs as data urls have null origins. That seems to be defying the purpose > > of the CL, or am I missing something here too? > > Thanks for going down the IETF rabbit-hole. :) The concern is the former, that > Expect-CT reports actually violate the CORS spec and threat model. Here are two > things that might help explain better: > > (1) Expect-CT reports aren't really browser-initiated requests. If you visit > https://evil.com, it can set an Expect-CT header with a report-uri pointing to > https://victim.com and subsequently intentionally violate CT policy -- that > would trigger a request to https://victim.com which http://victim.com might not be > expecting. So these requests are not really browser-initiated because they can > be triggered by web content. Well, yes they can be triggered by web content, but so are most browser initiated requests. However, in this case the page content doesn't have control over the report's contents which differentiates it from content initiated requests. But I don't intend to override the decision, just wanted to clarify the CL's intention. > > (2) The threat model of CORS is that existing servers shouldn't be made > vulnerable. So the specific concern here would be this: suppose there is an > intranet server https://victim-corp that implicitly trusts data received in a > `Content-Type: blah` request -- say, maybe the server crashes upon receiving > such a request unless the request has some particular body. This would be > considered "safe" behavior for a server, according to the CORS spec, because > arbitrary web content can't trigger requests to https://victim-corp with > `Content-Type: blah` headers, unless the server explicitly opts in to receiving > them by responding positively to a CORS preflight. The principle of CORS is that > the web platform shouldn't just go ahead and add an ability for web content to > trigger `Content-Type: blah` requests (even if the web content can't control the > body or read the response) without the server opting it, because that would make > the existing https://victim-corp server vulnerable. Without a preflight, this is > exactly what Expect-CT would be doing, except it's > 'application/expect-ct-report' instead of 'blah'. One could argue that this is a > silly threat model: that https://victim.corp is just plain buggy and it's not > the web platform's responsibility to worry about making it vulnerable. But this > is the threat model of CORS, and if we don't think that Expect-CT reports need > to send preflights, then that's a larger issue that we should take up with the > CORS spec. (I happen to think we should take it up with the CORS spec, but I > don't want to block Expect-CT on it; I'd rather send preflights to start with > and then later stop sending preflights if we can convince other browser vendors > that the Content-Type preflight requirement should be dropped from CORS.) > > So, the intent of the preflight is to obtain permission from the report > collection server that it is okay receiving requests with `Content-Type: > application/expect-ct-report` headers. There isn't an expectation that report > collection servers would say "yes" to some origins and "no" to others; it's just > a way to ensure that the server we are sending reports to expects to receive > Expect-CT reports at all. Right, we are on the same page here. My point was if the server opts in to receiving reports, this won't prevent arbitrary pages from sending those reports. But it looks like that's not what this change is for, it instead protects not-opted-in servers, which is also totally fine.
Lgtm https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... File chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc (right): https://codereview.chromium.org/2970913002/diff/1/chrome/browser/ssl/chrome_e... chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:74: void WaitForReport(const GURL& report_uri, const base::Closure& callback) { On 2017/07/06 06:39:43, estark (OOO until Jul 10) wrote: > On 2017/07/05 23:48:59, meacer wrote: > > This isn't actually waiting for a report, is it? > > > > Perhaps pass the runloop as a const ref and run it directly from here. > > > > But even then I'm confused about how this method would work. > > > > The test cases below create a runloop, call WaitForReport and then run the run > > loop. So > > - If the Send() happens before WaitForReport() is called, the run loop would > > never run because it's quit closure will run before the run loop runs. That's > > Working as intended. > > - But if the Send() happens after WaitForReport(), the test will timeout > because > > the runloop will start, and the quit closure will never run. > > > > Seems like what you want is to have the runloop as a member of this class, > then > > run its quit closure when a Send() happens. Then WaitForReport() can simply > > start the runloop if latest_report_uri_.is_empty(). > > I changed it to create and run the run loop inside the function, but other than > that I don't understand: why would the test timeout if Send() happens after > WaitForReport()? AFAICT this is basically following the canonical usage pattern > of creating a run loop and running it, with an asynchronous task that calls the > QuitClosure(): > https://cs.chromium.org/chromium/src/base/run_loop.h?sq=package:chromium&dr=S... > > (Anecdotally, I saw in my local runs that WaitForReport() happens before Send(), > and the test passes, doesn't time out.) Sorry, you are right. I missed the fact that you are setting report_callback_ = callback here and then calling report_callback_ in Send(). Without that there wouldn't be any connection between the two which is why I said it would time out. https://codereview.chromium.org/2970913002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/2970913002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_expect_ct_reporter.cc:220: (response_code < 200 || response_code > 299)) { tiny nit: Extra parans not necessary since it's all ||. https://codereview.chromium.org/2970913002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_expect_ct_reporter.cc:258: ChromeExpectCTReporter::PreflightInProgress::~PreflightInProgress() {} nit: blank line before this
On 2017/07/06 20:04:51, meacer wrote: > On 2017/07/06 06:05:44, estark (OOO until Jul 10) wrote: > > On 2017/07/05 20:50:11, meacer wrote: > > > On 2017/07/05 05:47:33, estark (OOO until Jul 10) wrote: > > > > meacer, can you please review? This is the last major bit of > implementation > > > work > > > > for the Expect-CT header, which I'm planning to ship in M61, so ideally > I'd > > > like > > > > to get this landed before Friday. Thanks! > > > > > > I read the threads (quite a journey down the living quarters of rabbits) and > > I'm > > > still not sure what exactly we are fixing, so please bear with me. > > > > > > Is the concern that Expect-CT reports (and possibly others) violate CORS > > > principles? Or is it that without CORS check web content can send arbitrary > > > report requests? Or both? > > > > > > If it's the former, there seems to be disagreement that browser initiated > > > requests should be considered in scope of SOP. I don't want to trigger a > > rehash > > > of the whole discussion, but it seems the ietf thread reached a consensus > > > without taking those concerns into consideration? (I'm probably missing > > > something here, but still) > > > > > > If the latter, I'm not sure how ACAO:* or ACAO:null prevents sites from > > sending > > > arbitrary reports. * would obviously still allow any site, and null would > > allow > > > data URLs as data urls have null origins. That seems to be defying the > purpose > > > of the CL, or am I missing something here too? > > > > Thanks for going down the IETF rabbit-hole. :) The concern is the former, that > > Expect-CT reports actually violate the CORS spec and threat model. Here are > two > > things that might help explain better: > > > > (1) Expect-CT reports aren't really browser-initiated requests. If you visit > > https://evil.com, it can set an Expect-CT header with a report-uri pointing to > > https://victim.com and subsequently intentionally violate CT policy -- that > > would trigger a request to https://victim.com which http://victim.com might > not be > > expecting. So these requests are not really browser-initiated because they can > > be triggered by web content. > > Well, yes they can be triggered by web content, but so are most browser > initiated requests. However, in this case the page content doesn't have control > over the report's contents which differentiates it from content initiated > requests. But I don't intend to override the decision, just wanted to clarify > the CL's intention. Ah, I see, I was thinking of a different definition of "browser-initiated". (I was thinking of requests that are part of browser features and truly can't be initiated by web content, like a sync request or a request to download an update.) In this case, the web content does have some limited control over the request (for example, it could present a maliciously crafted certificate chain which gets included in the report), but even if it couldn't, really the point is just to protect non-opted-in servers from receiving reports all together, as you say below. > > > > > (2) The threat model of CORS is that existing servers shouldn't be made > > vulnerable. So the specific concern here would be this: suppose there is an > > intranet server https://victim-corp that implicitly trusts data received in a > > `Content-Type: blah` request -- say, maybe the server crashes upon receiving > > such a request unless the request has some particular body. This would be > > considered "safe" behavior for a server, according to the CORS spec, because > > arbitrary web content can't trigger requests to https://victim-corp with > > `Content-Type: blah` headers, unless the server explicitly opts in to > receiving > > them by responding positively to a CORS preflight. The principle of CORS is > that > > the web platform shouldn't just go ahead and add an ability for web content to > > trigger `Content-Type: blah` requests (even if the web content can't control > the > > body or read the response) without the server opting it, because that would > make > > the existing https://victim-corp server vulnerable. Without a preflight, this > is > > exactly what Expect-CT would be doing, except it's > > 'application/expect-ct-report' instead of 'blah'. One could argue that this is > a > > silly threat model: that https://victim.corp is just plain buggy and it's not > > the web platform's responsibility to worry about making it vulnerable. But > this > > is the threat model of CORS, and if we don't think that Expect-CT reports need > > to send preflights, then that's a larger issue that we should take up with the > > CORS spec. (I happen to think we should take it up with the CORS spec, but I > > don't want to block Expect-CT on it; I'd rather send preflights to start with > > and then later stop sending preflights if we can convince other browser > vendors > > that the Content-Type preflight requirement should be dropped from CORS.) > > > > So, the intent of the preflight is to obtain permission from the report > > collection server that it is okay receiving requests with `Content-Type: > > application/expect-ct-report` headers. There isn't an expectation that report > > collection servers would say "yes" to some origins and "no" to others; it's > just > > a way to ensure that the server we are sending reports to expects to receive > > Expect-CT reports at all. > > Right, we are on the same page here. My point was if the server opts in to > receiving reports, this won't prevent arbitrary pages from sending those > reports. But it looks like that's not what this change is for, it instead > protects not-opted-in servers, which is also totally fine.
https://codereview.chromium.org/2970913002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/2970913002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_expect_ct_reporter.cc:220: (response_code < 200 || response_code > 299)) { On 2017/07/06 22:03:45, meacer wrote: > tiny nit: Extra parans not necessary since it's all ||. Done. https://codereview.chromium.org/2970913002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_expect_ct_reporter.cc:258: ChromeExpectCTReporter::PreflightInProgress::~PreflightInProgress() {} On 2017/07/06 22:03:45, meacer wrote: > nit: blank line before this Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2970913002/#ps40001 (title: "meacer nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499407646864060,
"parent_rev": "6de258a1803009bb76b7d1dbce0f99c45dd67d39", "commit_rev":
"70adceb53e4a7c7d1cbfa080ab6fd4d6ae2878c6"}
Message was sent while issue was closed.
Description was changed from ========== Implement CORS preflights for Expect-CT reports Expect-CT reports are sent with a Content-Type header of application/expect-ct-report+json. This Content-Type (nor "application/json") is not CORS safelisted, meaning that Chrome arguably ought to send CORS preflights to make sure the designated report collection server has opted in to receive Expect-CT reports. Otherwise, web content would be able to trigger reports to arbitrary endpoints with a non-safelisted Content-Type header. Therefore, this CL implements CORS preflight requests before sending reports. The wrinkle is that Expect-CT is checked at connection setup time, before being associated with a particular URLRequest, much less an initiating origin, so we cannot construct a proper Origin header to include in the preflight request. Instead, we set the Origin header to "null", and expect `Access-Control-Allow-Origin: *` or `Access-Control-Allow-Origin: null` in response. While this is a bit weird, it is safe because reports are sent without credentials and it requires the server to opt in to receiving reports. See https://lists.w3.org/Archives/Public/ietf-http-wg/2017AprJun/0168.html for more discussion on the CORS issues and background on why we have settled on sending `Origin: null` preflights. https://fetch.spec.whatwg.org/#cors-preflight-fetch describes the preflights implemented in this CL. BUG=679012 ========== to ========== Implement CORS preflights for Expect-CT reports Expect-CT reports are sent with a Content-Type header of application/expect-ct-report+json. This Content-Type (nor "application/json") is not CORS safelisted, meaning that Chrome arguably ought to send CORS preflights to make sure the designated report collection server has opted in to receive Expect-CT reports. Otherwise, web content would be able to trigger reports to arbitrary endpoints with a non-safelisted Content-Type header. Therefore, this CL implements CORS preflight requests before sending reports. The wrinkle is that Expect-CT is checked at connection setup time, before being associated with a particular URLRequest, much less an initiating origin, so we cannot construct a proper Origin header to include in the preflight request. Instead, we set the Origin header to "null", and expect `Access-Control-Allow-Origin: *` or `Access-Control-Allow-Origin: null` in response. While this is a bit weird, it is safe because reports are sent without credentials and it requires the server to opt in to receiving reports. See https://lists.w3.org/Archives/Public/ietf-http-wg/2017AprJun/0168.html for more discussion on the CORS issues and background on why we have settled on sending `Origin: null` preflights. https://fetch.spec.whatwg.org/#cors-preflight-fetch describes the preflights implemented in this CL. BUG=679012 Review-Url: https://codereview.chromium.org/2970913002 Cr-Commit-Position: refs/heads/master@{#484849} Committed: https://chromium.googlesource.com/chromium/src/+/70adceb53e4a7c7d1cbfa080ab6f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/70adceb53e4a7c7d1cbfa080ab6f...
Message was sent while issue was closed.
On 2017/07/07 07:09:35, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) as > https://chromium.googlesource.com/chromium/src/+/70adceb53e4a7c7d1cbfa080ab6f... Findit suggests this CL introduced a flaky test "ChromeExpectCTReporterTest.BadCORSPreflightResponseOrigin" according to analysis https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... Can someone please help verify? Thanks, Jeff on behalf of Findit team
Message was sent while issue was closed.
On 2017/07/14 18:53:37, lijeffrey wrote: > On 2017/07/07 07:09:35, commit-bot: I haz the power wrote: > > Committed patchset #3 (id:40001) as > > > https://chromium.googlesource.com/chromium/src/+/70adceb53e4a7c7d1cbfa080ab6f... > > Findit suggests this CL introduced a flaky test > "ChromeExpectCTReporterTest.BadCORSPreflightResponseOrigin" according to > analysis > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... > > Can someone please help verify? > > Thanks, > Jeff on behalf of Findit team ^ I'm looking into it |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
