Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12212091:
WebDatabase: check path traversal in origin_identifier (Closed)
|
Created:
7 years, 10 months ago by aedla Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jsbell Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWebDatabase: check path traversal in origin_identifier
BUG=172264
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183141
Patch Set 1 #
Total comments: 7
Patch Set 2 : IsDatabaseOpened -> IsValidOriginIdentifier in HandleSqliteError #
Total comments: 3
Patch Set 3 : move IsValidOriginIdentifier to DatabaseUtil #
Total comments: 4
Patch Set 4 : more testcases #
Messages
Total messages: 25 (0 generated)
https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... content/browser/renderer_host/database_message_filter.cc:348: if (!database_connections_.IsDatabaseOpened( Is it ok to call IsDatabaseOpened here?
It is feasible to add a unit test? https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... content/browser/renderer_host/database_message_filter.cc:283: bool DatabaseMessageFilter::IsValidOriginIdentifier( This implementation lgtm... https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... content/browser/renderer_host/database_message_filter.cc:348: if (!database_connections_.IsDatabaseOpened( On 2013/02/08 13:13:12, aedla wrote: > Is it ok to call IsDatabaseOpened here? I don't know the WebSQL backend well enough to say. Just a quick glance at callers of HandleSqliteError turns up a unit test which calls it with a non-existent database, so possibly the plumbing expects these might come in after the database is closed due to async message delivery etc. If you can't find an expert or come up to speed on the WebSQL code, it seems safest to just do the IsValidOriginIdentifier() test here as well. https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... File content/browser/renderer_host/database_message_filter.h (right): https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... content/browser/renderer_host/database_message_filter.h:89: bool IsValidOriginIdentifier(const string16 &origin_identifier); Make this static
Awesome, thanks for the review!
Not sure how easy it would be to add unit tests for DatabaseMessageFilter:: methods. @michaeln, what are your thoughts?
If you wanted to unit test the new is valid method, maybe move it to src/webkit/database/database_util.h and add a test to src/webkit/database/database_util_unittest.cc. https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... content/browser/renderer_host/database_message_filter.cc:348: if (!database_connections_.IsDatabaseOpened( On 2013/02/08 13:13:12, aedla wrote: > Is it ok to call IsDatabaseOpened here? Definitely safer to just test IsValidOriginIdentifier here. https://codereview.chromium.org/12212091/diff/5002/content/browser/renderer_h... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/12212091/diff/5002/content/browser/renderer_h... content/browser/renderer_host/database_message_filter.cc:286: char16 forbidden[] = {'/', '\\', '\0'}; funky way to define string values :)
On 2013/02/12 00:49:13, michaeln wrote: > If you wanted to unit test the new is valid method, maybe move it to > src/webkit/database/database_util.h and add a test to > src/webkit/database/database_util_unittest.cc. Makes sense, thanks! https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... content/browser/renderer_host/database_message_filter.cc:348: if (!database_connections_.IsDatabaseOpened( On 2013/02/12 00:49:13, michaeln wrote: > On 2013/02/08 13:13:12, aedla wrote: > > Is it ok to call IsDatabaseOpened here? > > Definitely safer to just test IsValidOriginIdentifier here. Done. https://codereview.chromium.org/12212091/diff/5002/content/browser/renderer_h... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/12212091/diff/5002/content/browser/renderer_h... content/browser/renderer_host/database_message_filter.cc:286: char16 forbidden[] = {'/', '\\', '\0'}; Yeah :) Do you know of a better way to craft a string16? AFAIK string literal won't work because under POSIX its chars are 8 or 32 bits. Don't know if it's clear, but '\0' is part of the forbidden chars, not a string terminator.
PTAL https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... File content/browser/renderer_host/database_message_filter.h (right): https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host... content/browser/renderer_host/database_message_filter.h:89: bool IsValidOriginIdentifier(const string16 &origin_identifier); On 2013/02/08 22:33:24, jsbell wrote: > Make this static Done. https://codereview.chromium.org/12212091/diff/5002/content/browser/renderer_h... File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/12212091/diff/5002/content/browser/renderer_h... content/browser/renderer_host/database_message_filter.cc:286: char16 forbidden[] = {'/', '\\', '\0'}; On 2013/02/12 09:47:15, aedla wrote: > Yeah :) Do you know of a better way to craft a string16? AFAIK string literal > won't work because under POSIX its chars are 8 or 32 bits. > > Don't know if it's clear, but '\0' is part of the forbidden chars, not a string > terminator. Ah ok, found ASCIIToUTF16.
https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_u... File webkit/database/database_util.cc (right): https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_u... webkit/database/database_util.cc:96: string16 forbidden = ASCIIToUTF16(base::StringPiece("\\/\0", 3)); Given the way it was used (as a list of forbidden characters), I actually thought it was more readable the other way, as an array initializer - { '\\', '/', '\0' }. https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_u... File webkit/database/database_util_unittest.cc (right): https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_u... webkit/database/database_util_unittest.cc:64: TEST(DatabaseUtilTest, IsValidOriginIdentifier) { Can you add a test case with a string containing ".." (should fail), and a test case with empty string (should pass, edge case)
PTAL https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_u... File webkit/database/database_util.cc (right): https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_u... webkit/database/database_util.cc:96: string16 forbidden = ASCIIToUTF16(base::StringPiece("\\/\0", 3)); On 2013/02/12 18:20:32, jsbell wrote: > Given the way it was used (as a list of forbidden characters), I actually > thought it was more readable the other way, as an array initializer - { '\\', > '/', '\0' }. Agreed, done. https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_u... File webkit/database/database_util_unittest.cc (right): https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_u... webkit/database/database_util_unittest.cc:64: TEST(DatabaseUtilTest, IsValidOriginIdentifier) { On 2013/02/12 18:20:32, jsbell wrote: > Can you add a test case with a string containing ".." (should fail), and a test > case with empty string (should pass, edge case) Done.
lgtm but you'll need OWNERS reviews to commit it
lgtm2 !
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aedla@chromium.org/12212091/3004
Presubmit check for 12212091-3004 failed and returned exit status 1.
INFO:root:Found 4 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/browser/renderer_host
Presubmit checks took 1.5s to calculate.
bah... somewhat annoyingly... database_message_filter.cc is outside of a directory with database specific owners :(
ccing Ben@ and Thakis@ for content/browser/renderer_host/database_message_filter.cc OWNERS review.
Is it possible to write a test for the change in renderer_host/database_message_filter.cc?
Anything (well many things) are possible. Not sure how valuable such a test
would be...
xxx void DatabaseMessageFilter::OnDoSomething(... origin_identifier ...) {
xxx ...
335 if (!DatabaseUtil::IsValidOriginIdentifier(origin_identifier)) {
336 RecordAction(UserMetricsAction("BadMessageTerminate_DBMF"));
337 BadMessageReceived();
338 return;
339 }
A unittest for these additions would invoke OnDoSomething() with a value that is
not valid and upon return, look for artifacts of RecordAction and
BadMessageReceived having been called. It would detect if/when somebody outright
deleted the newly added IsValid calls from those methods so there's some value
in that, but there are other measures in place to prevent arbitrary deletions
like that from happening (code review).
A browsertest is less possible since a renderer will not compose an invalid
origin_identifier string (modulo it being hacked in some malicious way).
fwiw, i'm satisfied with the unittest for the new IsValid function.
Nico, we want this patch for pwnium m25 patch 1 which is a few days away. And we want it to bake on dev. We can add the test later, please help to lgtm if the change looks good to you.
On 2013/02/14 17:53:23, inferno wrote: > Nico, we want this patch for pwnium m25 patch 1 which is a few days away. And we > want it to bake on dev. We can add the test later, please help to lgtm if the > change looks good to you. We don't accept CLs with "omg this is urgent; I'll write tests later". lgtm this time if you add a test within a week, but the next time please write tests before you send out the CL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aedla@chromium.org/12212091/3004
Message was sent while issue was closed.
Change committed as 183141
Message was sent while issue was closed.
On 2013/02/14 17:53:23, inferno wrote: > Nico, we want this patch for pwnium m25 patch 1 which is a few days away. And we > want it to bake on dev. We can add the test later How the test coming along?
Message was sent while issue was closed.
Sorry, haven't gotten around to doing it yet. Would a test really be necessary here though? The invoked IsValidOriginIdentifier() is already tested in DatabaseUtilTest, so an additional test for DatabaseMessageFilter wouldn't add much. It would guard against removal of the origin check.
Message was sent while issue was closed.
On 2013/03/04 12:51:31, aedla wrote: > Sorry, haven't gotten around to doing it yet. > > Would a test really be necessary here though? The invoked > IsValidOriginIdentifier() is already tested in DatabaseUtilTest, so an > additional test for DatabaseMessageFilter wouldn't add much. It would guard > against removal of the origin check. Right, and that's the code added in this CL. In general, people will try to break all code changes you make (unintentionally, but reliably). Writing tests is the only way to protect yourself from that. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
