New issue
Advanced search Search tips

Issue 704293 link

Starred by 0 users
Status: Fixed
Owner:
jbroman@chromium.org
Closed: May 2017
Cc:
jsb...@chromium.org
pwnall@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Fix WebSerializedScriptValueVersion logic for IDB to account for serializer rewrite

Project Member Reported by jbroman@chromium.org, Mar 22 2017 
If I understand correctly, the logic today works like this:

- when opening a database, read a metadata entry that stores the format version
- if it is greater than the current one (as seen in WebSerializedScriptValueVersion.h), reject it
- else, update to that version number

This is less trivial now, because part of the serialization logic lives in V8, which is versioned separately from Blink. This means that V8 might introduce incompatible changes to the serialization of basic objects, or Blink might introduce incompatible changes to the serialization of web DOM objects.

In the short term, I think I can bump WebSerializedScriptValueVersion.h to match the Blink-side version.

The proper natural equivalent would seem to be a (V8Version, BlinkVersion) tuple, which is "in the future" if either version is greater than the known current one. But this logic currently happens in the browser process, from which we probably ought not to be including v8 headers or referencing v8 symbols (even referencing Blink is suspect, though it's fine in this case because it's just a constant).

One thing that seems conceptually clean is that the IndexedDB client ought to be telling the service what version logic it supports, rather than the service magically sharing this information via a header. It doesn't seem trivial to plumb this through the IndexedDB infrastucture, though. WDYT, jsbell?

Comment 1 by jsb...@chromium.org, Mar 22 2017

Cc: pwnall@chromium.org
> ...a (V8Version, BlinkVersion) tuple...

Yep. It's already logically a tuple (db_version, blink_script_version) so we're extending it with one more. 

> ...even referencing Blink is suspect...

Yeah, we agonized about that at the time. I think this predates moving the browser code out of webkit (yes, we used to run webkit in the browser...) and left this in because we didn't want to do the plumbing change in the middle of that refactor...

> ... the IndexedDB client ought to be telling the service what version logic it supports...

SGTM. 

> It doesn't seem trivial...

It's a long slog due to thread hops and incomplete servicification... but not too bad:

IDBFactory::openInternal
WebIDBFactoryImpl::open
WebIDBFactoryImpl::IOThreadHelper::Open
content/common/indexed_db/indexed_db.mojom.h
IndexedDBDispatcherHost::Open
IndexedDBDispatcherHost::OpenOnIDBThread
IndexedDBFactoryImpl::Open
IndexedDBFactoryImpl::OpenBackingStore
IndexedDBFactoryImpl::OpenBackingStoreHelper
IndexedDBBackingStore::Open (and IsSchemaKnown)
IndexedDBBackingStore::SetUpMetadata

(If the backing store is already open we assume we're not dealing with version skew *within* chrome)

Comment 3 by jbroman@chromium.org, Mar 23 2017 

2 quick questions:

Because the backing store can be opened for any of the indexed_db::mojom::Factory methods, it seems that I'd either need a parameter on each, or a "configure" method call that happens when the service is connected to, to cache the value on the IndexedDBDispatcherHost. I'm slightly inclined to prefer the latter.

In either case, do you have an opinion on what should happen if the declared version differs from one provided when the backing store was previously opened? (This should never happen, unless a compromised renderer is playing games.) We could make it fail in some way, but I'm kind of inclined to log a message (we shouldn't crash the browser, though, IIUC) and continue.

Comment 4 by jsb...@chromium.org, Mar 23 2017 

Yeah, the configure approach seems forward looking.

Log and continue is fine. mojo::ReportBadMessage() is also an option - I believe that terminates a corrupt/compromised renderer.

Thank you *so* much for taking this on - it's awesome that you're following up on this. Ping pwnall@ and dmurph@ as necessary for help!
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 4 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e46f841849ef541e31c2cc4891c1ce44e57285b1

commit e46f841849ef541e31c2cc4891c1ce44e57285b1
Author: jbroman <jbroman@chromium.org>
Date: Tue Apr 04 19:26:32 2017

Export the current data format version used by ValueSerializer.

This enables clients like IndexedDB to know when the data format version has
decreased (i.e. the user has switched to an earlier version) and deal with the
resulting incompatibility up front.

BUG= chromium:704293 

Review-Url: https://codereview.chromium.org/2772723005
Cr-Commit-Position: refs/heads/master@{#44391}

[modify] https://crrev.com/e46f841849ef541e31c2cc4891c1ce44e57285b1/include/v8.h
[modify] https://crrev.com/e46f841849ef541e31c2cc4891c1ce44e57285b1/src/api.cc
[modify] https://crrev.com/e46f841849ef541e31c2cc4891c1ce44e57285b1/src/value-serializer.cc
[modify] https://crrev.com/e46f841849ef541e31c2cc4891c1ce44e57285b1/src/value-serializer.h
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 20 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ee0c8ab5180fb87ce012b784bb47a3da507c1a0

commit 3ee0c8ab5180fb87ce012b784bb47a3da507c1a0
Author: jbroman <jbroman@chromium.org>
Date: Thu Apr 20 14:25:03 2017

Use a two-part data format version in IndexedDB metadata.

This breaks the 64-bit data format version stored in the database metadata
into two 32-bit parts (V8 and Blink format versions), and allows them to be
compared by checking that the database uses supported versions of both.

If either component has reverted, the database is deemed to be from the future
(i.e. corrupt).

BUG= 704293 

Review-Url: https://codereview.chromium.org/2773823002
Cr-Commit-Position: refs/heads/master@{#466002}

[modify] https://crrev.com/3ee0c8ab5180fb87ce012b784bb47a3da507c1a0/content/browser/BUILD.gn
[modify] https://crrev.com/3ee0c8ab5180fb87ce012b784bb47a3da507c1a0/content/browser/indexed_db/indexed_db_backing_store.cc
[add] https://crrev.com/3ee0c8ab5180fb87ce012b784bb47a3da507c1a0/content/browser/indexed_db/indexed_db_data_format_version.cc
[add] https://crrev.com/3ee0c8ab5180fb87ce012b784bb47a3da507c1a0/content/browser/indexed_db/indexed_db_data_format_version.h
[modify] https://crrev.com/3ee0c8ab5180fb87ce012b784bb47a3da507c1a0/content/browser/indexed_db/indexed_db_factory_unittest.cc
[modify] https://crrev.com/3ee0c8ab5180fb87ce012b784bb47a3da507c1a0/content/browser/indexed_db/leveldb_coding_scheme.md
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd28a17e10afc0914d2784718df63e9ce3626076

commit cd28a17e10afc0914d2784718df63e9ce3626076
Author: jbroman <jbroman@chromium.org>
Date: Wed Apr 26 19:48:50 2017

Comment out the single use of v8::ValueSerializer::GetCurrentDataFormatVersion while it changes.

This is becoming a compile-time constant, and its name is changing in that
process. I'll restore this (non-critical) assertion once that change has
stuck.

R=pwnall@chromium.org
BUG= 704293 

Review-Url: https://codereview.chromium.org/2847463002
Cr-Commit-Position: refs/heads/master@{#467418}

[modify] https://crrev.com/cd28a17e10afc0914d2784718df63e9ce3626076/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp

Comment 10 by jbroman@chromium.org, May 9 2017

Status: Fixed (was: Started)
►

Sign in to add a comment