Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(172)

Issue 2828773002: Replace v8::Object::Set with CreateDataProperty for security reasons. (Closed)

Created:
3 years, 3 months ago by dmazzoni
Modified:
3 years, 1 month ago
Reviewers:
Devlin, jbroman
CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -25 lines) Patch
M chrome/renderer/extensions/automation_internal_custom_bindings.cc View 1 2 3 4 6 chunks +41 lines, -25 lines 0 comments Download

Messages

Total messages: 35 (22 generated)
dmazzoni
Devlin, I'm not sure how familiar you are with v8 and the security implications of ...
3 years, 3 months ago (2017-04-19 16:44:48 UTC) #4
Devlin
I'm getting slowly more familiar here, but jbroman is still much more expert than I. ...
3 years, 3 months ago (2017-04-19 19:01:51 UTC) #10
jbroman
On 2017/04/19 at 19:01:51, rdevlin.cronin wrote: > I'm getting slowly more familiar here, but jbroman ...
3 years, 3 months ago (2017-04-19 19:17:35 UTC) #13
dmazzoni
I added a comment. OK to land this change as-is, or anyone else that might ...
3 years, 3 months ago (2017-04-24 19:28:04 UTC) #16
dmazzoni
I added a comment. OK to land this change as-is, or anyone else that might ...
3 years, 3 months ago (2017-04-24 19:28:05 UTC) #17
Devlin
https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensions/automation_internal_custom_bindings.cc File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensions/automation_internal_custom_bindings.cc#newcode52 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/extensions/automation_internal_custom_bindings.cc#newcode61 chrome/renderer/extensions/automation_internal_custom_bindings.cc:61: ...
3 years, 3 months ago (2017-04-24 20:01:43 UTC) #18
jbroman
lgtm (fwiw) I have a thought about how to make this easier to get right, ...
3 years, 3 months ago (2017-04-25 17:57:45 UTC) #21
dmazzoni
https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensions/automation_internal_custom_bindings.cc File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2828773002/diff/40001/chrome/renderer/extensions/automation_internal_custom_bindings.cc#newcode52 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: > ...
3 years, 3 months ago (2017-04-26 00:09:35 UTC) #23
Devlin
lgtm https://codereview.chromium.org/2828773002/diff/60001/chrome/renderer/extensions/automation_internal_custom_bindings.cc File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2828773002/diff/60001/chrome/renderer/extensions/automation_internal_custom_bindings.cc#newcode52 chrome/renderer/extensions/automation_internal_custom_bindings.cc:52: const base::StringPiece& str) { nitty nit: base::StringPiece is ...
3 years, 3 months ago (2017-04-26 00:25:21 UTC) #25
dmazzoni
https://codereview.chromium.org/2828773002/diff/60001/chrome/renderer/extensions/automation_internal_custom_bindings.cc File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right): https://codereview.chromium.org/2828773002/diff/60001/chrome/renderer/extensions/automation_internal_custom_bindings.cc#newcode52 chrome/renderer/extensions/automation_internal_custom_bindings.cc:52: const base::StringPiece& str) { On 2017/04/26 00:25:21, Devlin wrote: ...
3 years, 3 months ago (2017-04-26 02:32:24 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2828773002/80001
3 years, 3 months ago (2017-04-26 02:33:09 UTC) #31
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/368d0cd01a9c13281b2526feb9dc083b27f2af91
3 years, 3 months ago (2017-04-26 04:03:50 UTC) #34
pdknsk
3 years, 1 month ago (2017-06-19 08:48:04 UTC) #35
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

Powered by Google App Engine
This is Rietveld 408576698