New issue
Advanced search Search tips

Issue 754610 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Failure to check the return result of NewFxDynamicObj

Reported by sljro...@gmail.com, Aug 11 2017

Issue description

According to this previous bug (https://pdfium-review.googlesource.com/c/2839), the NewFxDynamicObject() call in C++ does not necessarily properly initialize the internal fields of the corresponding constructor. In several files, NewFxDynamicObject() is called but the return result is not checked with a call to IsEmpty().
Here is the list of the files where the lack of IsEmpty() call might introduce an issue:

https://pdfium.googlesource.com/pdfium/+/refs/changes/39/2839/2/fpdfsdk/javascript/global.cpp#212
https://pdfium.googlesource.com/pdfium/+/master/fpdfsdk/javascript/global.cpp#337
https://pdfium.googlesource.com/pdfium/+/master/fxjs/fxjs_v8.cpp#411
https://pdfium.googlesource.com/pdfium/+/master/fxjs/fxjs_v8.cpp#481
https://pdfium.googlesource.com/pdfium/+/master/fxjs/fxjs_v8_embeddertest.cpp#194
 

Comment 1 by tsepez@chromium.org, Aug 11 2017

Owner: tsepez@chromium.org
Status: Assigned (was: Unconfirmed)
Thanks.  The calls with -1 as the ID should be fine, as there aren't any internal fields created in that case.  I'll double-check the other ones.

Comment 2 by tsepez@chromium.org, Aug 11 2017

Components: Internals>Plugins>PDF
Labels: Security_Severity-High Security_Impact-Stable OS-All
Assigning sev high out of an abundance of caution, though I don't think there's an issue here.
Project Member

Comment 3 by sheriffbot@chromium.org, Aug 12 2017

Labels: M-60
Project Member

Comment 4 by sheriffbot@chromium.org, Aug 12 2017

Labels: Pri-1

Comment 5 by tsepez@chromium.org, Aug 14 2017

Labels: -Type-Bug-Security -Pri-1 -Security_Impact-Stable -Security_Severity-High -M-60 M-62 Pri-3 Type-Bug
Here were talking about getting a NULL object (whereas https://pdfium-review.googlesource.com/c/2839 requires a non-NULL object with internal slots), and it looks like the worst that can happen is a fixed-offset NULL deref.

Such crashes are functional bugs, not security issues.  Still we should catch these, although I've not seen a lot of crash stats implicating these.

Comment 7 by tsepez@chromium.org, Aug 14 2017

Labels: -Restrict-View-SecurityTeam

Comment 8 by tsepez@chromium.org, Aug 14 2017

Summary: Failure to check the return result of NewFxDynamicObj (was: Security: Potential failing to check the return result of NewFxDynamicObjec)

Comment 9 by sljro...@gmail.com, Aug 15 2017

Thanks for taking care of this.

Sign in to add a comment