Security: V8: incorrect bounds calculation for StringIndexOf operator |
|||||||||||||||||
Issue descriptionThis is a variant of issue #762874 . I believe that the incorrect code is unreachable, which would mean that this currently has no security impact, but I'm not sure. I am marking this as a security bug for two reasons: - Maybe I'm wrong and the code is reachable after all. - Derestricting this bug would unnecessarily hint at issue #762874 . VULNERABILITY DETAILS The problematic code is in v8/src/compiler/typer.cc, in the visitor for the StringIndexOf operator: Type* Typer::Visitor::TypeStringIndexOf(Node* node) { return Type::Range(-1.0, String::kMaxLength - 1.0, zone()); } String.prototype.indexOf can return String::kMaxLength when called as `str.indexOf('', str.length)`, so this range is incorrect. As far as I can tell, the incorrect function can only be called if it runs during (or after?) the TypedLoweringPhase because calls to builtins start out as JSCall operators that are only converted to builtin operators using JSBuiltinReducer in the TypedLoweringPhase. The Typer is invoked in the following cases: - The Typer runs recursively during the TyperPhase, which happens before the TypedLoweringPhase. As far as I can tell, there is no way for a StringIndexOf operator to exist in the graph at this point. - The Typer runs non-recursively when a graph node is created using Graph::NewNode or cloned using Graph::CloneNode during the TypedLoweringPhase. The Typer can be invoked on e.g. StringFromCharCode nodes because the JSBuiltinReducer substitutes kStringFromCharCode calls using Graph::NewNode: Node* input = ToNumber(r.GetJSCallInput(0)); Node* value = graph()->NewNode(simplified()->StringFromCharCode(), input); return Replace(value); However, for kStringIndexOf, the JSBuiltinReducer just swaps out the contents of the Node, which doesn't cause the Typer to recompute the type: RelaxEffectsAndControls(node); node->ReplaceInput(0, receiver); node->ReplaceInput(1, search_string); node->ReplaceInput(2, position); node->TrimInputCount(3); NodeProperties::ChangeOp(node, simplified()->StringIndexOf()); return Changed(node); I think that Typer::Visitor::TypeStringIndexOf should either be changed to call UNREACHABLE() (like e.g. Typer::Visitor::TypeStringToLowerCaseIntl) or be fixed up analog to V8 commit b8f144ec4fd1cd808f0d883668f355498b56d7fa. I would like to thank sroettger@ for showing me issue #762874 .
,
Sep 28 2017
,
Sep 29 2017
Markinh as high severity out of abundance of caution. ishell@, please feel free to mark security impact none if no security implications as per c#0.
,
Sep 29 2017
Assigning ulan@ because ishell@ is on vacation.
,
Sep 29 2017
Re-assigning to the current clussterfuzz sheriff.
,
Oct 13 2017
rossberg: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers? If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one? If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 13 2017
Not a clusterfuzz issue but range analysis in the typer. @neis, please have a look.
,
Oct 13 2017
,
Oct 13 2017
,
Oct 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/d38488558ba3aa281a4fbaee0b9b074dbb21e718 commit d38488558ba3aa281a4fbaee0b9b074dbb21e718 Author: Jakob Gruber <jgruber@chromium.org> Date: Fri Oct 13 17:30:39 2017 [turbofan] Mark Typer::Visitor::TypeStringIndexOf unreachable The type of StringIndexOf nodes is never recomputed since the operation is simply changed on the original node. Bug: chromium:769923 Change-Id: I3a2956ea69d43a56d22aff0607ac9869cf65533c Reviewed-on: https://chromium-review.googlesource.com/718758 Reviewed-by: Jaroslav Sevcik <jarin@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#48558} [modify] https://crrev.com/d38488558ba3aa281a4fbaee0b9b074dbb21e718/src/compiler/typer.cc
,
Oct 16 2017
Fixed in #10 according to the analysis in the OP. A backport shouldn't be necessary since the affected code is unreachable.
,
Oct 16 2017
,
Oct 27 2017
,
Oct 27 2017
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 27 2017
Please add appropriate OSs.
,
Oct 27 2017
Like I said in #11, this won't need a backmerge. I'm not sure why sheriffbot@ decided to Merge-Request-63? Also lowering priority.
,
Oct 28 2017
Remvoing "Merge-Review-63" label per comment #16. +awhalley@ (Security TPM) as FYI, please let me know if there is any concern here. Thank you.
,
Nov 6 2017
,
Nov 22 2017
Per discussion on irc, bugs in unreachable code are not security bugs. So I'm dropping bug-security but keeping this as view restricted.
,
Jan 22 2018
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by elawrence@chromium.org
, Sep 28 2017