Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2398463004:
browser_test that creates filesystem resources in extension origin (Closed)
|
Created:
4 years, 2 months ago by ncarter (slow) Modified:
4 years ago CC:
chromium-reviews, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbrowser_test that creates filesystem resources in extension origin
Implement a security exploit test for bug 644966, and improve
DEPS layering by using browser_test_util trampoline functions
instead of content messages from chrome directly.
Synchronizing the CreateFile operations requires installing a
temporary FileUpdateObserver in the FileSystemContext. It turns
out that this is quite hard to do, so it's encapsulated in
a utility class.
BUG=644966
Committed: https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6
Cr-Commit-Position: refs/heads/master@{#436007}
Patch Set 1 #Patch Set 2 : Class comment. #Patch Set 3 : Things. #Patch Set 4 : Rebase. #Patch Set 5 : Fix flakiness by means of a new observer. #Patch Set 6 : Self-review #Patch Set 7 : Cleanup. #
Total comments: 13
Patch Set 8 : Rebase & apply alex's fixes. #
Messages
Total messages: 45 (29 generated)
The CQ bit was checked by nick@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...
Description was changed from ========== Implement chrome_security_exploit test per jam@'s suggestion of test_utils helper functions, instead of raw including messages.h BUG= ========== to ========== Security exploit test for bug 644966 Use browser_test_util trampoline functions instead of #including the header directly. BUG=644966 ==========
nick@chromium.org changed reviewers: + jam@chromium.org
As proposed.
lgtm, thank you
The CQ bit was checked by nick@chromium.org
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
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_...)
The CQ bit was checked by nick@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...
Holding off on committing this due to an observed flaky failure on the linux bot.
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_...)
FYI: Haven't abandoned this, it's #2 in my queue of things to come back to.
ping can you please land this soon? it shouldn't take much effort, and avoids an example in the codebase of something we avoid.
The CQ bit was checked by jam@chromium.org
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/11/04 17:22:36, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) I'm working on some higher priority things right now.
Description was changed from ========== Security exploit test for bug 644966 Use browser_test_util trampoline functions instead of #including the header directly. BUG=644966 ========== to ========== browser_test that creates filesystem resources in extension origin Implement a security exploit test for bug 644966, and improve DEPS layering by using browser_test_util trampoline functions instead of content messages from chrome directly. Synchronizing the CreateFile operations requires installing a temporary update observer in the FileSystemContext. BUG=644966 ==========
The CQ bit was checked by nick@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nick@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...
Description was changed from ========== browser_test that creates filesystem resources in extension origin Implement a security exploit test for bug 644966, and improve DEPS layering by using browser_test_util trampoline functions instead of content messages from chrome directly. Synchronizing the CreateFile operations requires installing a temporary update observer in the FileSystemContext. BUG=644966 ========== to ========== browser_test that creates filesystem resources in extension origin Implement a security exploit test for bug 644966, and improve DEPS layering by using browser_test_util trampoline functions instead of content messages from chrome directly. Synchronizing the CreateFile operations requires installing a temporary FileUpdateObserver in the FileSystemContext. It turns out that this is harder than expected without making changes to the interface, so I created a utility class that gets it done. BUG=644966 ==========
Description was changed from ========== browser_test that creates filesystem resources in extension origin Implement a security exploit test for bug 644966, and improve DEPS layering by using browser_test_util trampoline functions instead of content messages from chrome directly. Synchronizing the CreateFile operations requires installing a temporary FileUpdateObserver in the FileSystemContext. It turns out that this is harder than expected without making changes to the interface, so I created a utility class that gets it done. BUG=644966 ========== to ========== browser_test that creates filesystem resources in extension origin Implement a security exploit test for bug 644966, and improve DEPS layering by using browser_test_util trampoline functions instead of content messages from chrome directly. Synchronizing the CreateFile operations requires installing a temporary FileUpdateObserver in the FileSystemContext. It turns out that this is quite hard to do, so it's encapsulated in a utility class. BUG=644966 ==========
nick@chromium.org changed reviewers: + alexmos@chromium.org
Alex, please review the content/ changes. john already approved this before his leave and the test itself hasn't significantly changed. The primary difference since then is the test_fileapi_operation_waiter, which had to be added to make the test unflaky, but turned out to be a weird interface against which to write a temporary observer, so it's more complexity than you might expect.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits. Thanks for adding this and for all the explanations in comments! https://codereview.chromium.org/2398463004/diff/110001/chrome/browser/chrome_... File chrome/browser/chrome_security_exploit_browsertest.cc (right): https://codereview.chromium.org/2398463004/diff/110001/chrome/browser/chrome_... chrome/browser/chrome_security_exploit_browsertest.cc:133: // recieving unexpected browser->renderer IPCs that might CHECK. nit: s/recieving/receiving/ https://codereview.chromium.org/2398463004/diff/110001/chrome/browser/chrome_... chrome/browser/chrome_security_exploit_browsertest.cc:138: "while (1);")); IIUC, we should never reach the "while (1)" as the sync XHR will block on the send() above it. Is this a safeguard of some sort? https://codereview.chromium.org/2398463004/diff/110001/chrome/browser/chrome_... chrome/browser/chrome_security_exploit_browsertest.cc:141: std::string payload = "<html><body>pwned.<script></script>"; Did this intentionally not have the </body></html>? :) https://codereview.chromium.org/2398463004/diff/110001/content/public/test/br... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/2398463004/diff/110001/content/public/test/br... content/public/test/browser_test_utils.cc:1755: void PwnMessageHelper::CreateBlobWithPayload(RenderProcessHost* process, Move this down next to the other PwnMessageHelper methods? https://codereview.chromium.org/2398463004/diff/110001/content/public/test/br... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/2398463004/diff/110001/content/public/test/br... content/public/test/browser_test_utils.h:725: // Sends BlobStorageMsg_RegisterBlobUUID and BlobStorageMsg_StartBuildingBlob. Seems like this actually sends BlobStorageMsg_RegisterBlob instead of those two IPCs. Should the comment have that instead? https://codereview.chromium.org/2398463004/diff/110001/content/public/test/te... File content/public/test/test_fileapi_operation_waiter.cc (right): https://codereview.chromium.org/2398463004/diff/110001/content/public/test/te... content/public/test/test_fileapi_operation_waiter.cc:23: // removing an observer is not a supported operation. So to support temporary, That does sound like an awkward observer API. So the regular FileUpdateObservers (LocalFileChangeTracker, SandboxQuotaObserver, etc.) are only destroyed on browser shutdown? Your approach with the global observer seems ok then. https://codereview.chromium.org/2398463004/diff/110001/content/public/test/te... content/public/test/test_fileapi_operation_waiter.cc:74: nit: no blank line (for consistency with the other two)
https://codereview.chromium.org/2398463004/diff/110001/chrome/browser/chrome_... File chrome/browser/chrome_security_exploit_browsertest.cc (right): https://codereview.chromium.org/2398463004/diff/110001/chrome/browser/chrome_... chrome/browser/chrome_security_exploit_browsertest.cc:133: // recieving unexpected browser->renderer IPCs that might CHECK. On 2016/12/02 00:01:56, alexmos wrote: > nit: s/recieving/receiving/ Done. https://codereview.chromium.org/2398463004/diff/110001/chrome/browser/chrome_... chrome/browser/chrome_security_exploit_browsertest.cc:138: "while (1);")); On 2016/12/02 00:01:56, alexmos wrote: > IIUC, we should never reach the "while (1)" as the sync XHR will block on the > send() above it. Is this a safeguard of some sort? I put this here as a proactive fallback -- I was worried about xhr completing due to a network error, maybe while the test shuts down the testserver; these IPCs are still in the pipe waiting to produce CHECK failures the moment that happens. I originally implemented this with just the while(1) but noticed that it ran slowly, so added the xhr to prevent a busy core. https://codereview.chromium.org/2398463004/diff/110001/chrome/browser/chrome_... chrome/browser/chrome_security_exploit_browsertest.cc:141: std::string payload = "<html><body>pwned.<script></script>"; On 2016/12/02 00:01:56, alexmos wrote: > Did this intentionally not have the </body></html>? :) Done. https://codereview.chromium.org/2398463004/diff/110001/content/public/test/br... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/2398463004/diff/110001/content/public/test/br... content/public/test/browser_test_utils.cc:1755: void PwnMessageHelper::CreateBlobWithPayload(RenderProcessHost* process, On 2016/12/02 00:01:56, alexmos wrote: > Move this down next to the other PwnMessageHelper methods? Done. https://codereview.chromium.org/2398463004/diff/110001/content/public/test/br... File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/2398463004/diff/110001/content/public/test/br... content/public/test/browser_test_utils.h:725: // Sends BlobStorageMsg_RegisterBlobUUID and BlobStorageMsg_StartBuildingBlob. On 2016/12/02 00:01:56, alexmos wrote: > Seems like this actually sends BlobStorageMsg_RegisterBlob instead of those two > IPCs. Should the comment have that instead? Good catch. This comment was accurate when the function was written, but dmurph merged the IPCs while this CL was sitting around. https://codereview.chromium.org/2398463004/diff/110001/content/public/test/te... File content/public/test/test_fileapi_operation_waiter.cc (right): https://codereview.chromium.org/2398463004/diff/110001/content/public/test/te... content/public/test/test_fileapi_operation_waiter.cc:23: // removing an observer is not a supported operation. So to support temporary, On 2016/12/02 00:01:56, alexmos wrote: > That does sound like an awkward observer API. So the regular > FileUpdateObservers (LocalFileChangeTracker, SandboxQuotaObserver, etc.) are > only destroyed on browser shutdown? Your approach with the global observer > seems ok then. From what I can tell, the other FileUpdateObservers haven't encountered this problem because they're also owned by the FileSystemContext. There's one FileSystemContext per StoragePartition, so to first approximation that means it has a Profile lifetime. I think it might be safe to bound the lifetime of this object by letting the BrowserContext own it via SupportsUserData, but I don't want to overengineer it. I somewhat doubt this observer will ever be used again. FWIW: I wrote this six different ways, all of them different flavors of ugly, before landing on the leaky singleton approach. The runner up was to use refcounting, and AddRef at OnStartUpdate time, and Release at OnEndUpdate time but it was ugly, depended strongly on the current impl, and there was still no good way to remove an observer -- all you can really do is to restore the list to what it was when you started observing, which means spinning up multiple observers would have potentially very weird behavior.
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2398463004/#ps130001 (title: "Rebase & apply alex's fixes.")
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": 130001, "attempt_start_ts": 1480706300867180,
"parent_rev": "70b68a7935dc09a04e6db2908bc9dcf384e3328a", "commit_rev":
"ad7a83f9297f90fa1f0e1e9f04a11b791caf3305"}
Message was sent while issue was closed.
Description was changed from ========== browser_test that creates filesystem resources in extension origin Implement a security exploit test for bug 644966, and improve DEPS layering by using browser_test_util trampoline functions instead of content messages from chrome directly. Synchronizing the CreateFile operations requires installing a temporary FileUpdateObserver in the FileSystemContext. It turns out that this is quite hard to do, so it's encapsulated in a utility class. BUG=644966 ========== to ========== browser_test that creates filesystem resources in extension origin Implement a security exploit test for bug 644966, and improve DEPS layering by using browser_test_util trampoline functions instead of content messages from chrome directly. Synchronizing the CreateFile operations requires installing a temporary FileUpdateObserver in the FileSystemContext. It turns out that this is quite hard to do, so it's encapsulated in a utility class. BUG=644966 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== browser_test that creates filesystem resources in extension origin Implement a security exploit test for bug 644966, and improve DEPS layering by using browser_test_util trampoline functions instead of content messages from chrome directly. Synchronizing the CreateFile operations requires installing a temporary FileUpdateObserver in the FileSystemContext. It turns out that this is quite hard to do, so it's encapsulated in a utility class. BUG=644966 ========== to ========== browser_test that creates filesystem resources in extension origin Implement a security exploit test for bug 644966, and improve DEPS layering by using browser_test_util trampoline functions instead of content messages from chrome directly. Synchronizing the CreateFile operations requires installing a temporary FileUpdateObserver in the FileSystemContext. It turns out that this is quite hard to do, so it's encapsulated in a utility class. BUG=644966 Committed: https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6 Cr-Commit-Position: refs/heads/master@{#436007} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/bfaea4ee6b138d958efc97e16da9624dc28545e6 Cr-Commit-Position: refs/heads/master@{#436007} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
