jochen@, can you please take a look at this as it is V8 and PDFium? Also can you triage as I am not sure what the implication of this is?

PDFium has a notion that it's the only thing that sets internal fields (to a count of two), so that anything that has these is one of its own.  We've know that this is fragile for some time, but I didn't think v8 made any objects like that.  Hence the line in GetObjDefnID that checks before accessing

 if (pObj.IsEmpty() || !pObj->InternalFieldCount())
    return -1;

(we should probably check for two fields), we'd be safe.


except |this| enters the picture, which violates all those assumptions, even after the fix for the prior issue, it remains wrong.

reply to Comment 3:
v8 do have some objects which have exactly two internal fields, such as new Intl.NumberFormat(),new Intl.Collator, new Intl.DateFormat and so on, the internal filed 0 point to an inner Intl object, whose first word is a point to a virtual table,
pdfium will take the intl object as a CFXJS_PerObjectData object, and take the virtual table point as g_nObjDefnID, just because the virtual table point is not equal to CJS_TimerObj::g_nObjDefnID, the function return. and other v8 objects such as ArrayBuffer DateView, Wasm instance object have internal field too, maybe some of them can bypass the check "if (CFXJS_Engine::GetObjDefnID(pObj) != CJS_TimerObj::g_nObjDefnID)" and move on, in that case, this bug will be exploited easily.

d8> %DebugPrint(new Intl.v8BreakIterator())
DebugPrint: 0x2959c7c9: [JS_OBJECT_TYPE]
 - map = 0x3d10c769 [FastProperties]
 - prototype = 0x4df096a9
 - elements = 0x3b404125 <FixedArray[0]> [FAST_HOLEY_SMI_ELEMENTS]
 - internal fields: 2
 - properties = 0x2959c9cd <FixedArray[3]> {
    0x3b4056f1 <Symbol: intl_initialized_marker_symbol>: 0x3b467931 <String[13]: breakiterator> (data field 0)
    0x3b40575d <Symbol: intl_resolved_symbol>: 0x2959c591 <an Object with map 0x3d10c6e5> (data field 1)
 }
 - internal fields = {   ----------------------->two internal fields
    85819612
    0
 }
0x3d10c769: [Map]
 - type: JS_OBJECT_TYPE
 - instance size: 20
 - inobject properties: 0
 - elements kind: FAST_HOLEY_SMI_ELEMENTS
 - unused property fields: 1
 - enum length: invalid
 - stable_map
 - back pointer: 0x3d10c711 <Map(FAST_HOLEY_SMI_ELEMENTS)>
 - instance descriptors (own) #2: 0x2959c9e1 <FixedArray[8]>
 - prototype: 0x4df096a9 <an Object with map 0x3d10c73d>
 - constructor: 0x4df095e5 <JS Function v8BreakIterator (SharedFunctionInfo 0x3b418ef5)>
 - code cache: 0x3b404125 <FixedArray[0]>
 - dependent code: 0x3b404125 <FixedArray[0]>
 - construction counter: 0

{}
d8> %DebugPrint(new ArrayBuffer);
DebugPrint: 0x2959cb65: [JSArrayBuffer]
 - map = 0x3d106889 [FastProperties]
 - prototype = 0x4df0fdd5
 - elements = 0x3b404125 <FixedArray[0]> [FAST_HOLEY_SMI_ELEMENTS]
 - internal fields: 2
 - backing_store = 0
 - byte_length = 0
 - properties = 0x3b404125 <FixedArray[0]> {}
 - internal fields = {         ----------------------->two internal fields
    0
    0
 }
0x3d106889: [Map]
 - type: JS_ARRAY_BUFFER_TYPE
 - instance size: 32
 - inobject properties: 0
 - elements kind: FAST_HOLEY_SMI_ELEMENTS
 - unused property fields: 0
 - enum length: invalid
 - stable_map
 - back pointer: 0x3b4041a1 <undefined>
 - instance descriptors (own) #0: 0x3b40411d <FixedArray[0]>
 - prototype: 0x4df0fdd5 <an Object with map 0x3d1068b5>
 - constructor: 0x4df0fd9d <JS Function ArrayBuffer (SharedFunctionInfo 0x3b4437b1)>
 - code cache: 0x3b404125 <FixedArray[0]>
 - dependent code: 0x3b404125 <FixedArray[0]>
 - construction counter: 0

[object ArrayBuffer]

Fair enough, we should use something with more entropy than just integers starting at 0 to identify these.

https://pdfium-review.googlesource.com/2847 landed, jochen to review.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/11e15a648963d343125c2bb95369f4bfa178ea69

commit 11e15a648963d343125c2bb95369f4bfa178ea69
Author: pdfium-deps-roller <pdfium-deps-roller@chromium.org>
Date: Mon Feb 27 23:34:37 2017

Roll src/third_party/pdfium/ 9162ff85c..ce32acfa1 (4 commits).

https://pdfium.googlesource.com/pdfium.git/+log/9162ff85c323..ce32acfa1507

$ git log 9162ff85c..ce32acfa1 --date=short --no-merges --format='%ad %ae %s'
2017-02-27 npm Spacing nit in libtiff patch
2017-02-27 npm LCMS upstream patch to fix integer overflows
2017-02-27 npm Add public API for creating a Type1 font
2017-02-27 tsepez Explicitly tag fxjs native objects.

Created with:
  roll-dep src/third_party/pdfium
BUG= 696430 , 695830 

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

TBR=dsinclair@chromium.org

Review-Url: https://codereview.chromium.org/2718223002
Cr-Commit-Position: refs/heads/master@{#453390}

[modify] https://crrev.com/11e15a648963d343125c2bb95369f4bfa178ea69/DEPS

right - in chromium, we look at the memory field zero points at and require it to have a certain magic signature https://cs.chromium.org/chromium/src/gin/wrapper_info.cc?rcl=70079b5955de2a047669c39938839851fb107b44&l=9

jochen - won't GINs call to GetAlingedPointerFromInternalField() trip over the same release assert if it is handed an unrelated object that happens to have the right field count and the expected field isn't an SMI in this object?

hum, there shouldn't be objects that have non-SMIs in the first internal field. Also the i18n and ArrayBuffer instances all put aligned pointers there

tsepez@ - are you expecting to make any more changes here, or can this be marked as fixed? Thanks!

This can still happen I think, and I'm not sure what to do about it.  It isn't really a security issue anymore, just a crash.

why not set a private property when constructing a fx_jsobject, so you can you use this private property as a tag to judge if an object is a fx_jsobject. Private property is invisible to user JavaScript, so it's safe, user javascript can't modify it's value. 
you can set a private property simply by using v8 api
  Maybe<bool> SetPrivate(Local<Context> context, Local<Private> key,
                         Local<Value> value);


just an advice:-)

Hello, will you fix the left issue?

I'll mark this as fixed; we may see a non-security issue crash which ought to be tracked separately if it occurs.

Could you help to assign a CVE to this issue? 
thanks

awhalley: ^

Thanks for the ping. Since this wasn't marked as fixed when M58 went out it didn't get considered for a CVE then. Would it be OK to assign one when M60 goes stable (about 5-6 weeks) or do you need one sooner? 

Take your time, it's ok to assign one when M60 goes stable.

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