Graceful handling of new versions of IndexedDB serialized data.

third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 39 matching lines @@
 #include "modules/indexeddb/IDBValue.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/SharedBuffer.h"
 #include "wtf/MathExtras.h"
 #include "wtf/Vector.h"
 
 namespace blink {
 
 static v8::Local<v8::Value> deserializeIDBValueData(v8::Isolate*,
                                                     const IDBValue*);
-static v8::Local<v8::Value> deserializeIDBValue(
-    v8::Isolate*,
-    v8::Local<v8::Object> creationContext,
-    const IDBValue*);
 static v8::Local<v8::Value> deserializeIDBValueArray(
     v8::Isolate*,
     v8::Local<v8::Object> creationContext,
     const Vector<RefPtr<IDBValue>>*);
 
 v8::Local<v8::Value> ToV8(const IDBKeyPath& value,
                           v8::Local<v8::Object> creationContext,
                           v8::Isolate* isolate) {
   switch (value.getType()) {
     case IDBKeyPath::NullType:
@@ skipping 297 matching lines @@
     }
     return IDBKey::createArray(result);
   }
 
   ASSERT(keyPath.getType() == IDBKeyPath::StringType);
   return createIDBKeyFromValueAndKeyPath(isolate, value, keyPath.string(),
                                          exceptionState);
 }
 
 // Deserialize just the value data & blobInfo from the given IDBValue.
-// Does not deserialize the key & keypath.
+//
+// Primary key injection is performed in deserializeIDBValue() below.
 static v8::Local<v8::Value> deserializeIDBValueData(v8::Isolate* isolate,
                                                     const IDBValue* value) {
   ASSERT(isolate->InContext());
   if (!value || value->isNull())
     return v8::Null(isolate);
 
   RefPtr<SerializedScriptValue> serializedValue =
       value->createSerializedValue();
   SerializedScriptValue::DeserializeOptions options;
   options.blobInfo = value->blobInfo();
   options.readWasmFromStream = true;
+
+  // deserialize() returns null when serialization fails.  This is sub-optimal
+  // because IndexedDB values can be null, so an application cannot distinguish
+  // between a de-serialization failure and a legitimately stored null value.
+  //
+  // TODO(crbug.com/703704): Ideally, SerializedScriptValue should return an
+  // empty handle on serialization errors, which should be handled by higher
+  // layers. For example, IndexedDB could throw an exception, abort the
+  // transaction, or close the database connection.
   return serializedValue->deserialize(isolate, options);
 }
 
-// Deserialize the entire IDBValue (injecting key & keypath if present).
-static v8::Local<v8::Value> deserializeIDBValue(
-    v8::Isolate* isolate,
-    v8::Local<v8::Object> creationContext,
-    const IDBValue* value) {
+// Deserialize the entire IDBValue.
+//
+// On top of deserializeIDBValueData(), this handles the special case of having
+// to inject a key into the de-serialized value. See injectV8KeyIntoV8Value()
+// for details.
+v8::Local<v8::Value> deserializeIDBValue(v8::Isolate* isolate,
+                                         v8::Local<v8::Object> creationContext,
+                                         const IDBValue* value) {
   ASSERT(isolate->InContext());
   if (!value || value->isNull())
     return v8::Null(isolate);
 
   v8::Local<v8::Value> v8Value = deserializeIDBValueData(isolate, value);
   if (value->primaryKey()) {
     v8::Local<v8::Value> key =
         ToV8(value->primaryKey(), creationContext, isolate);
     if (key.IsEmpty())
       return v8::Local<v8::Value>();

[jsbell, 2017/04/04 16:42:33]

Aside: I wonder if we test this code path?

[pwnall, 2017/04/04 22:41:42]

Nope.

I looked at it... then I thought of what I'd need to do to see what happens
end-to-end... then I looked away :)

Would you like a follow-up CL?

[jsbell, 2017/04/05 17:56:07]

Tracking bug is fine. 

(Without exploring all of the code, this seems like there should be an assertion
here - we should never fail to convert the key to a value. Maybe it comes out of
handling array buffer allocation failures?)

-    bool injected =
-        injectV8KeyIntoV8Value(isolate, key, v8Value, value->keyPath());
-    DCHECK(injected);
+
+    injectV8KeyIntoV8Value(isolate, key, v8Value, value->keyPath());
+
+    // TODO(crbug.com/703704): Throw an error here or at a higher layer if
+    // injectV8KeyIntoV8Value() returns false, which means that the serialized
+    // value got corrupted while on disk.
   }
 
   return v8Value;
 }
 
 static v8::Local<v8::Value> deserializeIDBValueArray(
     v8::Isolate* isolate,
     v8::Local<v8::Object> creationContext,
     const Vector<RefPtr<IDBValue>>* values) {
   ASSERT(isolate->InContext());
 
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
   v8::Local<v8::Array> array = v8::Array::New(isolate, values->size());
   for (size_t i = 0; i < values->size(); ++i) {
     v8::Local<v8::Value> v8Value =
         deserializeIDBValue(isolate, creationContext, values->at(i).get());
     if (v8Value.IsEmpty())
       v8Value = v8::Undefined(isolate);
     if (!v8CallBoolean(array->CreateDataProperty(context, i, v8Value)))
       return v8Undefined();
   }
 
   return array;
 }
 
-// This is only applied to deserialized values which were validated before
-// serialization, so various assumptions/assertions can be made.
+// Injects a primary key into a deserialized V8 value.
+//
+// In general, the value stored in IndexedDB is the serialized version of a
+// value passed to the API. However, the specification has a special case of
+// object stores that specify a key path and have a key generator. In this case,
+// the conceptual description in the spec states that the key produced by the
+// key generator is injected into the value before it is written to IndexedDB.
+//
+// We cannot implementing the spec's conceptual description. We need to assign
+// primary keys in the browser process, to ensure that multiple renderer
+// processes talking to the same database receive sequential keys. At the same
+// time, we want the value serialization code to live in the renderer process,
+// because this type of code is a likely victim to security exploits.
+//
+// We handle this special case by serializing and writing values without the
+// corresponding keys. At read time, we obtain the keys and the values
+// separately, and we inject the keys into values.
 bool injectV8KeyIntoV8Value(v8::Isolate* isolate,
                             v8::Local<v8::Value> key,
                             v8::Local<v8::Value> value,
                             const IDBKeyPath& keyPath) {
   IDB_TRACE("injectIDBV8KeyIntoV8Value");
   ASSERT(isolate->InContext());
 
   ASSERT(keyPath.getType() == IDBKeyPath::StringType);
   Vector<String> keyPathElements = parseKeyPath(keyPath.string());
 
   // The conbination of a key generator and an empty key path is forbidden by
   // spec.
   if (!keyPathElements.size()) {
     ASSERT_NOT_REACHED();
     return false;
   }
 
   v8::HandleScope handleScope(isolate);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
 
   // For an object o = {} which should have keypath 'a.b.c' and key k, this
   // populates o to be {a:{b:{}}}. This is only applied to deserialized
-  // values which were validated before serialization, so various
-  // assumptions can be made, e.g. there are no getters/setters on the
+  // values, so we can assume that there are no getters/setters on the
   // object itself (though there might be on the prototype chain).
+  //
+  // Previous versions of this code assumed that the deserialized value meets
+  // the constraints checked by the serialization validation code. For example,
+  // given a keypath of a.b.c, the code assumed that the de-serialized value
+  // cannot possibly be {a:{b:42}}. This is not a safe assumption.
+  //
+  // IndexedDB's backing store (LevelDB) does use CRC32C to protect against disk
+  // errors. However, this does not prevent corruption caused by bugs in the
+  // higher level code writing invalid values. The following cases are
+  // interesting here.
+  //
+  // (1) Deserialization failures, which are currently handled by returning
+  // null. Disk errors aside, deserialization errors can also occur when a user
+  // switches channels and receives an older build which does not support the
+  // serialization format used by the previous (more recent) build that the user
+  // had.
+  //
+  // (2) Bugs that write a value which is incompatible with the primary key
+  // injection required by the object store. The simplest example is writing
+  // numbers or booleans to an object store with an auto-incrementing primary
+  // keys.
   for (size_t i = 0; i < keyPathElements.size() - 1; ++i) {
+    if (!value->IsObject())
+      return false;
+
     const String& keyPathElement = keyPathElements[i];
-    ASSERT(value->IsObject());
     ASSERT(!isImplicitProperty(isolate, value, keyPathElement));
     v8::Local<v8::Object> object = value.As<v8::Object>();
     v8::Local<v8::String> property = v8String(isolate, keyPathElement);
     bool hasOwnProperty;
     if (!v8Call(object->HasOwnProperty(context, property), hasOwnProperty))
       return false;
     if (hasOwnProperty) {
       if (!object->Get(context, property).ToLocal(&value))
         return false;
     } else {
       value = v8::Object::New(isolate);
       if (!v8CallBoolean(object->CreateDataProperty(context, property, value)))
         return false;
     }
   }
 
   // Implicit properties don't need to be set. The caller is not required to
   // be aware of this, so this is an expected no-op. The caller can verify
   // that the value is correct via assertPrimaryKeyValidOrInjectable.
   if (isImplicitProperty(isolate, value, keyPathElements.back()))
     return true;
 
-  // If it's not an implicit property of value, value must be an object.
+  // If the key path does not point to an implicit property, the value must be
+  // an object. Previous code versions DCHECKed this, which is unsafe in the
+  // event of database corruption.

[jsbell, 2017/04/04 16:42:33]

maybe append "or undetected version skew."

[pwnall, 2017/04/04 22:41:42]

Done.

+  if (!value->IsObject())
+    return false;
+
   v8::Local<v8::Object> object = value.As<v8::Object>();
   v8::Local<v8::String> property = v8String(isolate, keyPathElements.back());
   if (!v8CallBoolean(object->CreateDataProperty(context, property, key)))
     return false;
 
   return true;
 }
 
 // Verify that an value can have an generated key inserted at the location
 // specified by the key path (by injectV8KeyIntoV8Value) when the object is
@@ skipping 107 matching lines @@
   if (expectedKey && expectedKey->isEqual(value->primaryKey()))
     return;
 
   bool injected = injectV8KeyIntoV8Value(
       isolate, keyValue.v8Value(), scriptValue.v8Value(), value->keyPath());
   DCHECK(injected);
 }
 #endif
 
 }  // namespace blink