New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 172264 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
User never visited
Closed: Feb 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment

DatabaseMessageFilter: path traversal in origin_identifier

Project Member Reported by aedla@chromium.org, Jan 25 2013

Issue description

Many IPC messages in DatabaseMessageFilter take an origin_identifier parameter. It is not checked against ".." and can end up in paths given to file_util::Delete and file_util::Move.

REPRODUCTION CASE

Here is a PoC to delete an arbitrary directory. Apply renderer.patch, edit target, compile and run chrome.

 
renderer.patch
1.1 KB View Download

Comment 1 by aedla@chromium.org, Jan 25 2013

Cc: cevans@chromium.org tsepez@chromium.org michaeln@chromium.org

Comment 2 by aedla@chromium.org, Jan 28 2013

@michaeln: maybe you could take a look?
looks like a good find, do you have a proposed fix in mind?
Oh... there is some code in there that attempts to defeat things like this. Is this not sufficient to defeat the the cases you're seeing? Maybe we need checks like that further down the stack (like in the DatabaseTracker::GetFullDBFilePath method)?

FilePath DatabaseUtil::GetFullFilePathForVfsFile(
    DatabaseTracker* db_tracker, const string16& vfs_file_name) {
  string16 origin_identifier;
  string16 database_name;
  string16 sqlite_suffix;
  if (!CrackVfsFileName(vfs_file_name, &origin_identifier,
                        &database_name, &sqlite_suffix)) {
    return FilePath(); // invalid vfs_file_name
  }

  FilePath full_path = db_tracker->GetFullDBFilePath(
      origin_identifier, database_name);
  if (!full_path.empty() && !sqlite_suffix.empty()) {
    DCHECK(full_path.Extension().empty());
    full_path = full_path.InsertBeforeExtensionASCII(
        UTF16ToASCII(sqlite_suffix));
  }
  // Watch out for directory traversal attempts from a compromised renderer.
  if (full_path.value().find(FILE_PATH_LITERAL("..")) !=
          FilePath::StringType::npos)
    return FilePath();
  return full_path;
}

Comment 5 by aedla@chromium.org, Jan 29 2013

Indeed, the GetFullFilePathForVfsFile is not sufficient. It is only called by IPC-s that open, delete or stat a database file.

Moving the check to GetFullDBFilePath would catch most of the bad cases, but two issues:
1) DatabaseTracker::DeleteOrigin doesn't use it because it constructs an origin directory path not a database file path.
2) There is no error handling for GetFullDBFilePath. All the callers should be updated to check if an empty path was returned.

Another option would be to block bad origins before they are written to tracker database. So maybe check them in DatabaseMessageFilter::OnDatabaseOpened.

Then OnDatabaseModified and OnDatabaseClosed would fail with bad origins because of !IsDatabaseOpened. Shouldn't OnHandleSqliteError have IsDatabaseOpened check as well?

Comment 6 by aedla@chromium.org, Jan 29 2013

There probably used to be another issue, that renderer could open Databases.db directly with vfs_file_name == "Databases.db\0/database_name#".

But that shouldn't be a problem no more, because NULs were recently banned from FilePath.

Comment 7 by parisa@google.com, Feb 7 2013

Hey Michael, any chance you can be an Owner for the fix on this one?
Labels: ReleaseBlock-Stable
@michaeln: this is sufficiently unpleasant to warrant a release block.

Owner: michaeln@chromium.org
ok, what date are we talking about?
@michaeln: if you can't take care of this imminently, please ring alarm bells. Perhaps @aedla would be kind enough to fix this for you (he's pretty awesome ;-)
i'm out of town till next week... sounds like aedla has a real good handle on a potential fix from comment 5... i wont dig into this  (make a CL) until next week at the earliest.
Owner: aedla@chromium.org
I'll take this then.
Cc: jsb...@chromium.org dgro...@chromium.org
Review URL: https://codereview.chromium.org/12212091

@michaeln @jsbell @dgrogan: would someone please take a look?

@jsbell @dgrogan: sorry, not sure why I added IndexedDB guys here. I should get some sleep. Feel free to ignore this.
Labels: WebKit-Storage
Labels: -Mstone-24 Mstone-25
moving m24 bugs to m25.
Cc: thakis@chromium.org
asked thakis@ to rush on final lgtm, skipping the test. This is important for pwnium.
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: Fixed
https://src.chromium.org/viewvc/chrome?view=rev&revision=183141
M26 @ r183643
Labels: -merge-merged-1410 Merge-Approved
Labels: -Merge-Approved Merge-Merged Release-1
M25 @ r184719
Labels: CVE-2013-0911
Project Member

Comment 26 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Type-Security -SecSeverity-High -Area-Internals -SecImpacts-Stable -SecImpacts-Beta -WebKit-Storage -Mstone-25 Security-Impact-Stable Security-Impact-Beta Cr-Content-Storage Cr-Internals Security-Severity-High Type-Bug-Security M-25
Project Member

Comment 27 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-High Security_Severity-High
Project Member

Comment 28 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 29 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member

Comment 30 by bugdroid1@chromium.org, Apr 5 2013

Labels: Cr-Blink
Project Member

Comment 31 by bugdroid1@chromium.org, Apr 6 2013

Labels: -Cr-Content-Storage Cr-Blink-Storage
Labels: -Restrict-View-SecurityNotify
Bulk release of old security bug reports.

Project Member

Comment 33 by sheriffbot@chromium.org, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 34 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 35 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Labels: CVE_description-submitted

Sign in to add a comment