Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2214293002:
[BlobStorage] Added back security policy for files in blobs (Closed)
|
Created:
4 years, 4 months ago by dmurph Modified:
4 years, 4 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[BlobStorage] Added back security policy for files in blobs
BUG=634557
Committed: https://crrev.com/a85b6e5f4d01bf0593612d0552d0c7a1ad7bc87f
Cr-Commit-Position: refs/heads/master@{#412013}
Patch Set 1 #
Total comments: 2
Patch Set 2 : forgot filesystem #
Total comments: 12
Patch Set 3 : comments #Patch Set 4 : Made file system context refptr #
Messages
Total messages: 23 (12 generated)
dmurph@chromium.org changed reviewers: + jsbell@chromium.org, kinuko@chromium.org, michaeln@chromium.org
Hey all, This is the security fix we talked about in the email. Tests will be in the next blob patch. Daniel
https://codereview.chromium.org/2214293002/diff/1/content/browser/blob_storag... File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2214293002/diff/1/content/browser/blob_storag... content/browser/blob_storage/blob_dispatcher_host.cc:146: !security_policy_->CanReadFile(process_id_, item.path())) { I think we used to have the same / similar check for TYPE_FILE_FILESYSTEM too?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2214293002/diff/1/content/browser/blob_storag... File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2214293002/diff/1/content/browser/blob_storag... content/browser/blob_storage/blob_dispatcher_host.cc:146: !security_policy_->CanReadFile(process_id_, item.path())) { On 2016/08/05 00:39:36, kinuko wrote: > I think we used to have the same / similar check for TYPE_FILE_FILESYSTEM too? Oops! Yep, done, added.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Looks reasonable, some nits... https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.cc:52: security_policy_(ChildProcessSecurityPolicyImpl::GetInstance()), Do we want to store this pointer in this class? It feels just calling ChildProcessSecurityPolicy::GetInstance() could be fine too https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.cc:161: uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED)); (I think we were previously just ignoring such item, but now we're canceling this at all? Not sure if it causes undesirable (or more desirable) consequences...) https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... File content/browser/blob_storage/blob_dispatcher_host.h (right): https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.h:52: ChromeBlobStorageContext* blob_storage_context); nit: I'd place file_system_context parameter after blob_storage_context https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.h:139: int process_id_; const https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.h:140: storage::FileSystemContext* file_system_context_; nit: I think this'd be safe, but maybe we'd better store this as scoped_refptr?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.cc:52: security_policy_(ChildProcessSecurityPolicyImpl::GetInstance()), On 2016/08/05 15:27:31, kinuko wrote: > Do we want to store this pointer in this class? It feels just calling > ChildProcessSecurityPolicy::GetInstance() could be fine too Sure, I was just modeling fileapi_message_filter. https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.cc:161: uuid, IPCBlobCreationCancelCode::FILE_WRITE_FAILED)); On 2016/08/05 15:27:30, kinuko wrote: > (I think we were previously just ignoring such item, but now we're canceling > this at all? Not sure if it causes undesirable (or more desirable) > consequences...) Well, strangely this is the same. We now need to cancel it so the renderer stops sending us memory, and we still have the blob reference saved, it's just now in the empty 'broken' state. https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... File content/browser/blob_storage/blob_dispatcher_host.h (right): https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.h:52: ChromeBlobStorageContext* blob_storage_context); On 2016/08/05 15:27:31, kinuko wrote: > nit: I'd place file_system_context parameter after blob_storage_context Done. https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.h:139: int process_id_; On 2016/08/05 15:27:31, kinuko wrote: > const Done. https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.h:140: storage::FileSystemContext* file_system_context_; On 2016/08/05 15:27:31, kinuko wrote: > nit: I think this'd be safe, but maybe we'd better store this as scoped_refptr? The FileApiMessageFilter just stores the raw pointer... is it alright if we keep it a raw pointer? I'm not sure if I'll be introducing a refcount cycle if I make it a scoped_refptr
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... File content/browser/blob_storage/blob_dispatcher_host.h (right): https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.h:140: storage::FileSystemContext* file_system_context_; On 2016/08/05 19:12:45, dmurph wrote: > On 2016/08/05 15:27:31, kinuko wrote: > > nit: I think this'd be safe, but maybe we'd better store this as > scoped_refptr? > > The FileApiMessageFilter just stores the raw pointer... is it alright if we keep > it a raw pointer? I'm not sure if I'll be introducing a refcount cycle if I make > it a scoped_refptr Either works for me as far as we don't crash/leak, but it looked a bit weird / inconsistent to me as we hold blob_storage_context_ as a ref-ptr while file_system_context_ as a raw-ptr.
On 2016/08/08 15:52:28, kinuko wrote: > lgtm > > https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... > File content/browser/blob_storage/blob_dispatcher_host.h (right): lgtm 2 > https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... > content/browser/blob_storage/blob_dispatcher_host.h:140: > storage::FileSystemContext* file_system_context_; > On 2016/08/05 19:12:45, dmurph wrote: > > On 2016/08/05 15:27:31, kinuko wrote: > > > nit: I think this'd be safe, but maybe we'd better store this as > > scoped_refptr? > > > > The FileApiMessageFilter just stores the raw pointer... is it alright if we > keep > > it a raw pointer? I'm not sure if I'll be introducing a refcount cycle if I > make > > it a scoped_refptr > > Either works for me as far as we don't crash/leak, but it looked a bit weird / > inconsistent to me as we hold blob_storage_context_ as a ref-ptr while > file_system_context_ as a raw-ptr. I'd vote for a refptr here too, it should not create a cycle.
done https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... File content/browser/blob_storage/blob_dispatcher_host.h (right): https://codereview.chromium.org/2214293002/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_dispatcher_host.h:140: storage::FileSystemContext* file_system_context_; On 2016/08/08 15:52:28, kinuko wrote: > On 2016/08/05 19:12:45, dmurph wrote: > > On 2016/08/05 15:27:31, kinuko wrote: > > > nit: I think this'd be safe, but maybe we'd better store this as > > scoped_refptr? > > > > The FileApiMessageFilter just stores the raw pointer... is it alright if we > keep > > it a raw pointer? I'm not sure if I'll be introducing a refcount cycle if I > make > > it a scoped_refptr > > Either works for me as far as we don't crash/leak, but it looked a bit weird / > inconsistent to me as we hold blob_storage_context_ as a ref-ptr while > file_system_context_ as a raw-ptr. Done.
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2214293002/#ps60001 (title: "Made file system context refptr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Added back security policy for files in blobs BUG=634557 ========== to ========== [BlobStorage] Added back security policy for files in blobs BUG=634557 Committed: https://crrev.com/a85b6e5f4d01bf0593612d0552d0c7a1ad7bc87f Cr-Commit-Position: refs/heads/master@{#412013} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a85b6e5f4d01bf0593612d0552d0c7a1ad7bc87f Cr-Commit-Position: refs/heads/master@{#412013} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
