Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2869423002:
PlzNavigate: Do not disclose urls between cross-origin renderers. (Closed)
|
Created:
3 years, 7 months ago by arthursonzogni Modified:
3 years, 7 months ago CC:
blink-reviews, chromium-reviews, creis+watch_chromium.org, Charlie Reis, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, nasko, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Do not disclose urls between cross-origin renderers.
The browser transmits Urls between potentially separate, cross-origin renderers
in RenderFrameHostImpl::ReportContentSecurityPolicyViolation.
It is bad from a security point of view when one of the renderer is
compromised.
This CL prevent the browser to send the full path of the urls when they
are cross-origin.
It regresses the quality of some console messages.
BUG=718942
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel;master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2869423002
Cr-Commit-Position: refs/heads/master@{#472433}
Committed: https://chromium.googlesource.com/chromium/src/+/74a80d7c654a53bbb1ad714d8d86c60af1646573
Patch Set 1 #
Total comments: 23
Patch Set 2 : alexmos@ suggestions. #
Total comments: 15
Patch Set 3 : Applying suggestions. #
Total comments: 16
Patch Set 4 : Addressed comments #Patch Set 5 : Add [ Failure ] for virtual/off-main-thread-fetch/[...]/onload-detach-during-csp-frame-src-none.html #
Messages
Total messages: 42 (29 generated)
Description was changed from ========== PlzNavigate: Do not disclose urls between cross-origin renderers. The browser transmits Urls between potentially separate, cross-origin renderers in RenderFrameHostImpl::ReportContentSecurityPolicyViolation. It is bad from a security point of view when one of the renderer is compromised. This CL prevent the browser to send the full path of the urls when they are cross-origin. It regresses the quality of some console messages. BUG=718942 ========== to ========== PlzNavigate: Do not disclose urls between cross-origin renderers. The browser transmits Urls between potentially separate, cross-origin renderers in RenderFrameHostImpl::ReportContentSecurityPolicyViolation. It is bad from a security point of view when one of the renderer is compromised. This CL prevent the browser to send the full path of the urls when they are cross-origin. It regresses the quality of some console messages. BUG=718942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate: Do not disclose urls between cross-origin renderers. The browser transmits Urls between potentially separate, cross-origin renderers in RenderFrameHostImpl::ReportContentSecurityPolicyViolation. It is bad from a security point of view when one of the renderer is compromised. This CL prevent the browser to send the full path of the urls when they are cross-origin. It regresses the quality of some console messages. BUG=718942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Do not disclose urls between cross-origin renderers. The browser transmits Urls between potentially separate, cross-origin renderers in RenderFrameHostImpl::ReportContentSecurityPolicyViolation. It is bad from a security point of view when one of the renderer is compromised. This CL prevent the browser to send the full path of the urls when they are cross-origin. It regresses the quality of some console messages. BUG=718942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
The CQ bit was checked by arthursonzogni@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
alexmos@chromium.org changed reviewers: + alexmos@chromium.org
Hey Arthur, Nasko asked me to preemptively start reviewing this CL to decrease some of the latency, so here are some comments. (Sorry if you were already working on some of them!) The overall approach looks good, and I'm hoping the bot failures are just flakes. Thanks for working on this! https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:47: GURL safe_url = context->IsOriginSafeToUseInCspViolation(url::Origin(url)) It feels like we actually want to ask this question about the URL rather than origin. I.e., origins are always safe to use -- it's the URL that we sometimes want to strip to an origin. So what about changing this to something like IsURLSafeForCspViolation, passing in the whole URL, and letting RFHI compare origins internally? https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:49: : url.GetOrigin(); Both of these checks could use a comment explaining the rationale. https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:55: : SourceLocation(); Interesting, so we clear it out entirely? This doesn't seem to match what the renderer-side check is doing, which is only changing the source_location.url to an origin and leaving the line/column numbers there (is that right?). If we end up changing the behavior here (e.g., not reporting line/column numbers anymore), let's get mkwst@'s confirmation for it. I can see that we might want to stop leaking cross-origin line/column numbers, but it might still make sense to include the source_location.url stripped to an origin. https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/csp_context.h:59: // messages, source location and reports. When this function returns false, Note: using those URLs in console error messages is fine/fixable, as long as only the devtools process knows them, and we don't pass them through the renderer process (see by comment on the disabled tests below). https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/csp_context.h:60: // only the url's origin is displayed instead. This latter part doesn't apply to SourceLocation, which you clear out entirely, so perhaps the wording can account for that? (I.e., it can clarify that it's the blocked_url in violation events that is converted to origins.) https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... File content/common/content_security_policy/csp_context_unittest.cc (right): https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/csp_context_unittest.cc:46: std::vector<url::Origin> origins_unsafe_to_use_in_csp_violation_; Why not std::set? That'll also let you use origins_unsafe_to_use_in_csp_violation_.find(origin) == origins_unsafe_to_use_in_csp_violation_.end(). https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation (right): https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:22: # PlzNavigate: URLs are potentially disclosed across cross-origin renderers I'd also mention that this is about CSP violations somewhere in the comment. https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:23: # Not to disclose |source_location| and/or |blocked url| between cross-origin nit: s/to disclose/disclosing/ https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:24: # renderers regresses the quality of error messages. Just so I understand why the error messages regress. Is it because we now print the origin rather than full URL in the expected console errors, because you also changed |message| to use |safe_url| in ReportViolation? Whereas on the renderer side, we used to only strip the URL to the origin for CSP violation events, but not for console messages, which were considered ok since they went to the devtools process? Nasko and I were discussing that we should just directly print the console error from the browser process without passing it through the renderer process, which would allow us to fix this -- perhaps it's worth filing a bug to do this in a followup.
Description was changed from ========== PlzNavigate: Do not disclose urls between cross-origin renderers. The browser transmits Urls between potentially separate, cross-origin renderers in RenderFrameHostImpl::ReportContentSecurityPolicyViolation. It is bad from a security point of view when one of the renderer is compromised. This CL prevent the browser to send the full path of the urls when they are cross-origin. It regresses the quality of some console messages. BUG=718942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Do not disclose urls between cross-origin renderers. The browser transmits Urls between potentially separate, cross-origin renderers in RenderFrameHostImpl::ReportContentSecurityPolicyViolation. It is bad from a security point of view when one of the renderer is compromised. This CL prevent the browser to send the full path of the urls when they are cross-origin. It regresses the quality of some console messages. BUG=718942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel;master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by arthursonzogni@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...
arthursonzogni@chromium.org changed reviewers: + mkwst@chromium.org
Thanks alexmos@! Bot failure were just flake except of 1 more test that has regressed. Hi mkwst@, please take a look at this CL. https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:47: GURL safe_url = context->IsOriginSafeToUseInCspViolation(url::Origin(url)) On 2017/05/10 22:33:08, alexmos wrote: > It feels like we actually want to ask this question about the URL rather than > origin. I.e., origins are always safe to use -- it's the URL that we sometimes > want to strip to an origin. So what about changing this to something like > IsURLSafeForCspViolation, passing in the whole URL, and letting RFHI compare > origins internally? Yes, that is what I did initially. But even if the url doesn't contains any sensitive data. I still would like to clear the line/column number. I will try to rephrase this. ShouldProtectDataInCspViolation(...)? It is hard to make it short and meaningful. https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:49: : url.GetOrigin(); On 2017/05/10 22:33:08, alexmos wrote: > Both of these checks could use a comment explaining the rationale. Done. https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:55: : SourceLocation(); On 2017/05/10 22:33:08, alexmos wrote: > Interesting, so we clear it out entirely? This doesn't seem to match what the > renderer-side check is doing, which is only changing the source_location.url to > an origin and leaving the line/column numbers there (is that right?). If we end > up changing the behavior here (e.g., not reporting line/column numbers anymore), > let's get mkwst@'s confirmation for it. I can see that we might want to stop > leaking cross-origin line/column numbers, but it might still make sense to > include the source_location.url stripped to an origin. I think we have to clear the line/column numbers because it might also be a sensitive information. It tells an attacker what line of JavaScript code triggers the navigation. mkwst@: what do you think of it? I don't know what is the best thing between: * SourceLocation() * SourceLocation(source_location.url.GetOrigin(), 0, 0) [0 means unknown] In the next patch, I will use the second one. https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/csp_context.h:59: // messages, source location and reports. When this function returns false, On 2017/05/10 22:33:08, alexmos wrote: > Note: using those URLs in console error messages is fine/fixable, as long as > only the devtools process knows them, and we don't pass them through the > renderer process (see by comment on the disabled tests below). Yes, using those URLs will be fine as soon as we will be able to send console error message directly to the devtool process without using the renderer process in between. I don't know how hard it is. https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/csp_context.h:60: // only the url's origin is displayed instead. On 2017/05/10 22:33:08, alexmos wrote: > This latter part doesn't apply to SourceLocation, which you clear out entirely, > so perhaps the wording can account for that? (I.e., it can clarify that it's > the blocked_url in violation events that is converted to origins.) source_location is used in console message and in reports. I need to fix this comment. It looks weird. https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... File content/common/content_security_policy/csp_context_unittest.cc (right): https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/csp_context_unittest.cc:46: std::vector<url::Origin> origins_unsafe_to_use_in_csp_violation_; On 2017/05/10 22:33:08, alexmos wrote: > Why not std::set? That'll also let you use > origins_unsafe_to_use_in_csp_violation_.find(origin) == > origins_unsafe_to_use_in_csp_violation_.end(). You are absolutely right. https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation (right): https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:22: # PlzNavigate: URLs are potentially disclosed across cross-origin renderers On 2017/05/10 22:33:08, alexmos wrote: > I'd also mention that this is about CSP violations somewhere in the comment. Done. https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:23: # Not to disclose |source_location| and/or |blocked url| between cross-origin On 2017/05/10 22:33:08, alexmos wrote: > nit: s/to disclose/disclosing/ Done. https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:24: # renderers regresses the quality of error messages. On 2017/05/10 22:33:08, alexmos wrote: > Just so I understand why the error messages regress. Is it because we now print > the origin rather than full URL in the expected console errors, because you also > changed |message| to use |safe_url| in ReportViolation? Whereas on the renderer > side, we used to only strip the URL to the origin for CSP violation events, but > not for console messages, which were considered ok since they went to the > devtools process? Nasko and I were discussing that we should just directly > print the console error from the browser process without passing it through the > renderer process, which would allow us to fix this -- perhaps it's worth filing > a bug to do this in a followup. Here are the diff for the 3 tests that have regressed: ###[ form-action-src-get-blocked-with-redirect.html ]### -CONSOLE ERROR: Refused to send form data to 'http://localhost:8000/navigation/resources/form-target.pl' because it violates the following Content Security Policy directive: "form-action 127.0.0.1:8000". +CONSOLE ERROR: Refused to send form data to 'http://localhost:8000/' because it violates the following Content Security Policy directive: "form-action 127.0.0.1:8000". ###[ form-action-src-redirect-blocked.html ]### -CONSOLE ERROR: Refused to send form data to 'http://localhost:8000/navigation/resources/form-target.pl' because it violates the following Content Security Policy directive: "form-action 127.0.0.1:8000". +CONSOLE ERROR: Refused to send form data to 'http://localhost:8000/' because it violates the following Content Security Policy directive: "form-action 127.0.0.1:8000". ###[ frame-src-child-frame-navigates-to-blocked-origin.html ] ### + FAIL The frame's navigation is blocked. assert_equals: This shouldn't actually be 7, since it happens in a cross-origin frame. :/ expected 7 but got 0 For the first two, it is because we print the origin instead of the full url in console error messages. For the last one, it is because the source_location line number has been removed. By looking at the it more closely, it looks like what we have with PlzNavigate is better :-) There is one more test that has regressed: ###[ http/tests/misc/onload-detach-during-csp-frame-src-none.html ]### -CONSOLE ERROR: Refused to frame 'data:,hello' because it violates the following Content Security Policy directive: "frame-src 'none'". +CONSOLE ERROR: Refused to frame '' because it violates the following Content Security Policy directive: "frame-src 'none'". I don't really know what to think of this one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:47: GURL safe_url = context->IsOriginSafeToUseInCspViolation(url::Origin(url)) On 2017/05/11 13:06:23, arthursonzogni wrote: > On 2017/05/10 22:33:08, alexmos wrote: > > It feels like we actually want to ask this question about the URL rather than > > origin. I.e., origins are always safe to use -- it's the URL that we > sometimes > > want to strip to an origin. So what about changing this to something like > > IsURLSafeForCspViolation, passing in the whole URL, and letting RFHI compare > > origins internally? > > Yes, that is what I did initially. But even if the url doesn't contains any > sensitive data. I still would like to clear the line/column number. > > I will try to rephrase this. ShouldProtectDataInCspViolation(...)? It is hard to > make it short and meaningful. Yeah, it's hard, and I guess it also depends on what we decide with line/column numbers (see below). Perhaps we could further clarify this to ShouldSanitizeDataInCspViolation? Another option is to let CSPContext do this internally and call this StripURLForUseInReport, which would take care of any necessary stripping internally. This is also what Blink does today. This is nice because you end up doing the stripping only in one place. But I guess you'd also need StripSourceLocationForUseInReport if we end up also doing line/column clearing here (which can call StripURLForUseInReport internally for the source_location.url). https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:55: : SourceLocation(); On 2017/05/11 13:06:23, arthursonzogni wrote: > On 2017/05/10 22:33:08, alexmos wrote: > > Interesting, so we clear it out entirely? This doesn't seem to match what the > > renderer-side check is doing, which is only changing the source_location.url > to > > an origin and leaving the line/column numbers there (is that right?). If we > end > > up changing the behavior here (e.g., not reporting line/column numbers > anymore), > > let's get mkwst@'s confirmation for it. I can see that we might want to stop > > leaking cross-origin line/column numbers, but it might still make sense to > > include the source_location.url stripped to an origin. > > I think we have to clear the line/column numbers because it might also be a > sensitive information. It tells an attacker what line of JavaScript code > triggers the navigation. mkwst@: what do you think of it? > > I don't know what is the best thing between: > * SourceLocation() > * SourceLocation(source_location.url.GetOrigin(), 0, 0) [0 means unknown] > > In the next patch, I will use the second one. I'll leave this up to Mike. I agree it seems more desirable to also clear out the line/column number, but that seems like a web-facing change in how CSP works which might need to go through the blink intent process. Whereas stripping URLs to origins is something that we already do in cross-origin cases. So, it might be better to leave the line/column numbers alone (i.e., present) in this CL, so that there's no behavior change here, and then put together another CL to further strip them away and have this discussion there? https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation (right): https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:24: # renderers regresses the quality of error messages. On 2017/05/11 13:06:24, arthursonzogni wrote: > On 2017/05/10 22:33:08, alexmos wrote: > > Just so I understand why the error messages regress. Is it because we now > print > > the origin rather than full URL in the expected console errors, because you > also > > changed |message| to use |safe_url| in ReportViolation? Whereas on the > renderer > > side, we used to only strip the URL to the origin for CSP violation events, > but > > not for console messages, which were considered ok since they went to the > > devtools process? Nasko and I were discussing that we should just directly > > print the console error from the browser process without passing it through > the > > renderer process, which would allow us to fix this -- perhaps it's worth > filing > > a bug to do this in a followup. > > Here are the diff for the 3 tests that have regressed: > > ###[ form-action-src-get-blocked-with-redirect.html ]### > -CONSOLE ERROR: Refused to send form data to > 'http://localhost:8000/navigation/resources/form-target.pl' because it violates > the following Content Security Policy directive: "form-action 127.0.0.1:8000". > +CONSOLE ERROR: Refused to send form data to 'http://localhost:8000/' because it > violates the following Content Security Policy directive: "form-action > 127.0.0.1:8000". Hmm, this raises an interesting point, and perhaps another thing for Mike to comment on. The current stripping logic in Blink's StripURLForUseInReport, if I read it correctly, only applies to frame-src and object-src, whereas this CL will also apply it to form-action. Do we actually need to strip URLs for form-action reports (where this isn't really an information leak from one renderer to another)? > > ###[ form-action-src-redirect-blocked.html ]### > -CONSOLE ERROR: Refused to send form data to > 'http://localhost:8000/navigation/resources/form-target.pl' because it violates > the following Content Security Policy directive: "form-action 127.0.0.1:8000". > +CONSOLE ERROR: Refused to send form data to 'http://localhost:8000/' because it > violates the following Content Security Policy directive: "form-action > 127.0.0.1:8000". > > ###[ frame-src-child-frame-navigates-to-blocked-origin.html ] ### > + FAIL The frame's navigation is blocked. assert_equals: This shouldn't actually > be 7, since it happens in a cross-origin frame. :/ expected 7 but got 0 > > For the first two, it is because we print the origin instead of the full url in > console error messages. > For the last one, it is because the source_location line number has been > removed. By looking at the it more closely, it looks like what we have with > PlzNavigate is better :-) > > There is one more test that has regressed: > ###[ http/tests/misc/onload-detach-during-csp-frame-src-none.html ]### > -CONSOLE ERROR: Refused to frame 'data:,hello' because it violates the following > Content Security Policy directive: "frame-src 'none'". > +CONSOLE ERROR: Refused to frame '' because it violates the following Content > Security Policy directive: "frame-src 'none'". > > I don't really know what to think of this one. Is last one because we strip the data URL down to an origin, which is a unique origin? Though then I would've expected it to print as "null". https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:49: // ensure that these information are not transmitted between different nit: "these information are" -> "these are" (or "this information is" https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:50: // cross-origin renderers. Perhaps also include a reference to https://crbug.com/633306? https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/csp_context.h:58: // For security reasons, some urls must not be disclosed. It includes the nit: disclosed -> disclosed cross-origin in violation reports. https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/csp_context.h:60: // informations are potentially transmitted between different renderer nit: These informations are -> This information is https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/csp_context.h:63: // TODO(arthursonzogni): Stop hidding sensitive parts of URLs in console nit: s/hidding/hiding/ https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/csp_context.h:64: // error message as soon as there is a way to send it to the devtool process nit: s/message/messages/ s/devtool/devtools/
On 2017/05/12 at 01:37:20, alexmos wrote: > I'll leave this up to Mike. I agree it seems more desirable to also clear out the line/column number, but that seems like a web-facing change in how CSP works which might need to go through the blink intent process. Whereas stripping URLs to origins is something that we already do in cross-origin cases. So, it might be better to leave the line/column numbers alone (i.e., present) in this CL, so that there's no behavior change here, and then put together another CL to further strip them away and have this discussion there? I think that sounds reasonable, but I also agree that we probably shouldn't be providing line/column numbers if the navigation is initiated from the other document. > Hmm, this raises an interesting point, and perhaps another thing for Mike to comment on. The current stripping logic in Blink's StripURLForUseInReport, if I read it correctly, only applies to frame-src and object-src, whereas this CL will also apply it to form-action. Do we actually need to strip URLs for form-action reports (where this isn't really an information leak from one renderer to another)? In an ideal world, we'd always report the URL that the page already knows, as opposed to the URL that results after redirects. I simply haven't done the work to pipe that through to the response. To do that safely, we also need to know which page initiated the navigation. That is, it's safe for me to learn that a full URL that I initiated violated my policy. It's significantly less safe for me to learn that someone else's navigation in a frame I control violated the policy. Until we can get that data, we're stripping everything to an origin. As you note, I don't believe the same rationale applies to `form-action`. > > For the last one, it is because the source_location line number has been > > removed. By looking at the it more closely, it looks like what we have with > > PlzNavigate is better :-) Better is gooder? :) > > There is one more test that has regressed: > > ###[ http/tests/misc/onload-detach-during-csp-frame-src-none.html ]### > > -CONSOLE ERROR: Refused to frame 'data:,hello' because it violates the following > > Content Security Policy directive: "frame-src 'none'". > > +CONSOLE ERROR: Refused to frame '' because it violates the following Content > > Security Policy directive: "frame-src 'none'". > > > > I don't really know what to think of this one. > > Is last one because we strip the data URL down to an origin, which is a unique origin? Though then I would've expected it to print as "null". If we change the redirect behavior as above, I think this will fix itself (as `data:` URLs are not valid redirect targets). https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:53: : url; I think we can skip this if |is_redirect| is false (because if it's not a redirect, the initiating renderer already knows the URL, since it created the request. I suspect that will solve some of the changed tests.
The CQ bit was checked by arthursonzogni@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...
Thanks for the reviews! I made some changes taking your comments into account. On 2017/05/12 14:16:44, Mike West wrote: > On 2017/05/12 at 01:37:20, alexmos wrote: > > I'll leave this up to Mike. I agree it seems more desirable to also clear out > the line/column number, but that seems like a web-facing change in how CSP works > which might need to go through the blink intent process. Whereas stripping URLs > to origins is something that we already do in cross-origin cases. So, it might > be better to leave the line/column numbers alone (i.e., present) in this CL, so > that there's no behavior change here, and then put together another CL to > further strip them away and have this discussion there? > > I think that sounds reasonable, but I also agree that we probably shouldn't be > providing line/column numbers if the navigation is initiated from the other > document. Do we want to leave the line/number present for the moment and maybe clearing it in a future CL? > > Hmm, this raises an interesting point, and perhaps another thing for Mike to > comment on. The current stripping logic in Blink's StripURLForUseInReport, if I > read it correctly, only applies to frame-src and object-src, whereas this CL > will also apply it to form-action. Do we actually need to strip URLs for > form-action reports (where this isn't really an information leak from one > renderer to another)? > > In an ideal world, we'd always report the URL that the page already knows, as > opposed to the URL that results after redirects. I simply haven't done the work > to pipe that through to the response. > > To do that safely, we also need to know which page initiated the navigation. > That is, it's safe for me to learn that a full URL that I initiated violated my > policy. It's significantly less safe for me to learn that someone else's > navigation in a frame I control violated the policy. Until we can get that data, > we're stripping everything to an origin. > > As you note, I don't believe the same rationale applies to `form-action`. > > > > For the last one, it is because the source_location line number has been > > > removed. By looking at the it more closely, it looks like what we have with > > > PlzNavigate is better :-) > > Better is gooder? :) I am not sure to really understand, sorry :) In frame-src-child-frame-navigates-to-blocked-origin.html, there is a comment: "This shouldn't actually be 7, since it happens in a cross-origin frame. :/" It is better since now we don't leak the source location. The subframe navigates by itself and we don't want the main frame to know about the source-location. > > > > There is one more test that has regressed: > > > ###[ http/tests/misc/onload-detach-during-csp-frame-src-none.html ]### > > > -CONSOLE ERROR: Refused to frame 'data:,hello' because it violates the > following > > > Content Security Policy directive: "frame-src 'none'". > > > +CONSOLE ERROR: Refused to frame '' because it violates the following > Content > > > Security Policy directive: "frame-src 'none'". > > > > > > I don't really know what to think of this one. > > > > Is last one because we strip the data URL down to an origin, which is a unique > origin? Though then I would've expected it to print as "null". > > If we change the redirect behavior as above, I think this will fix itself (as > `data:` URLs are not valid redirect targets). I changed the redirect behavior only for 'form-action'. So it is still not fixed. Maybe we should display "data: [...]" instead. Do you think ``` if (url::Origin(url).unique()) source_location.url = url.scheme() + "[...]" else source_location.url = url.GetOrigin().spec(); // + " [...]" maybe ``` for sanitizing is the right thing to do? > https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... > File content/common/content_security_policy/content_security_policy.cc (right): > > https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... > content/common/content_security_policy/content_security_policy.cc:53: : url; > I think we can skip this if |is_redirect| is false (because if it's not a > redirect, the initiating renderer already knows the URL, since it created the > request. I suspect that will solve some of the changed tests. Done, but only for 'form-action'. Please see the explanation below. https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/1/content/common/content_secu... content/common/content_security_policy/content_security_policy.cc:47: GURL safe_url = context->IsOriginSafeToUseInCspViolation(url::Origin(url)) On 2017/05/12 01:37:19, alexmos wrote: > On 2017/05/11 13:06:23, arthursonzogni wrote: > > On 2017/05/10 22:33:08, alexmos wrote: > > > It feels like we actually want to ask this question about the URL rather > than > > > origin. I.e., origins are always safe to use -- it's the URL that we > > sometimes > > > want to strip to an origin. So what about changing this to something like > > > IsURLSafeForCspViolation, passing in the whole URL, and letting RFHI compare > > > origins internally? > > > > Yes, that is what I did initially. But even if the url doesn't contains any > > sensitive data. I still would like to clear the line/column number. > > > > I will try to rephrase this. ShouldProtectDataInCspViolation(...)? It is hard > to > > make it short and meaningful. > > Yeah, it's hard, and I guess it also depends on what we decide with line/column > numbers (see below). Perhaps we could further clarify this to > ShouldSanitizeDataInCspViolation? > > Another option is to let CSPContext do this internally and call this > StripURLForUseInReport, which would take care of any necessary stripping > internally. This is also what Blink does today. This is nice because you end > up doing the stripping only in one place. But I guess you'd also need > StripSourceLocationForUseInReport if we end up also doing line/column clearing > here (which can call StripURLForUseInReport internally for the > source_location.url). > Thanks for "ShouldSanitizeDataInCspViolation". I will apply your suggestion to strip urls in the CSPContext instead. https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation (right): https://codereview.chromium.org/2869423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:24: # renderers regresses the quality of error messages. Yes, it is because the data URL origin is an unique origin. https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:49: // ensure that these information are not transmitted between different On 2017/05/12 01:37:20, alexmos wrote: > nit: "these information are" -> "these are" (or "this information is" Done. https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:50: // cross-origin renderers. On 2017/05/12 01:37:20, alexmos wrote: > Perhaps also include a reference to https://crbug.com/633306 Done (In RenderFrameHostImpl::SanitizeDataForUseInCspViolation) https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:53: : url; On 2017/05/12 14:16:44, Mike West wrote: > I think we can skip this if |is_redirect| is false (because if it's not a > redirect, the initiating renderer already knows the URL, since it created the > request. I suspect that will solve some of the changed tests. For form-action: I think you are right. For frame-src: When a cross-origin subframe navigates by itself, I think the main frame doesn't know (and mustn't know) the url. I think we can skip if |is_redirect| is false and |directive| is form-action. https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/csp_context.h:58: // For security reasons, some urls must not be disclosed. It includes the On 2017/05/12 01:37:20, alexmos wrote: > nit: disclosed -> disclosed cross-origin in violation reports. Done. https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/csp_context.h:60: // informations are potentially transmitted between different renderer On 2017/05/12 01:37:20, alexmos wrote: > nit: These informations are -> This information is Done. https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/csp_context.h:63: // TODO(arthursonzogni): Stop hidding sensitive parts of URLs in console On 2017/05/12 01:37:20, alexmos wrote: > nit: s/hidding/hiding/ Done. https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/csp_context.h:64: // error message as soon as there is a way to send it to the devtool process On 2017/05/12 01:37:20, alexmos wrote: > nit: s/message/messages/ > s/devtool/devtools/ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! I'm happy with content/ -- just a few more minor things below. On 2017/05/15 12:20:47, arthursonzogni wrote: > Thanks for the reviews! > I made some changes taking your comments into account. > > On 2017/05/12 14:16:44, Mike West wrote: > > On 2017/05/12 at 01:37:20, alexmos wrote: > > > I'll leave this up to Mike. I agree it seems more desirable to also clear > out > > the line/column number, but that seems like a web-facing change in how CSP > works > > which might need to go through the blink intent process. Whereas stripping > URLs > > to origins is something that we already do in cross-origin cases. So, it > might > > be better to leave the line/column numbers alone (i.e., present) in this CL, > so > > that there's no behavior change here, and then put together another CL to > > further strip them away and have this discussion there? > > > > I think that sounds reasonable, but I also agree that we probably shouldn't be > > providing line/column numbers if the navigation is initiated from the other > > document. > > Do we want to leave the line/number present for the moment and maybe clearing it > in a future CL? If Mike is fine doing it in this CL, I'm fine with it too. :) https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:53: : url; On 2017/05/15 12:20:46, arthursonzogni wrote: > On 2017/05/12 14:16:44, Mike West wrote: > > I think we can skip this if |is_redirect| is false (because if it's not a > > redirect, the initiating renderer already knows the URL, since it created the > > request. I suspect that will solve some of the changed tests. > > For form-action: I think you are right. > For frame-src: When a cross-origin subframe navigates by itself, I think the > main frame doesn't know (and mustn't know) the url. > > I think we can skip if |is_redirect| is false and |directive| is form-action. Ack - I think that makes sense to me. Mike, do you agree? https://codereview.chromium.org/2869423002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2869423002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:986: // The main goal of this is to avoid leaking informations between potentially nit: s/informations/information/ https://codereview.chromium.org/2869423002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2869423002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:222: GURL* blocked_url, nit: reorder params so that inputs come before outputs. https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:53: // For security reasons, some urls must not be disclosed. It includes the nit: It includes -> This includes (also below) https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context.h:63: // messages as soon as there is a way to send it to the devtools process nit: s/it/them/ https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... File content/common/content_security_policy/csp_context_unittest.cc (right): https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context_unittest.cc:18: const CSPViolationParams& LastViolation() { return last_violation_; } nit: last_violation() https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context_unittest.cc:28: void SetSanitizeDataForUseInCspViolation(bool value) { nit: can use lowercase_with_underscores() for simple setter https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context_unittest.cc:50: SourceLocation source_location_; nit: looks unused? https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context_unittest.cc:52: bool sanitize_data_for_use_in_csp_violation = false; nit: end member var name with _
On 2017/05/16 at 05:56:49, alexmos wrote: > Thanks! I'm happy with content/ -- just a few more minor things below. > > On 2017/05/15 12:20:47, arthursonzogni wrote: > > Thanks for the reviews! > > I made some changes taking your comments into account. > > > > On 2017/05/12 14:16:44, Mike West wrote: > > > On 2017/05/12 at 01:37:20, alexmos wrote: > > > > I'll leave this up to Mike. I agree it seems more desirable to also clear > > out > > > the line/column number, but that seems like a web-facing change in how CSP > > works > > > which might need to go through the blink intent process. Whereas stripping > > URLs > > > to origins is something that we already do in cross-origin cases. So, it > > might > > > be better to leave the line/column numbers alone (i.e., present) in this CL, > > so > > > that there's no behavior change here, and then put together another CL to > > > further strip them away and have this discussion there? > > > > > > I think that sounds reasonable, but I also agree that we probably shouldn't be > > > providing line/column numbers if the navigation is initiated from the other > > > document. > > > > Do we want to leave the line/number present for the moment and maybe clearing it > > in a future CL? > > If Mike is fine doing it in this CL, I'm fine with it too. :) I'm fine with it, either in this CL or a subsequent CL. > https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... > File content/common/content_security_policy/content_security_policy.cc (right): > > https://codereview.chromium.org/2869423002/diff/20001/content/common/content_... > content/common/content_security_policy/content_security_policy.cc:53: : url; > On 2017/05/15 12:20:46, arthursonzogni wrote: > > On 2017/05/12 14:16:44, Mike West wrote: > > > I think we can skip this if |is_redirect| is false (because if it's not a > > > redirect, the initiating renderer already knows the URL, since it created the > > > request. I suspect that will solve some of the changed tests. > > > > For form-action: I think you are right. > > For frame-src: When a cross-origin subframe navigates by itself, I think the > > main frame doesn't know (and mustn't know) the url. > > > > I think we can skip if |is_redirect| is false and |directive| is form-action. > > Ack - I think that makes sense to me. Mike, do you agree? I do.
The CQ bit was checked by arthursonzogni@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...
Here is a new patch that fixes the nits. https://codereview.chromium.org/2869423002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2869423002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:986: // The main goal of this is to avoid leaking informations between potentially On 2017/05/16 05:56:48, alexmos wrote: > nit: s/informations/information/ Done. https://codereview.chromium.org/2869423002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2869423002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:222: GURL* blocked_url, On 2017/05/16 05:56:49, alexmos wrote: > nit: reorder params so that inputs come before outputs. Done. https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... File content/common/content_security_policy/content_security_policy.cc (right): https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/content_security_policy.cc:53: // For security reasons, some urls must not be disclosed. It includes the On 2017/05/16 05:56:49, alexmos wrote: > nit: It includes -> This includes (also below) Done. https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context.h:63: // messages as soon as there is a way to send it to the devtools process On 2017/05/16 05:56:49, alexmos wrote: > nit: s/it/them/ Done. https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... File content/common/content_security_policy/csp_context_unittest.cc (right): https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context_unittest.cc:18: const CSPViolationParams& LastViolation() { return last_violation_; } On 2017/05/16 05:56:49, alexmos wrote: > nit: last_violation() Done. https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context_unittest.cc:28: void SetSanitizeDataForUseInCspViolation(bool value) { On 2017/05/16 05:56:49, alexmos wrote: > nit: can use lowercase_with_underscores() for simple setter Done. https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context_unittest.cc:50: SourceLocation source_location_; On 2017/05/16 05:56:49, alexmos wrote: > nit: looks unused? Yes! https://codereview.chromium.org/2869423002/diff/40001/content/common/content_... content/common/content_security_policy/csp_context_unittest.cc:52: bool sanitize_data_for_use_in_csp_violation = false; On 2017/05/16 05:56:49, alexmos wrote: > nit: end member var name with _ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, LGTM. There are some failures on the PlzNav Linux bot, but hopefully those are unrelated.
LGTM
The CQ bit was checked by arthursonzogni@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: This issue passed the CQ dry run.
Thanks for the review alexmos@ and mkwst@! On 2017/05/16 16:01:16, alexmos wrote: > Thanks, LGTM. There are some failures on the PlzNav Linux bot, but hopefully > those are unrelated. Yes, it's 99% true. In the last patch I had to add a new test regression. * virtual/off-main-thread-fetch/[...]/onload-detach-during-csp-frame-src-none.html It was already here, but without the "virtual/off-main-thread-fetch" extension. There are also a lot of regressions unrelated to this patch. I filled bugs for them: https://crbug.com/723595 https://crbug.com/723602 https://crbug.com/713388
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2869423002/#ps80001 (title: "Add [ Failure ] for virtual/off-main-thread-fetch/[...]/onload-detach-during-csp-frame-src-none.html")
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": 80001, "attempt_start_ts": 1495024924458680,
"parent_rev": "9f7c63b2876bc23a75c29c107dba98ba61e23fd0", "commit_rev":
"74a80d7c654a53bbb1ad714d8d86c60af1646573"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Do not disclose urls between cross-origin renderers. The browser transmits Urls between potentially separate, cross-origin renderers in RenderFrameHostImpl::ReportContentSecurityPolicyViolation. It is bad from a security point of view when one of the renderer is compromised. This CL prevent the browser to send the full path of the urls when they are cross-origin. It regresses the quality of some console messages. BUG=718942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel;master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Do not disclose urls between cross-origin renderers. The browser transmits Urls between potentially separate, cross-origin renderers in RenderFrameHostImpl::ReportContentSecurityPolicyViolation. It is bad from a security point of view when one of the renderer is compromised. This CL prevent the browser to send the full path of the urls when they are cross-origin. It regresses the quality of some console messages. BUG=718942 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel;master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2869423002 Cr-Commit-Position: refs/heads/master@{#472433} Committed: https://chromium.googlesource.com/chromium/src/+/74a80d7c654a53bbb1ad714d8d86... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/74a80d7c654a53bbb1ad714d8d86... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
