Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(791)

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
Visibility:
Public.

Description

WebDatabase: 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -0 lines) Patch
M content/browser/renderer_host/database_message_filter.cc View 1 2 2 chunks +13 lines, -0 lines 0 comments Download
M webkit/database/database_util.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M webkit/database/database_util.cc View 1 2 3 2 chunks +12 lines, -0 lines 0 comments Download
M webkit/database/database_util_unittest.cc View 1 2 3 3 chunks +16 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (0 generated)
aedla
7 years, 10 months ago (2013-02-08 13:07:59 UTC) #1
aedla
https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host/database_message_filter.cc File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host/database_message_filter.cc#newcode348 content/browser/renderer_host/database_message_filter.cc:348: if (!database_connections_.IsDatabaseOpened( Is it ok to call IsDatabaseOpened here?
7 years, 10 months ago (2013-02-08 13:13:12 UTC) #2
jsbell
It is feasible to add a unit test? https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host/database_message_filter.cc File content/browser/renderer_host/database_message_filter.cc (right): https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host/database_message_filter.cc#newcode283 content/browser/renderer_host/database_message_filter.cc:283: bool ...
7 years, 10 months ago (2013-02-08 22:33:24 UTC) #3
aedla
Awesome, thanks for the review!
7 years, 10 months ago (2013-02-09 00:54:17 UTC) #4
aedla
Not sure how easy it would be to add unit tests for DatabaseMessageFilter:: methods. @michaeln, ...
7 years, 10 months ago (2013-02-11 10:52:23 UTC) #5
michaeln
If you wanted to unit test the new is valid method, maybe move it to ...
7 years, 10 months ago (2013-02-12 00:49:13 UTC) #6
aedla
On 2013/02/12 00:49:13, michaeln wrote: > If you wanted to unit test the new is ...
7 years, 10 months ago (2013-02-12 09:47:14 UTC) #7
aedla
PTAL https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host/database_message_filter.h File content/browser/renderer_host/database_message_filter.h (right): https://codereview.chromium.org/12212091/diff/1/content/browser/renderer_host/database_message_filter.h#newcode89 content/browser/renderer_host/database_message_filter.h:89: bool IsValidOriginIdentifier(const string16 &origin_identifier); On 2013/02/08 22:33:24, jsbell ...
7 years, 10 months ago (2013-02-12 18:11:00 UTC) #8
jsbell
https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_util.cc File webkit/database/database_util.cc (right): https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_util.cc#newcode96 webkit/database/database_util.cc:96: string16 forbidden = ASCIIToUTF16(base::StringPiece("\\/\0", 3)); Given the way it ...
7 years, 10 months ago (2013-02-12 18:20:32 UTC) #9
aedla
PTAL https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_util.cc File webkit/database/database_util.cc (right): https://codereview.chromium.org/12212091/diff/5004/webkit/database/database_util.cc#newcode96 webkit/database/database_util.cc:96: string16 forbidden = ASCIIToUTF16(base::StringPiece("\\/\0", 3)); On 2013/02/12 18:20:32, ...
7 years, 10 months ago (2013-02-12 19:27:43 UTC) #10
jsbell
lgtm but you'll need OWNERS reviews to commit it
7 years, 10 months ago (2013-02-12 19:36:33 UTC) #11
michaeln
lgtm2 !
7 years, 10 months ago (2013-02-12 21:16:54 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aedla@chromium.org/12212091/3004
7 years, 10 months ago (2013-02-12 22:30:27 UTC) #13
commit-bot: I haz the power
Presubmit check for 12212091-3004 failed and returned exit status 1. INFO:root:Found 4 file(s). Running presubmit ...
7 years, 10 months ago (2013-02-12 22:30:31 UTC) #14
michaeln
bah... somewhat annoyingly... database_message_filter.cc is outside of a directory with database specific owners :(
7 years, 10 months ago (2013-02-13 00:45:19 UTC) #15
inferno
ccing Ben@ and Thakis@ for content/browser/renderer_host/database_message_filter.cc OWNERS review.
7 years, 10 months ago (2013-02-13 18:52:34 UTC) #16
Nico
Is it possible to write a test for the change in renderer_host/database_message_filter.cc?
7 years, 10 months ago (2013-02-13 18:56:06 UTC) #17
michaeln
Anything (well many things) are possible. Not sure how valuable such a test would be... ...
7 years, 10 months ago (2013-02-14 00:51:07 UTC) #18
inferno
Nico, we want this patch for pwnium m25 patch 1 which is a few days ...
7 years, 10 months ago (2013-02-14 17:53:23 UTC) #19
Nico
On 2013/02/14 17:53:23, inferno wrote: > Nico, we want this patch for pwnium m25 patch ...
7 years, 10 months ago (2013-02-18 09:24:00 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aedla@chromium.org/12212091/3004
7 years, 10 months ago (2013-02-18 19:27:10 UTC) #21
commit-bot: I haz the power
Change committed as 183141
7 years, 10 months ago (2013-02-18 22:21:07 UTC) #22
Nico
On 2013/02/14 17:53:23, inferno wrote: > Nico, we want this patch for pwnium m25 patch ...
7 years, 9 months ago (2013-02-28 16:02:49 UTC) #23
aedla
Sorry, haven't gotten around to doing it yet. Would a test really be necessary here ...
7 years, 9 months ago (2013-03-04 12:51:31 UTC) #24
Nico
7 years, 9 months ago (2013-03-04 12:57:10 UTC) #25
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.

Powered by Google App Engine
This is Rietveld 408576698