Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2828773002:
Replace v8::Object::Set with CreateDataProperty for security reasons. (Closed)
DescriptionReplace v8::Object::Set with CreateDataProperty for security reasons.
A malicious author can use Object.defineProperty to override the setter for
specific object properties and trigger arbitrary code in the middle of
automation extension API bindings. Fix that by using CreateDataProperty
instead.
BUG=689396
Review-Url: https://codereview.chromium.org/2828773002
Cr-Commit-Position: refs/heads/master@{#467229}
Committed: https://chromium.googlesource.com/chromium/src/+/368d0cd01a9c13281b2526feb9dc083b27f2af91
Patch Set 1 #Patch Set 2 : Accidentally deleted an important case #Patch Set 3 : Added comment #
Total comments: 8
Patch Set 4 : Address feedback #
Total comments: 2
Patch Set 5 : StringPiece doesn't need to be a reference #Messages
Total messages: 35 (22 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmazzoni@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, I'm not sure how familiar you are with v8 and the security implications of calling Set vs CreateDataProperty - could you suggest a good reviewer if you're unsure?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rdevlin.cronin@chromium.org changed reviewers: + jbroman@chromium.org
I'm getting slowly more familiar here, but jbroman is still much more expert than I. He should take a look. However, I think the biggest issue here is that tree caches can be invalidated when untrusted JS runes (e.g. by removing the frame). I'm not sure that using CreateDataProperty would be enough to entirely remove that concern. For instance, CreateDataProperty() could fail if there's already a non-configurable property of the same name on the object (as could be the case if the untrusted code used Object.defineProperty()). There might be a clever way that the attack could leverage that to run their own code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/19 at 19:01:51, rdevlin.cronin wrote: > I'm getting slowly more familiar here, but jbroman is still much more expert than I. He should take a look. > > However, I think the biggest issue here is that tree caches can be invalidated when untrusted JS runes (e.g. by removing the frame). I'm not sure that using CreateDataProperty would be enough to entirely remove that concern. For instance, CreateDataProperty() could fail if there's already a non-configurable property of the same name on the object (as could be the case if the untrusted code used Object.defineProperty()). There might be a clever way that the attack could leverage that to run their own code. It appears to me that this is only used on freshly created objects (via v8::Object::New), so I don't think authors could mess with the created object, just with its prototype chain. This should also prevent it from being any of the kinds of weird objects that might object to it (e.g. access checks, Object.preventExtensions, etc.). w.r.t. non-configurable properties, if Object.prototype has a non-configurable property, that doesn't prevent other objects from adding a property with the same name (hiding it), just from it being reconfigured on Object.prototype itself. So I think this implementation of SafeSetV8Property ought to be safe here (though it's subtle enough that it might warrant a comment justifying why the steps it takes suffices to be safe).
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I added a comment. OK to land this change as-is, or anyone else that might want to weigh in?
I added a comment. OK to land this change as-is, or anyone else that might want to weigh in?
https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:52: return v8::String::NewFromUtf8(isolate, str.c_str(), nit: Let's use gin::StringToSymbol() https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:61: // things to happen in the middle of one of our functions below. I'd expand this to say that this is only safe if the |object| is passed into this function before it can be exposed to any untrusted script. This isn't a panacea for getters/setters, and only works because we know that the object can't have any getters/setters installed yet. https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:64: const std::string& key, s/const std::string&/base::StringPiece. Since these are all passed in as raw strings, no need to allocate a full std::string. https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:69: LOG(ERROR) << "Failed to set property with key " << key; We should at least NOTREACHED() here. Maybe even CHECK(maybe.ToChecked()), since we don't think this is possible.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm (fwiw) I have a thought about how to make this easier to get right, but I don't think this CL need block on it.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:52: return v8::String::NewFromUtf8(isolate, str.c_str(), On 2017/04/24 20:01:42, Devlin wrote: > nit: Let's use gin::StringToSymbol() Done. https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:61: // things to happen in the middle of one of our functions below. On 2017/04/24 20:01:42, Devlin wrote: > I'd expand this to say that this is only safe if the |object| is passed into > this function before it can be exposed to any untrusted script. This isn't a > panacea for getters/setters, and only works because we know that the object > can't have any getters/setters installed yet. Done. https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:64: const std::string& key, On 2017/04/24 20:01:42, Devlin wrote: > s/const std::string&/base::StringPiece. > > Since these are all passed in as raw strings, no need to allocate a full > std::string. Done. https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:69: LOG(ERROR) << "Failed to set property with key " << key; On 2017/04/24 20:01:42, Devlin wrote: > We should at least NOTREACHED() here. Maybe even CHECK(maybe.ToChecked()), > since we don't think this is possible. Done. Note that maybe.ToChecked() is the same as maybe.FromJust().
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2828773002/diff/60001/chrome/renderer/extensi... File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2828773002/diff/60001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:52: const base::StringPiece& str) { nitty nit: base::StringPiece is designed to be super cheap to copy; I'd just pass it by value rather than reference.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2828773002/diff/60001/chrome/renderer/extensi... File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2828773002/diff/60001/chrome/renderer/extensi... chrome/renderer/extensions/automation_internal_custom_bindings.cc:52: const base::StringPiece& str) { On 2017/04/26 00:25:21, Devlin wrote: > nitty nit: base::StringPiece is designed to be super cheap to copy; I'd just > pass it by value rather than reference. Done.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2828773002/#ps80001 (title: "StringPiece doesn't need to be a reference")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493173952426890,
"parent_rev": "6fd672f43f11c93d78c93f39c9d0fb9a50d98375", "commit_rev":
"368d0cd01a9c13281b2526feb9dc083b27f2af91"}
Message was sent while issue was closed.
Description was changed from ========== Replace v8::Object::Set with CreateDataProperty for security reasons. A malicious author can use Object.defineProperty to override the setter for specific object properties and trigger arbitrary code in the middle of automation extension API bindings. Fix that by using CreateDataProperty instead. BUG=689396 ========== to ========== Replace v8::Object::Set with CreateDataProperty for security reasons. A malicious author can use Object.defineProperty to override the setter for specific object properties and trigger arbitrary code in the middle of automation extension API bindings. Fix that by using CreateDataProperty instead. BUG=689396 Review-Url: https://codereview.chromium.org/2828773002 Cr-Commit-Position: refs/heads/master@{#467229} Committed: https://chromium.googlesource.com/chromium/src/+/368d0cd01a9c13281b2526feb9dc... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/368d0cd01a9c13281b2526feb9dc...
Message was sent while issue was closed.
I don't know how, but this made numbers locale-formatted. https://bugs.chromium.org/p/chromium/issues/detail?id=720222 |
