|
|
Created:
4 years, 6 months ago by Łukasz Anforowicz Modified:
4 years, 6 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixing renderer's access to a file from HTTP POST (after a xsite transfer).
Renderer needs to access files from HTTP POST (i.e. from XSSAuditor).
This CL makes sure that file access is preserved across xsite transfers.
BUG=101395, 613260
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/bd82f922853ea6e61c4d907f2b51786f3ed9f5a2
Cr-Commit-Position: refs/heads/master@{#400460}
Patch Set 1 #Patch Set 2 : One more test expectation. #
Total comments: 2
Patch Set 3 : Got the fix. #Patch Set 4 : More self-review + the CL actually builds this time around... :-/ #Patch Set 5 : Refactoring / deduplicating a bit of code. #Patch Set 6 : Removed unnecessary logging + simplified condition of an "if" statement. #
Total comments: 12
Patch Set 7 : Fixing Windows build. #
Total comments: 8
Patch Set 8 : Rebasing... #Patch Set 9 : Moved the test to another browser tests suite. #Patch Set 10 : Removed the new (redundant) access check from RDHI. #
Total comments: 2
Patch Set 11 : Moving FileChooserDelegate into content_browser_test_utils_*internal*.h #Dependent Patchsets: Messages
Total messages: 30 (13 generated)
Description was changed from ========== Test renderer ability to access a file from HTTP POST - xsite transfer case. BUG=101395, 613260 ========== to ========== Test renderer ability to access a file from HTTP POST - xsite transfer case. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
lukasza@chromium.org changed reviewers: + creis@chromium.org
Charlie, thanks for pointing me at the old CL [1] and the problem of granting file access permissions to the renderer process. I've tried to repro this with the new browser test in this CL, but the test unexpectedly passes. Any guesses why the test is passes? Does the test need to be tweaked to hit a repro? Or maybe this was fixed by earlier CLs (i.e. https://codereview.chromium.org/745053002)? At any rate - could you do a review please? More tests is good, right? (I haven't been able to find an existing test for this - there are only RenderFrameHostManagerTest. RestoreSubframeFileAccessForHistoryNavigation and RestoreFileAccessForHistoryNavigation AFAICT). Note that the CL is not landable as-is, because of 2 failing test assertions that I am pointing out below (the essence of the test passes though). [1] https://codereview.chromium.org/11193051/diff/48008/content/browser/web_conte... https://codereview.chromium.org/2062523002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2062523002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2798: old_process_id, file_path)); This test expectation fails today - sounds like an opportunity to further restrict capabilities granted to a renderer process, right? https://codereview.chromium.org/2062523002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2816: old_process_id, file_path)); This test expectation also fails today - it seems that we should be able to revoke this ability (i.e. via ChildProcessSecurityPolicyImpl::RevokeAllPermissionsForFile)? It seems that we revoke file permissions for 1) downloads, 2) indexed db and 3) FileSystemHostMsg_CreateSnapshotFile (whatever this is), but I am not sure if we do it for files from HTTP POST body (so permissions accumulate? this would also be a mini memory leak?).
Actually, please ignore me - last week I must have run the test without --site-per-process flag. I've just tried the test with --site-per-process and I do hit a renderer kill with the test. On 2016/06/10 23:07:04, Łukasz Anforowicz wrote: > Charlie, thanks for pointing me at the old CL [1] and the problem of granting > file access permissions to the renderer process. > > I've tried to repro this with the new browser test in this CL, but the test > unexpectedly passes. Any guesses why the test is passes? Does the test need to > be tweaked to hit a repro? Or maybe this was fixed by earlier CLs (i.e. > https://codereview.chromium.org/745053002) > > At any rate - could you do a review please? More tests is good, right? (I > haven't been able to find an existing test for this - there are only > RenderFrameHostManagerTest. > RestoreSubframeFileAccessForHistoryNavigation and > RestoreFileAccessForHistoryNavigation AFAICT). Note that the CL is not landable > as-is, because of 2 failing test assertions that I am pointing out below (the > essence of the test passes though). > > [1] > https://codereview.chromium.org/11193051/diff/48008/content/browser/web_conte... > > https://codereview.chromium.org/2062523002/diff/20001/content/browser/frame_h... > File content/browser/frame_host/render_frame_host_manager_browsertest.cc > (right): > > https://codereview.chromium.org/2062523002/diff/20001/content/browser/frame_h... > content/browser/frame_host/render_frame_host_manager_browsertest.cc:2798: > old_process_id, file_path)); > This test expectation fails today - sounds like an opportunity to further > restrict capabilities granted to a renderer process, right? > > https://codereview.chromium.org/2062523002/diff/20001/content/browser/frame_h... > content/browser/frame_host/render_frame_host_manager_browsertest.cc:2816: > old_process_id, file_path)); > This test expectation also fails today - it seems that we should be able to > revoke this ability (i.e. via > ChildProcessSecurityPolicyImpl::RevokeAllPermissionsForFile)? It seems that we > revoke file permissions for 1) downloads, 2) indexed db and 3) > FileSystemHostMsg_CreateSnapshotFile (whatever this is), but I am not sure if we > do it for files from HTTP POST body (so permissions accumulate? this would also > be a mini memory leak?).
Description was changed from ========== Test renderer ability to access a file from HTTP POST - xsite transfer case. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Test renderer ability to access a file from HTTP POST - xsite transfer case. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
lukasza@chromium.org changed reviewers: - creis@chromium.org
Description was changed from ========== Test renderer ability to access a file from HTTP POST - xsite transfer case. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fixing renderer's access to a file from HTTP POST (after a xsite transfer). Renderer needs to access files from HTTP POST (i.e. from XSSAuditor). This CL makes sure that file access is preserved across xsite transfers. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fixing renderer's access to a file from HTTP POST (after a xsite transfer). Renderer needs to access files from HTTP POST (i.e. from XSSAuditor). This CL makes sure that file access is preserved across xsite transfers. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fixing renderer's access to a file from HTTP POST (after a xsite transfer). Renderer needs to access files from HTTP POST (i.e. from XSSAuditor). This CL makes sure that file access is preserved across xsite transfers. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
lukasza@chromium.org changed reviewers: + creis@chromium.org
Ok Charlie, this time around this CL should be ready for a real review :-) Can you take a look please? https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1401: } AFAICT this check should cover all cases where ResourceRequestBody is sent from a renderer to the browser: - ResourceHostMsg_SyncLoad - ResourceHostMsg_RequestResource The things above were found by grepping //content/common for ResourceRequestBody and then grepping for structs embedding ResourceRequestBody (CommonNavigationParams - only browser->renderer and ResourceRequest - this one is sent renderer->browser). Note that even before the check added above things were okay from security perspective before this CL. In particular file access was getting checked in content/browser/loader/resource_dispatcher_host_impl.cc: 335: ShouldServiceRequest (but the response was safe/secure fallback + NOTREACHED, not a renderer kill). https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... content/common/resource_request_body.cc:54: if (element.type() == Element::TYPE_FILE) AFAICT this is the only type that can refer to files. There is also TYPE_FILE_FILESYSTEM that corresponds to blink::WebHTTPBody::Element::Type::TypeFileSystemURL (with DCHECKs in some places saying that the URI scheme should be filesystem:) and seems to be handled by FileSystemProtocolHandler. This type doesn't seem to deal with real paths and therefore I think we don't need to grant any file rights based on TYPE_FILE_FILESYSTEM elements. OTOH, I admit that I don't fully understand what storage/browser/fileapi does - maybe we also need to care about file access for these elements?
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062523002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! I think I agree that a second validation check in BeginRequest is a good idea, both for the transfer case and to avoid actually uploading files that the process doesn't have access to (not just at commit time after the offense has occurred). LGTM with nits. https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2795: render_view_host_->GrantFileAccessFromPageState(request_params.page_state); Seems a bit unfortunate we have to do this twice, once here and once for the post_data in the params, but I suppose that's necessary (for history navigations vs transfers). That said, can we use a shared RFHI::GrantFileAccess(file_paths) helper function for both cases, where we pass in request_params.page_state.GetReferencedFiles() and common_params.post_data.GetReferencedFiles(), respectively? Then we can delete the RVH method. https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2826: // Navigate to the page with form that posts via 307 redirection to Can you elaborate a bit that 307 preserves the POST vs 302 which converts to GET? https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2866: if (SiteIsolationPolicy::UseDedicatedProcessesForAllSites()) This test is actually about transfers, right? Maybe we could put this on in cross_site_transfer_browsertest.cc and remove this line, since it already calls IsolateAllSitesForTesting in its setup?
https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1401: } On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > AFAICT this check should cover all cases where ResourceRequestBody is sent from > a renderer to the browser: > - ResourceHostMsg_SyncLoad > - ResourceHostMsg_RequestResource > > The things above were found by grepping //content/common for ResourceRequestBody > and then grepping for structs embedding ResourceRequestBody > (CommonNavigationParams - only browser->renderer and ResourceRequest - this one > is sent renderer->browser). > > Note that even before the check added above things were okay from security > perspective before this CL. In particular file access was getting checked in > content/browser/loader/resource_dispatcher_host_impl.cc: 335: > ShouldServiceRequest (but the response was safe/secure fallback + NOTREACHED, > not a renderer kill). Oh, we did catch it there. We get to ShouldServiceRequest before deciding to transfer as well. Is your new check only for the post-transfer case? https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... content/common/resource_request_body.cc:54: if (element.type() == Element::TYPE_FILE) On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > AFAICT this is the only type that can refer to files. > > There is also TYPE_FILE_FILESYSTEM that corresponds to > blink::WebHTTPBody::Element::Type::TypeFileSystemURL (with DCHECKs in some > places saying that the URI scheme should be filesystem:) and seems to be handled > by FileSystemProtocolHandler. This type doesn't seem to deal with real paths > and therefore I think we don't need to grant any file rights based on > TYPE_FILE_FILESYSTEM elements. OTOH, I admit that I don't fully understand what > storage/browser/fileapi does - maybe we also need to care about file access for > these elements? Yeah, filesystem: URLs are associated with origin-specific storage, so any renderer is allowed to request them (for now). We'll lock them down later for Site Isolation.
On 2016/06/16 20:18:00, Charlie Reis (OOO til June 18) wrote: > I think I agree that a second validation check in BeginRequest is a > good idea, both for the transfer case and to avoid actually uploading files that > the process doesn't have access to (not just at commit time after the offense > has occurred). Hmmm... after reading your comments and looking at the CL again, I think that the existing checks (via RDHI::ShouldServiceRequest) should be sufficient (and that maybe I should remove the check from this CL and/or considered adding a renderer kill in a separate CL). > > LGTM with nits. > https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1401: } On 2016/06/16 20:22:11, Charlie Reis (OOO til June 18) wrote: > On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > > AFAICT this check should cover all cases where ResourceRequestBody is sent > from > > a renderer to the browser: > > - ResourceHostMsg_SyncLoad > > - ResourceHostMsg_RequestResource > > > > The things above were found by grepping //content/common for > ResourceRequestBody > > and then grepping for structs embedding ResourceRequestBody > > (CommonNavigationParams - only browser->renderer and ResourceRequest - this > one > > is sent renderer->browser). > > > > Note that even before the check added above things were okay from security > > perspective before this CL. In particular file access was getting checked in > > content/browser/loader/resource_dispatcher_host_impl.cc: 335: > > ShouldServiceRequest (but the response was safe/secure fallback + NOTREACHED, > > not a renderer kill). > > Oh, we did catch it there. We get to ShouldServiceRequest before deciding to > transfer as well. Hmmm... I didn't previously look very hard how ShouldServiceRequest is getting called. I now see that it is called right below, directly from BeginRequest. So - the new check does indeed look a bit redundant. I've added the new check, because I was trying to mimic how we validate PageState and didn't realize that ShouldServiceRequest check is done right here (I was searching for it separately to verify that there was a check and confused myself into thinking that this already existing check happens some time after the IPC). The only advantage of the new check is that it would allow us to 1) more aggressively stop compromised renderers and 2) get some insight into these renderer kills via UMA. OTOH, the new check also 1) adds a bit of unnecessary code complexity and 2) has some small risk of actually killing an innocent renderer (but in this case we would find bugs via UMA - yay?). So - after typing the above, I am considering removing the new check (and the new UMA / kill enum + histogram). WDYT? What would you want to do if I haven't already pushed out a CR that includes the new check? :-) > Is your new check only for the post-transfer case? I don't understand the question :-( https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... content/common/resource_request_body.cc:54: if (element.type() == Element::TYPE_FILE) On 2016/06/16 20:22:12, Charlie Reis (OOO til June 18) wrote: > On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > > AFAICT this is the only type that can refer to files. > > > > There is also TYPE_FILE_FILESYSTEM that corresponds to > > blink::WebHTTPBody::Element::Type::TypeFileSystemURL (with DCHECKs in some > > places saying that the URI scheme should be filesystem:) and seems to be > handled > > by FileSystemProtocolHandler. This type doesn't seem to deal with real paths > > and therefore I think we don't need to grant any file rights based on > > TYPE_FILE_FILESYSTEM elements. OTOH, I admit that I don't fully understand > what > > storage/browser/fileapi does - maybe we also need to care about file access > for > > these elements? > > Yeah, filesystem: URLs are associated with origin-specific storage, so any > renderer is allowed to request them (for now). We'll lock them down later for > Site Isolation. Thanks for looking into this. https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2795: render_view_host_->GrantFileAccessFromPageState(request_params.page_state); On 2016/06/16 20:18:00, Charlie Reis (OOO til June 18) wrote: > Seems a bit unfortunate we have to do this twice, once here and once for the > post_data in the params, but I suppose that's necessary (for history navigations > vs transfers). Yes. These 2 scenarios are also transferring the access for slightly different reasons (copying access granted via historical file dialogs VS copying access from the old to the new renderer after a cross-site transfer): scenario 0: file dialog grants the renderer access to the files (even before any navigation, because the files might be accessed by a plugin...). scenario 1: history navigations: let's apply historical file dialog decisions to the new renderer (without PlzNavigate we have to give the renderer the PageState and only *afterwards* it will reply with ResourceRequestBody). scenario 2: transfers: let's transfer file access from the old to the new renderer (the old renderer sent ResourceRequestBody to the browser; this would be checked for access in content/browser/loader/resource_dispatcher_host_impl.cc: 335: ShouldServiceRequest (but only by safe fallback + NOTREACHED, not by a renderer kill)). > That said, can we use a shared RFHI::GrantFileAccess(file_paths) helper function > for both cases, where we pass in request_params.page_state.GetReferencedFiles() > and common_params.post_data.GetReferencedFiles(), respectively? Then we can > delete the RVH method. Yes! I am already on it :-) Please see the second CL at https://codereview.chromium.org/2067493003/ https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2826: // Navigate to the page with form that posts via 307 redirection to On 2016/06/16 20:18:00, Charlie Reis (OOO til June 18) wrote: > Can you elaborate a bit that 307 preserves the POST vs 302 which converts to > GET? Done. https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2866: if (SiteIsolationPolicy::UseDedicatedProcessesForAllSites()) On 2016/06/16 20:18:00, Charlie Reis (OOO til June 18) wrote: > This test is actually about transfers, right? Maybe we could put this on in > cross_site_transfer_browsertest.cc and remove this line, since it already calls > IsolateAllSitesForTesting in its setup? Good point. Done. Note that this required extracting FileChooserDelegate to a common file, where it can be reused both by RenderFrameHostManagerTest and CrossSiteTransferBrowserTest.
https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1401: } On 2016/06/16 22:05:04, Łukasz Anforowicz wrote: > On 2016/06/16 20:22:11, Charlie Reis (OOO til June 18) wrote: > > On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > > > AFAICT this check should cover all cases where ResourceRequestBody is sent > > from > > > a renderer to the browser: > > > - ResourceHostMsg_SyncLoad > > > - ResourceHostMsg_RequestResource > > > > > > The things above were found by grepping //content/common for > > ResourceRequestBody > > > and then grepping for structs embedding ResourceRequestBody > > > (CommonNavigationParams - only browser->renderer and ResourceRequest - this > > one > > > is sent renderer->browser). > > > > > > Note that even before the check added above things were okay from security > > > perspective before this CL. In particular file access was getting checked > in > > > content/browser/loader/resource_dispatcher_host_impl.cc: 335: > > > ShouldServiceRequest (but the response was safe/secure fallback + > NOTREACHED, > > > not a renderer kill). > > > > Oh, we did catch it there. We get to ShouldServiceRequest before deciding to > > transfer as well. > > Hmmm... I didn't previously look very hard how ShouldServiceRequest is getting > called. I now see that it is called right below, directly from BeginRequest. > So - the new check does indeed look a bit redundant. I've added the new check, > because I was trying to mimic how we validate PageState and didn't realize that > ShouldServiceRequest check is done right here (I was searching for it separately > to verify that there was a check and confused myself into thinking that this > already existing check happens some time after the IPC). > > The only advantage of the new check is that it would allow us to 1) more > aggressively stop compromised renderers and 2) get some insight into these > renderer kills via UMA. OTOH, the new check also 1) adds a bit of unnecessary > code complexity and 2) has some small risk of actually killing an innocent > renderer (but in this case we would find bugs via UMA - yay?). > > So - after typing the above, I am considering removing the new check (and the > new UMA / kill enum + histogram). WDYT? What would you want to do if I haven't > already pushed out a CR that includes the new check? :-) > > > Is your new check only for the post-transfer case? > > I don't understand the question :-( Ok, I agree this whole thing is a bit confusing. Let's look closer: BeginRequest gets called twice: once before the transfer from the old RFH (in which case we hit ShouldServiceRequest below), and once from the new RFH (in which case we call CompleteTransfer() and return before getting to ShouldServiceRequest). We won't kill the new RFH if it doesn't have access to those files at the time of BeginRequest, but we will kill it at commit time. That said, it's not like killing the new RFH in BeginRequest is that meaningful anyway, because the POST data has *already* been sent to the server. The only thing that happens from here is that the page gets committed in the new renderer, at which point we'll verify it had access to the files. I think this means we can probably skip the extra kill. (FYI: Renderer kills now show up both in UMA data and as DumpWithoutCrashing browser crash reports, so that we can track them with stacks, magic signatures, etc.) https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2795: render_view_host_->GrantFileAccessFromPageState(request_params.page_state); On 2016/06/16 22:05:04, Łukasz Anforowicz wrote: > On 2016/06/16 20:18:00, Charlie Reis (OOO til June 18) wrote: > > Seems a bit unfortunate we have to do this twice, once here and once for the > > post_data in the params, but I suppose that's necessary (for history > navigations > > vs transfers). > > Yes. These 2 scenarios are also transferring the access for slightly different > reasons (copying access granted via historical file dialogs VS copying access > from the old to the new renderer after a cross-site transfer): > > scenario 0: file dialog grants the renderer access to the files (even before any > navigation, because the files might be accessed by a plugin...). > > scenario 1: history navigations: let's apply historical file dialog decisions to > the new renderer (without PlzNavigate we have to give the renderer the PageState > and only *afterwards* it will reply with ResourceRequestBody). > > scenario 2: transfers: let's transfer file access from the old to the new > renderer > (the old renderer sent ResourceRequestBody to the browser; this would be > checked for access in content/browser/loader/resource_dispatcher_host_impl.cc: > 335: ShouldServiceRequest (but only by safe fallback + NOTREACHED, not by a > renderer kill)). > > > That said, can we use a shared RFHI::GrantFileAccess(file_paths) helper > function > > for both cases, where we pass in > request_params.page_state.GetReferencedFiles() > > and common_params.post_data.GetReferencedFiles(), respectively? Then we can > > delete the RVH method. > > Yes! I am already on it :-) Please see the second CL at > https://codereview.chromium.org/2067493003/ Ha! Sorry I missed it, and thanks! :) https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_browsertest.cc (right): https://codereview.chromium.org/2062523002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_browsertest.cc:2866: if (SiteIsolationPolicy::UseDedicatedProcessesForAllSites()) On 2016/06/16 22:05:04, Łukasz Anforowicz wrote: > On 2016/06/16 20:18:00, Charlie Reis (OOO til June 18) wrote: > > This test is actually about transfers, right? Maybe we could put this on in > > cross_site_transfer_browsertest.cc and remove this line, since it already > calls > > IsolateAllSitesForTesting in its setup? > > Good point. Done. > > Note that this required extracting FileChooserDelegate to a common file, where > it can be reused both by RenderFrameHostManagerTest and > CrossSiteTransferBrowserTest. Acknowledged.
Thanks Charlie. I think it should be ready to land now. https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1401: } On 2016/06/16 22:19:59, Charlie Reis (OOO til June 18) wrote: > On 2016/06/16 22:05:04, Łukasz Anforowicz wrote: > > On 2016/06/16 20:22:11, Charlie Reis (OOO til June 18) wrote: > > > On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > > > > AFAICT this check should cover all cases where ResourceRequestBody is sent > > > from > > > > a renderer to the browser: > > > > - ResourceHostMsg_SyncLoad > > > > - ResourceHostMsg_RequestResource > > > > > > > > The things above were found by grepping //content/common for > > > ResourceRequestBody > > > > and then grepping for structs embedding ResourceRequestBody > > > > (CommonNavigationParams - only browser->renderer and ResourceRequest - > this > > > one > > > > is sent renderer->browser). > > > > > > > > Note that even before the check added above things were okay from security > > > > perspective before this CL. In particular file access was getting checked > > in > > > > content/browser/loader/resource_dispatcher_host_impl.cc: 335: > > > > ShouldServiceRequest (but the response was safe/secure fallback + > > NOTREACHED, > > > > not a renderer kill). > > > > > > Oh, we did catch it there. We get to ShouldServiceRequest before deciding > to > > > transfer as well. > > > > Hmmm... I didn't previously look very hard how ShouldServiceRequest is getting > > called. I now see that it is called right below, directly from BeginRequest. > > So - the new check does indeed look a bit redundant. I've added the new > check, > > because I was trying to mimic how we validate PageState and didn't realize > that > > ShouldServiceRequest check is done right here (I was searching for it > separately > > to verify that there was a check and confused myself into thinking that this > > already existing check happens some time after the IPC). > > > > The only advantage of the new check is that it would allow us to 1) more > > aggressively stop compromised renderers and 2) get some insight into these > > renderer kills via UMA. OTOH, the new check also 1) adds a bit of unnecessary > > code complexity and 2) has some small risk of actually killing an innocent > > renderer (but in this case we would find bugs via UMA - yay?). > > > > So - after typing the above, I am considering removing the new check (and the > > new UMA / kill enum + histogram). WDYT? What would you want to do if I > haven't > > already pushed out a CR that includes the new check? :-) > > > > > Is your new check only for the post-transfer case? > > > > I don't understand the question :-( > > Ok, I agree this whole thing is a bit confusing. Let's look closer: > > BeginRequest gets called twice: once before the transfer from the old RFH (in > which case we hit ShouldServiceRequest below), and once from the new RFH (in > which case we call CompleteTransfer() and return before getting to > ShouldServiceRequest). > > We won't kill the new RFH if it doesn't have access to those files at the time > of BeginRequest, but we will kill it at commit time. That said, it's not like > killing the new RFH in BeginRequest is that meaningful anyway, because the POST > data has *already* been sent to the server. The only thing that happens from > here is that the page gets committed in the new renderer, at which point we'll > verify it had access to the files. > > I think this means we can probably skip the extra kill. Done. I think this is the right call - the new kill I've added wasn't really needed (as I learned during the review) and was only unnecessarily adding extra complexity. BTW: ShouldServiceRequest already performs some renderer kills - if at a later point in time we think the new kill is desirable, then we can add it there (not in BeginRequest). Also - note that after removing the kill, some of the refactoring/deduplicating work wasn't really needed in this CL (i.e. no need for ChildProcessSecurityPolicyImpl::CanReadAllFiles if it only has one caller / if it is no longer called from RDHI) so I moved this part of the refactoring into the follow-up, separate CL at https://codereview.chromium.org/2067493003/. > (FYI: Renderer kills now show up both in UMA data and as DumpWithoutCrashing > browser crash reports, so that we can track them with stacks, magic signatures, > etc.) https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... content/common/resource_request_body.cc:54: if (element.type() == Element::TYPE_FILE) On 2016/06/16 22:05:04, Łukasz Anforowicz wrote: > On 2016/06/16 20:22:12, Charlie Reis (OOO til June 18) wrote: > > On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > > > AFAICT this is the only type that can refer to files. > > > > > > There is also TYPE_FILE_FILESYSTEM that corresponds to > > > blink::WebHTTPBody::Element::Type::TypeFileSystemURL (with DCHECKs in some > > > places saying that the URI scheme should be filesystem:) and seems to be > > handled > > > by FileSystemProtocolHandler. This type doesn't seem to deal with real > paths > > > and therefore I think we don't need to grant any file rights based on > > > TYPE_FILE_FILESYSTEM elements. OTOH, I admit that I don't fully understand > > what > > > storage/browser/fileapi does - maybe we also need to care about file access > > for > > > these elements? > > > > Yeah, filesystem: URLs are associated with origin-specific storage, so any > > renderer is allowed to request them (for now). We'll lock them down later for > > Site Isolation. > > Thanks for looking into this. Actually, I think we can land the current CL as-is (i.e. forwarding only TYPE_FILE elements of HTTP body), but I really need to cook-up a test that also exercises TYPE_FILE_FILESYSTEM elements (in a separate CL). I understand that web storage API is supposed to be restricted to a single origin, but I bet that XSSAuditor will try to read these elements (similarly to how it reads TYPE_FILE elements which results in a kill + similarily to how TYPE_FILE elements will [I assume / guess] end up in PageState of the new renderer). The argument for safety of the transfer (of filesystem-storage elements) would be kind of similar to the argument we made for regular files - that POST data was already handed out to the server at the origin of the new renderer (so no additional harm would come from also giving this data to the renderer).
https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... content/common/resource_request_body.cc:54: if (element.type() == Element::TYPE_FILE) On 2016/06/16 23:33:55, Łukasz Anforowicz wrote: > On 2016/06/16 22:05:04, Łukasz Anforowicz wrote: > > On 2016/06/16 20:22:12, Charlie Reis (OOO til June 18) wrote: > > > On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > > > > AFAICT this is the only type that can refer to files. > > > > > > > > There is also TYPE_FILE_FILESYSTEM that corresponds to > > > > blink::WebHTTPBody::Element::Type::TypeFileSystemURL (with DCHECKs in some > > > > places saying that the URI scheme should be filesystem:) and seems to be > > > handled > > > > by FileSystemProtocolHandler. This type doesn't seem to deal with real > > paths > > > > and therefore I think we don't need to grant any file rights based on > > > > TYPE_FILE_FILESYSTEM elements. OTOH, I admit that I don't fully > understand > > > what > > > > storage/browser/fileapi does - maybe we also need to care about file > access > > > for > > > > these elements? > > > > > > Yeah, filesystem: URLs are associated with origin-specific storage, so any > > > renderer is allowed to request them (for now). We'll lock them down later > for > > > Site Isolation. > > > > Thanks for looking into this. > > Actually, I think we can land the current CL as-is (i.e. forwarding only > TYPE_FILE elements of HTTP body), but I really need to cook-up a test that also > exercises TYPE_FILE_FILESYSTEM elements (in a separate CL). I understand that > web storage API is supposed to be restricted to a single origin, but I bet that > XSSAuditor will try to read these elements (similarly to how it reads TYPE_FILE > elements which results in a kill + similarily to how TYPE_FILE elements will [I > assume / guess] end up in PageState of the new renderer). The argument for > safety of the transfer (of filesystem-storage elements) would be kind of similar > to the argument we made for regular files - that POST data was already handed > out to the server at the origin of the new renderer (so no additional harm would > come from also giving this data to the renderer). Hmmm... I think that the only way to refer to filesystem-files in HTTP body is via FormData API (basing this on looking at callers of FormDataElement(const KURL& fileSystemURL,...)). AFAICT FormData API can only be used for XHRs. Since XHRs will not cause a navigation (and therefore will not cause a cross-site renderer transfer) then maybe there really is no follow-up needed for TYPE_FILE_FILESYSTEM.
Thanks, LGTM without the extra kill and with just handling TYPE_FILE. https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... content/common/resource_request_body.cc:54: if (element.type() == Element::TYPE_FILE) On 2016/06/17 00:10:32, Łukasz Anforowicz wrote: > On 2016/06/16 23:33:55, Łukasz Anforowicz wrote: > > On 2016/06/16 22:05:04, Łukasz Anforowicz wrote: > > > On 2016/06/16 20:22:12, Charlie Reis (OOO til June 18) wrote: > > > > On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > > > > > AFAICT this is the only type that can refer to files. > > > > > > > > > > There is also TYPE_FILE_FILESYSTEM that corresponds to > > > > > blink::WebHTTPBody::Element::Type::TypeFileSystemURL (with DCHECKs in > some > > > > > places saying that the URI scheme should be filesystem:) and seems to be > > > > handled > > > > > by FileSystemProtocolHandler. This type doesn't seem to deal with real > > > paths > > > > > and therefore I think we don't need to grant any file rights based on > > > > > TYPE_FILE_FILESYSTEM elements. OTOH, I admit that I don't fully > > understand > > > > what > > > > > storage/browser/fileapi does - maybe we also need to care about file > > access > > > > for > > > > > these elements? > > > > > > > > Yeah, filesystem: URLs are associated with origin-specific storage, so any > > > > renderer is allowed to request them (for now). We'll lock them down later > > for > > > > Site Isolation. > > > > > > Thanks for looking into this. > > > > Actually, I think we can land the current CL as-is (i.e. forwarding only > > TYPE_FILE elements of HTTP body), but I really need to cook-up a test that > also > > exercises TYPE_FILE_FILESYSTEM elements (in a separate CL). I understand that > > web storage API is supposed to be restricted to a single origin, but I bet > that > > XSSAuditor will try to read these elements (similarly to how it reads > TYPE_FILE > > elements which results in a kill + similarily to how TYPE_FILE elements will > [I > > assume / guess] end up in PageState of the new renderer). The argument for > > safety of the transfer (of filesystem-storage elements) would be kind of > similar > > to the argument we made for regular files - that POST data was already handed > > out to the server at the origin of the new renderer (so no additional harm > would > > come from also giving this data to the renderer). > > Hmmm... I think that the only way to refer to filesystem-files in HTTP body is > via FormData API (basing this on looking at callers of FormDataElement(const > KURL& fileSystemURL,...)). AFAICT FormData API can only be used for XHRs. > Since XHRs will not cause a navigation (and therefore will not cause a > cross-site renderer transfer) then maybe there really is no follow-up needed for > TYPE_FILE_FILESYSTEM. Sorry for the confusion-- I was agreeing with your "therefore I think we don't need to grant any file rights based on TYPE_FILE_FILESYSTEM elements" conclusion. We only need to care about TYPE_FILE in this CL. https://codereview.chromium.org/2062523002/diff/180001/content/public/test/co... File content/public/test/content_browser_test_utils.h (right): https://codereview.chromium.org/2062523002/diff/180001/content/public/test/co... content/public/test/content_browser_test_utils.h:140: class FileChooserDelegate : public WebContentsDelegate { This should probably go in content_browser_test_utils_internal.h if there aren't any uses of it outside content.
Thanks Charlie. https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/2062523002/diff/100001/content/common/resourc... content/common/resource_request_body.cc:54: if (element.type() == Element::TYPE_FILE) On 2016/06/17 07:13:35, Charlie Reis (OOO til June 18) wrote: > On 2016/06/17 00:10:32, Łukasz Anforowicz wrote: > > On 2016/06/16 23:33:55, Łukasz Anforowicz wrote: > > > On 2016/06/16 22:05:04, Łukasz Anforowicz wrote: > > > > On 2016/06/16 20:22:12, Charlie Reis (OOO til June 18) wrote: > > > > > On 2016/06/14 01:07:17, Łukasz Anforowicz wrote: > > > > > > AFAICT this is the only type that can refer to files. > > > > > > > > > > > > There is also TYPE_FILE_FILESYSTEM that corresponds to > > > > > > blink::WebHTTPBody::Element::Type::TypeFileSystemURL (with DCHECKs in > > some > > > > > > places saying that the URI scheme should be filesystem:) and seems to > be > > > > > handled > > > > > > by FileSystemProtocolHandler. This type doesn't seem to deal with > real > > > > paths > > > > > > and therefore I think we don't need to grant any file rights based on > > > > > > TYPE_FILE_FILESYSTEM elements. OTOH, I admit that I don't fully > > > understand > > > > > what > > > > > > storage/browser/fileapi does - maybe we also need to care about file > > > access > > > > > for > > > > > > these elements? > > > > > > > > > > Yeah, filesystem: URLs are associated with origin-specific storage, so > any > > > > > renderer is allowed to request them (for now). We'll lock them down > later > > > for > > > > > Site Isolation. > > > > > > > > Thanks for looking into this. > > > > > > Actually, I think we can land the current CL as-is (i.e. forwarding only > > > TYPE_FILE elements of HTTP body), but I really need to cook-up a test that > > also > > > exercises TYPE_FILE_FILESYSTEM elements (in a separate CL). I understand > that > > > web storage API is supposed to be restricted to a single origin, but I bet > > that > > > XSSAuditor will try to read these elements (similarly to how it reads > > TYPE_FILE > > > elements which results in a kill + similarily to how TYPE_FILE elements will > > [I > > > assume / guess] end up in PageState of the new renderer). The argument for > > > safety of the transfer (of filesystem-storage elements) would be kind of > > similar > > > to the argument we made for regular files - that POST data was already > handed > > > out to the server at the origin of the new renderer (so no additional harm > > would > > > come from also giving this data to the renderer). > > > > Hmmm... I think that the only way to refer to filesystem-files in HTTP body is > > via FormData API (basing this on looking at callers of FormDataElement(const > > KURL& fileSystemURL,...)). AFAICT FormData API can only be used for XHRs. > > Since XHRs will not cause a navigation (and therefore will not cause a > > cross-site renderer transfer) then maybe there really is no follow-up needed > for > > TYPE_FILE_FILESYSTEM. > > Sorry for the confusion-- I was agreeing with your "therefore I think we don't > need to grant any file rights based on TYPE_FILE_FILESYSTEM elements" > conclusion. We only need to care about TYPE_FILE in this CL. Ok. https://codereview.chromium.org/2062523002/diff/180001/content/public/test/co... File content/public/test/content_browser_test_utils.h (right): https://codereview.chromium.org/2062523002/diff/180001/content/public/test/co... content/public/test/content_browser_test_utils.h:140: class FileChooserDelegate : public WebContentsDelegate { On 2016/06/17 07:13:36, Charlie Reis (OOO til June 18) wrote: > This should probably go in content_browser_test_utils_internal.h if there aren't > any uses of it outside content. Done.
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2062523002/#ps200001 (title: "Moving FileChooserDelegate into content_browser_test_utils_*internal*.h")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062523002/200001
Message was sent while issue was closed.
Description was changed from ========== Fixing renderer's access to a file from HTTP POST (after a xsite transfer). Renderer needs to access files from HTTP POST (i.e. from XSSAuditor). This CL makes sure that file access is preserved across xsite transfers. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fixing renderer's access to a file from HTTP POST (after a xsite transfer). Renderer needs to access files from HTTP POST (i.e. from XSSAuditor). This CL makes sure that file access is preserved across xsite transfers. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Fixing renderer's access to a file from HTTP POST (after a xsite transfer). Renderer needs to access files from HTTP POST (i.e. from XSSAuditor). This CL makes sure that file access is preserved across xsite transfers. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fixing renderer's access to a file from HTTP POST (after a xsite transfer). Renderer needs to access files from HTTP POST (i.e. from XSSAuditor). This CL makes sure that file access is preserved across xsite transfers. BUG=101395, 613260 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/bd82f922853ea6e61c4d907f2b51786f3ed9f5a2 Cr-Commit-Position: refs/heads/master@{#400460} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/bd82f922853ea6e61c4d907f2b51786f3ed9f5a2 Cr-Commit-Position: refs/heads/master@{#400460}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2080223002/ by benwells@chromium.org. The reason for reverting is: New test is failing on DrMemory: First failing build: https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%2... Sample output: CrossSiteTransferTest.PostWithFileData: c:\b\build\slave\drm-cr\build\src\content\browser\cross_site_transfer_browsertest.cc(458): error: Value of: delegate->file_chosen() Actual: false Expected: true [2980:4872:0617/180652:2750297:WARNING:render_frame_host_impl.cc(1927)] OnDidStopLoading was called twice. c:\b\build\slave\drm-cr\build\src\content\browser\cross_site_transfer_browsertest.cc(480): error: Value of: ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile( new_process_id, file_path) Actual: false Expected: true c:\b\build\slave\drm-cr\build\src\content\browser\cross_site_transfer_browsertest.cc(490): error: Value of: actual_page_body Expected: has substring "test-file-content" Actual: "------WebKitFormBoundary8oQLaVhRjyBKYRPr\nContent-Disposition: form-data; name=\"file\"; filename=\"\"\nContent-Type: application/octet-stream\n\n\n------WebKitFormBoundary8oQLaVhRjyBKYRPr--\n\n" c:\b\build\slave\drm-cr\build\src\content\browser\cross_site_transfer_browsertest.cc(492): error: Value of: actual_page_body Expected: has substring "3488.tmp" Actual: "------WebKitFormBoundary8oQLaVhRjyBKYRPr\nContent-Disposition: form-data; name=\"file\"; filename=\"\"\nContent-Type: application/octet-stream\n\n\n------WebKitFormBoundary8oQLaVhRjyBKYRPr--\n\n". |