New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: May 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security



Sign in to add a comment
Security: release assert trigger in pdfium
Reported by higonggu...@gmail.com, Feb 24 2017 Back to list
VULNERABILITY DETAILS
embed the following simple JavaScript into a PDF
app.clearTimeOut(this);
open it in Chrome, you'll see the crashed pdfviewer plugin

void app::ClearTimerCommon(CJS_Runtime* pRuntime, const CJS_Value& param) {
  if (param.GetType() != CJS_Value::VT_object)
    return;

  v8::Local<v8::Object> pObj = param.ToV8Object(pRuntime);
  if (CFXJS_Engine::GetObjDefnID(pObj) != CJS_TimerObj::g_nObjDefnID)  -----> this line will read the internal field 0 of pObj directly without validation, if  the object have the wrong internal field, it's trigger a release assert, if we can find an object whose internal field point to a small integer, the following code will trigger a type confusion.
    return;

  CJS_Object* pJSObj = param.ToCJSObject(pRuntime);
  if (!pJSObj)
    return;

  TimerObj* pTimerObj = static_cast<TimerObj*>(pJSObj->GetEmbedObject());
  if (!pTimerObj)
    return;

  GlobalTimer::Cancel(pTimerObj->GetTimerID());
}
the crash stack is as follows:

#
# Fatal error in v8::Object::GetAlignedPointerFromInternalField()
# Not a Smi
#


Program received signal SIGILL, Illegal instruction.
v8::base::OS::Abort () at ../../v8/src/base/platform/platform-posix.cc:230
230	    V8_IMMEDIATE_CRASH();
(gdb) bt
#0  v8::base::OS::Abort () at ../../v8/src/base/platform/platform-posix.cc:230
#1  0x00007ffff6ef97f1 in ApiCheck (location=<optimized out>, message=<optimized out>, condition=<optimized out>) at ../../v8/src/api.h:120
#2  DecodeSmiToAligned (value=<optimized out>, location=<optimized out>) at ../../v8/src/api.cc:1016
#3  v8::Object::SlowGetAlignedPointerFromInternalField (this=<optimized out>, index=<optimized out>) at ../../v8/src/api.cc:5929
#4  0x000000000083e932 in GetAlignedPointerFromInternalField (this=0x7fffffffc988, index=0) at ../../v8/include/v8.h:8966
#5  CFXJS_Engine::GetObjDefnID (pObj=...) at ../../third_party/pdfium/fxjs/fxjs_v8.cpp:241
#6  0x000000000082ba7d in app::ClearTimerCommon (this=0xc3da70, pRuntime=0xbc9930, param=...) at ../../third_party/pdfium/fpdfsdk/javascript/app.cpp:558
#7  0x000000000082ba15 in app::clearTimeOut (this=0xc3da70, cc=0xbde140, params=std::__debug::vector of length 1, capacity 1 = {...}, vRet=..., sError=...)
    at ../../third_party/pdfium/fpdfsdk/javascript/app.cpp:536
#8  0x0000000000833956 in JSMethod<app, &app::clearTimeOut> (method_name_string=0x91cd62 "clearTimeOut", class_name_string=0x91ccd1 "app", info=...)
    at ../../third_party/pdfium/fpdfsdk/javascript/JS_Define.h:153
#9  0x000000000082dbd6 in CJS_App::clearTimeOut_static (info=...) at ../../third_party/pdfium/fpdfsdk/javascript/app.h:188
#10 0x00007ffff6ec8d68 in v8::internal::FunctionCallbackArguments::Call (this=<optimized out>, f=<optimized out>) at ../../v8/src/api-arguments.cc:19
#11 0x00007ffff6f9d071 in v8::internal::(anonymous namespace)::HandleApiCallHelper<false> (isolate=<optimized out>, function=..., new_target=..., fun_data=..., 
    receiver=..., args=...) at ../../v8/src/builtins/builtins-api.cc:106
#12 0x00007ffff6f9b9f7 in v8::internal::Builtin_Impl_HandleApiCall (args=..., isolate=<optimized out>) at ../../v8/src/builtins/builtins-api.cc:135
#13 0x00007ffff6f9b519 in v8::internal::Builtin_HandleApiCall (args_length=<optimized out>, args_object=<optimized out>, isolate=<optimized out>)
    at ../../v8/src/builtins/builtins-api.cc:123
a poc is attached as poc2.pdf

VERSION
Chrome Version: [56.0.2924.87] + [stable]
Operating System: [any]

 
poc2.pdf
927 bytes Download
Components: Internals>Plugins>PDF
Owner: jochen@chromium.org
Status: Assigned
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?
Comment 2 by tsepez@chromium.org, Feb 24 2017
Cc: thestig@chromium.org dsinclair@chromium.org jochen@chromium.org
Owner: tsepez@chromium.org
Comment 3 by tsepez@chromium.org, Feb 25 2017
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.

Comment 4 by tsepez@chromium.org, Feb 25 2017
Labels: Security_Severity-Low Security_Impact-Stable
Project Member Comment 5 by sheriffbot@chromium.org, Feb 25 2017
Labels: Pri-2
Labels: OS-All
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]

Comment 9 by tsepez@chromium.org, Feb 27 2017
Fair enough, we should use something with more entropy than just integers starting at 0 to identify these.
Cc: tsepez@chromium.org
Owner: jochen@chromium.org
https://pdfium-review.googlesource.com/2847 landed, jochen to review.
Labels: M-56 M-57 M-58
Project Member Comment 12 by bugdroid1@chromium.org, Feb 27 2017
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

Owner: tsepez@chromium.org
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
Project Member Comment 16 by sheriffbot@chromium.org, Mar 10 2017
Labels: -M-56
Labels: -M-57
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?
Status: Fixed
I'll mark this as fixed; we may see a non-security issue crash which ought to be tracked separately if it occurs.
Project Member Comment 23 by sheriffbot@chromium.org, May 12
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Could you help to assign a CVE to this issue? 
thanks
Cc: awhalley@chromium.org
awhalley: ^
Labels: -M-58 M-60
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.
Labels: Release-0-M60
Labels: CVE-2017-5108
Project Member Comment 30 by sheriffbot@chromium.org, Aug 18
Labels: -Restrict-View-SecurityNotify allpublic
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