enterprise.platformKeys: Copy-on-read the 'algorithm' member of Key objects.

enterprise.platformKeys: Copy-on-read the 'algorithm' member of Key objects.

With this change, a Key object's algorithm member is copied on every read access. Effectively preventing it's modification as required by the WebCrypto specification.

BUG=385143
R=bartfab@chromium.org, kalman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278833

[pneubeck (no reviews), 2014-06-16 17:53:34]

Benjamin/Eric, I'd like to get your feedback on the recursive freeze. I'm not
sure yet, how to prevent modification of the publicExponent. I could implement a
getter that does a copy on read, if it's really required.

[not at google - send to devlin, 2014-06-16 17:58:33]

https://codereview.chromium.org/332233002/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key.js
(right):

https://codereview.chromium.org/332233002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/enterprise_platform_keys/key.js:56:
deepFreeze(this.algorithm);
why do you need this? the way that you're exposing Key, with {readonly:
['algorithm']}, should prevent it from being modified. or am I missing
something?

also I don't know what part of this test is functional and what part is test
cleanup.

[pneubeck (no reviews), 2014-06-16 18:01:19]

https://codereview.chromium.org/332233002/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key.js
(right):

https://codereview.chromium.org/332233002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/enterprise_platform_keys/key.js:56:
deepFreeze(this.algorithm);
On 2014/06/16 17:58:32, kalman wrote:
> why do you need this? the way that you're exposing Key, with {readonly:
> ['algorithm']}, should prevent it from being modified. or am I missing
> something?
algorithm is an Object and defineProperty doesn't prevent modifying nested
properties. It only prevents assign another object to the member 'algorithm' of
the Key object.

> 
> also I don't know what part of this test is functional and what part is test
> cleanup.
Sorry. I'll separate it into two CLs.

[not at google - send to devlin, 2014-06-16 18:08:32]

https://codereview.chromium.org/332233002/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key.js
(right):

https://codereview.chromium.org/332233002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/enterprise_platform_keys/key.js:56:
deepFreeze(this.algorithm);
On 2014/06/16 18:01:18, pneubeck wrote:
> On 2014/06/16 17:58:32, kalman wrote:
> > why do you need this? the way that you're exposing Key, with {readonly:
> > ['algorithm']}, should prevent it from being modified. or am I missing
> > something?
> algorithm is an Object and defineProperty doesn't prevent modifying nested
> properties. It only prevents assign another object to the member 'algorithm'
of
> the Key object.

ah right. "deepFreeze" is a great name.

I find it a little odd that a sideeffect of constructing a KeyImpl is to freeze
an object that it's given. Better to either ensure that the algorithm can't be
modified from outside the function (i.e. that an unmodifiable algorithm is
passed in) or KeyImpl should make a copy of it.

[pneubeck (no reviews), 2014-06-16 18:37:19]

On 2014/06/16 18:08:32, kalman wrote:
>
https://codereview.chromium.org/332233002/diff/60001/chrome/renderer/resource...
> File chrome/renderer/resources/extensions/enterprise_platform_keys/key.js
> (right):
> 
>
https://codereview.chromium.org/332233002/diff/60001/chrome/renderer/resource...
> chrome/renderer/resources/extensions/enterprise_platform_keys/key.js:56:
> deepFreeze(this.algorithm);
> On 2014/06/16 18:01:18, pneubeck wrote:
> > On 2014/06/16 17:58:32, kalman wrote:
> > > why do you need this? the way that you're exposing Key, with {readonly:
> > > ['algorithm']}, should prevent it from being modified. or am I missing
> > > something?
> > algorithm is an Object and defineProperty doesn't prevent modifying nested
> > properties. It only prevents assign another object to the member 'algorithm'
> of
> > the Key object.
> 
> ah right. "deepFreeze" is a great name.
> 
> I find it a little odd that a sideeffect of constructing a KeyImpl is to
freeze
> an object that it's given. Better to either ensure that the algorithm can't be
> modified from outside the function (i.e. that an unmodifiable algorithm is
> passed in) or KeyImpl should make a copy of it.

Ryan clarified on the bug, that a copy of the algorithm member on each read is
acceptable.
I'll update the CL.

[pneubeck (no reviews), 2014-06-17 21:12:04]

-kalman (there is now deepFreeze anymore ;-) )

@Eric, this should now correctly prevent modifications of the hash parameter.
ptal.

[pneubeck (no reviews), 2014-06-18 09:09:50]

@Bartosz, please take a look.

[bartfab (slow), 2014-06-18 16:19:06]

lgtm

https://codereview.chromium.org/332233002/diff/140001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js
(right):

https://codereview.chromium.org/332233002/diff/140001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:284:
fail('Expected that the property is read only and an exception to ' +
Nit: Grammar: "Expected the property to be read-only and an exception to be
thrown."

[eroman, 2014-06-18 18:12:13]

The change itself looks fine, however I am not familiar enough with the
javascript runtime to know if this _guarantees_ that the underlying key object
cannot be modified. Perhaps is it possible to install getter/settingers on
Object.proptotype or some such hack to still override the algorith.hash.name?

[pneubeck (no reviews), 2014-06-18 18:18:02]

Adding Benjamin back; do have any concern with this copy-on-read?

[pneubeck (no reviews), 2014-06-18 22:30:27]

@Benjamin, ptal

[pneubeck (no reviews), 2014-06-18 22:47:43]

@Benjamin, please check the commit flag if you're ok with the change. With this
and https://codereview.chromium.org/331173002/ being committed, all requirements
for switching the AppExt flag should be met.

Thanks for all the review/design support!

[not at google - send to devlin, 2014-06-19 00:12:43]

consider the comment about using WebSerializedScriptValue. I'm hesitant to CQ
this for you now, but assuming you choose not to go that route and commit more
or less what you have, lgtm for that.

but the WebSerializedScriptValue thing would be super handy and neat :)

https://codereview.chromium.org/332233002/diff/180001/extensions/renderer/res...
File extensions/renderer/resources/utils.js (right):

https://codereview.chromium.org/332233002/diff/180001/extensions/renderer/res...
extensions/renderer/resources/utils.js:131: * prototypes).
there's actually a really sneaky way you can implement this method I think: use
SerializedScriptValue, the utility that IndexedDB uses to serialize and
deserialize values. that should handle everything: cycles, exotic types like
uint8array...

see here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It seems as though this function could be:

function deepCopy(value) {
  return utilsNatives.DeepCopy(value);
}

with DeepCopy defined in utils_native_handler.cc:

void DeepCopy(v8::Arguments args) {
  args.SetResult(
      WebSerializedScriptValue::serialize(args[0]).deserialize());
}

https://codereview.chromium.org/332233002/diff/180001/extensions/renderer/res...
extensions/renderer/resources/utils.js:144: } else if
(value.__proto__.constructor.name === 'Uint8Array') {
I don't think you need the __proto__ ? just value.constructor.name should be
fine, or is there some tricky thing that could happen?

https://codereview.chromium.org/332233002/diff/180001/extensions/renderer/res...
extensions/renderer/resources/utils.js:153: copy[key] = deepCopy(value[key]);
nit: I would invert the hasOwnProperty check to save 1 line :)

[pneubeck (no reviews), 2014-06-19 16:09:42]

@Benjamin, ptal

https://codereview.chromium.org/332233002/diff/180001/extensions/renderer/res...
File extensions/renderer/resources/utils.js (right):

https://codereview.chromium.org/332233002/diff/180001/extensions/renderer/res...
extensions/renderer/resources/utils.js:131: * prototypes).
On 2014/06/19 00:12:42, kalman wrote:
> there's actually a really sneaky way you can implement this method I think:
use
> SerializedScriptValue, the utility that IndexedDB uses to serialize and
> deserialize values. that should handle everything: cycles, exotic types like
> uint8array...
> 
> see here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> It seems as though this function could be:
> 
> function deepCopy(value) {
>   return utilsNatives.DeepCopy(value);
> }
> 
> with DeepCopy defined in utils_native_handler.cc:
> 
> void DeepCopy(v8::Arguments args) {
>   args.SetResult(
>       WebSerializedScriptValue::serialize(args[0]).deserialize());
> }

Done.

https://codereview.chromium.org/332233002/diff/200001/extensions/renderer/res...
File extensions/renderer/resources/utils.js (right):

https://codereview.chromium.org/332233002/diff/200001/extensions/renderer/res...
extensions/renderer/resources/utils.js:133: return nativeDeepCopy(value);
this wrapper function is not strictly required, but helpful for debugging and
this documentation might be found more easily.

[not at google - send to devlin, 2014-06-19 17:23:13]

lgtm

https://codereview.chromium.org/332233002/diff/200001/extensions/renderer/res...
File extensions/renderer/resources/utils.js (right):

https://codereview.chromium.org/332233002/diff/200001/extensions/renderer/res...
extensions/renderer/resources/utils.js:133: return nativeDeepCopy(value);
On 2014/06/19 16:09:41, pneubeck wrote:
> this wrapper function is not strictly required, but helpful for debugging and
> this documentation might be found more easily.

yes I like it

[pneubeck (no reviews), 2014-06-19 17:24:07]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-19 17:26:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/332233002/200001

[commit-bot: I haz the power, 2014-06-19 22:43:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 22:43:03]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[pneubeck (no reviews), 2014-06-20 14:43:09]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-06-20 14:43:25]

The CQ bit was unchecked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-06-20 15:00:47]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-20 15:03:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/332233002/220001

[commit-bot: I haz the power, 2014-06-20 19:06:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-20 19:15:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:15:44]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[pneubeck (no reviews), 2014-06-20 20:26:30]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-20 20:28:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/332233002/220001

[commit-bot: I haz the power, 2014-06-20 20:40:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-20 20:48:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 20:48:06]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[pneubeck (no reviews), 2014-06-20 21:59:57]

Committed patchset #7 manually as r278833 (presubmit successful).