New issue
Advanced search Search tips

Issue 813155 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: May 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac
Pri: 3
Type: Bug-Security

Blocking:
issue 62400



Sign in to add a comment

Heap-use-after-free in fxcrt::UnownedPtr<CFX_XMLNode>::ProbeForLowSeverityLifetimeIssue

Project Member Reported by ClusterFuzz, Feb 16 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=6462963138166784

Fuzzer: libFuzzer_pdfium_xfa_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Heap-use-after-free READ 1
Crash Address: 0x607000010160
Crash State:
  fxcrt::UnownedPtr<CFX_XMLNode>::ProbeForLowSeverityLifetimeIssue
  fxcrt::UnownedPtr<CFX_XMLNode>::operator=
  fxcrt::MaybeOwned<CFX_XMLNode, std::__1::default_delete<CFX_XMLNode> >::Reset
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=537116:537122

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6462963138166784

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.
 
Project Member

Comment 1 by ClusterFuzz, Feb 16 2018

Components: Internals>Plugins>PDF
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 2 by ClusterFuzz, Feb 16 2018

Labels: Test-Predator-Auto-Owner
Owner: dsinclair@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://pdfium.googlesource.com/pdfium/+/e40678ed8a22ecd57421877af39cf7f281f618c4 (Make the CFX_XMLNode a MaybeOwned pointer).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Labels: -Security_Impact-Head -Security_Severity-High Security_Impact-None Security_Severity-Low Pri-3
Blocking: 62400
Project Member

Comment 5 by ClusterFuzz, Mar 22 2018

Labels: OS-Mac
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 9 2018

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/a0706f0c904673ea9f679829ff87e730e5800765

commit a0706f0c904673ea9f679829ff87e730e5800765
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Mon Apr 09 16:06:24 2018

Move code to set XML value to CXFA_Node

This CL moves the code to set the XML node data from CJX_Node to
CXFA_Node. The XML node is owned by the XFA node, not the CJX node so it
makes more sense to have the modifications happen there.

This combines the duplicate code in CJX_Node into a single SetToXML
method.

Bug:  chromium:813155 
Change-Id: I493725d1412688cb1a0d04bd9ae9fa5a36caebf3
Reviewed-on: https://pdfium-review.googlesource.com/29858
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/a0706f0c904673ea9f679829ff87e730e5800765/fxjs/xfa/cjx_object.cpp
[modify] https://crrev.com/a0706f0c904673ea9f679829ff87e730e5800765/xfa/fxfa/parser/cxfa_node.h
[modify] https://crrev.com/a0706f0c904673ea9f679829ff87e730e5800765/xfa/fxfa/parser/cxfa_node.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 9 2018

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/2b4ad9ff5bf1727d12ef2bcce7e3fa925bbe8978

commit 2b4ad9ff5bf1727d12ef2bcce7e3fa925bbe8978
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Mon Apr 09 16:14:55 2018

Remove CXFA nodes instead of CFX_XML nodes

When we set data into the CJX nodes, we need to update the backing store
of the XFA nodes, which are the XML nodes. When we update the backing
store, there are cases where we need to remove the children (setting
CData on a node or setting the value of an Attribute which is backed by
a real node).

Currently we call DeleteChildren on the XML node. This causes issues
when we cleanup the tree as XFA nodes may point to these deleted XML
nodes.

Instead, this CL cleans up the XFA nodes which will in turn clean up the
XML nodes. This seems like the right thing to do as we no longer have
the backing nodes, so we should no longer have the XFA nodes on top of
them.

Bug:  chromium:813155 
Change-Id: I63d53fd56999ec9d3b3af2ab5952b374663f040c
Reviewed-on: https://pdfium-review.googlesource.com/29950
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/2b4ad9ff5bf1727d12ef2bcce7e3fa925bbe8978/xfa/fxfa/parser/cxfa_node.cpp

Status: Fixed (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 9 2018

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/eb9502ffdfb27e61ea587cbfbb03092ded2b5630

commit eb9502ffdfb27e61ea587cbfbb03092ded2b5630
Author: dsinclair <dsinclair@chromium.org>
Date: Mon Apr 09 17:56:15 2018

Revert "Remove CXFA nodes instead of CFX_XML nodes"

This reverts commit 2b4ad9ff5bf1727d12ef2bcce7e3fa925bbe8978.

Reason for revert: Causing crashes on windows:

Rendering PDF file E:\b\build\slave\windows_xfa\build\pdfium\out\debug_xfa_v8\gen\pdfium\testing\pixel\static_list_box_caption.pdf.

==== C stack trace ===============================

diff: 0.00% passed
	std::tuple<CXFA_Node * __ptr64 const & __ptr64>::tuple<CXFA_Node * __ptr64 const & __ptr64> [0x0000000141785700+48]
	??@875fbed02c72fe689afc0ad24a247bf8@ [0x000000013F7BB443+131]
	??$_Buynode@AEBUpiecewise_construct_t@std@@V?$tuple@AEBQEAVCXFA_Node@@@2@V?$tuple@$$V@2@@?$_Tree_comp_alloc@V?$_Tmap_traits@PEAVCXFA_Node@@V?$unique_ptr@V?$map@IV?$unique_ptr@U?$pair@V?$set@PEAVCXFA_Node@@U?$less@PEAVCXFA_Node@@@std@@V?$allocator@PEAVCXFA [0x0000000141785309+169]
	??@2b14910fe4cc3866ca68e10bfed05253@ [0x000000013F7CCB09+185]
	CJX_Object::SetAttributeValue [0x000000013FAE0F7E+302]
	v8::internal::compiler::Int64Lowering::LowerNode [0x000000013FBB04C0+31920]
	CXFA_Document::DataMerge_CopyContainer [0x00000001417A43A3+9603]
	CXFA_Document::DataMerge_CopyContainer [0x00000001417A2048+552]
	CXFA_Document::DataMerge_CopyContainer [0x00000001417A3436+5654]
	CXFA_Document::DataMerge_CopyContainer [0x00000001417A1FEE+462]
	CXFA_Document::DoDataMerge [0x00000001417A5F96+3142]
	CXFA_FFDocView::StartLayout [0x00000001416BECCD+93]
	CPDFXFA_Context::LoadXFADoc [0x000000014169162D+669]
	FPDF_LoadXFA [0x0000000140AC5A2A+42]
	std::_Compressed_pair<std::default_delete<CFWL_WidgetProperties>,CFWL_WidgetProperties * __ptr64,1>::_Get_first [0x000000013F72A648+1432]
	main [0x000000013F727C05+1685]
	invoke_main [0x0000000141CDB924+52] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:79)
	__scrt_common_main_seh [0x0000000141CDB817+295] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
	__scrt_common_main [0x0000000141CDB6DE+14] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:326)
	mainCRTStartup [0x0000000141CDB9B9+9] (f:\dd\vctools\crt\vcstartup\src\startup\exe_main.cpp:17)
	BaseThreadInitThunk [0x0000000077415A4D+13]
	RtlUserThreadStart [0x000000007764B831+33]
Rendering PDF file E:\b\build\slave\windows_xfa\build\pdfium\out\debug_xfa_v8\gen\pdfium\testing\pixel\resolve_nodes.pdf.

==== C stack trace ===============================

	std::tuple<CXFA_Node * __ptr64 const & __ptr64>::tuple<CXFA_Node * __ptr64 const & __ptr64> [0x0000000141785700+48]
	??@875fbed02c72fe689afc0ad24a247bf8@ [0x000000013F7BB443+131]
	??$_Buynode@AEBUpiecewise_construct_t@std@@V?$tuple@AEBQEAVCXFA_Node@@@2@V?$tuple@$$V@2@@?$_Tree_comp_alloc@V?$_Tmap_traits@PEAVCXFA_Node@@V?$unique_ptr@V?$map@IV?$unique_ptr@U?$pair@V?$set@PEAVCXFA_Node@@U?$less@PEAVCXFA_Node@@@std@@V?$allocator@PEAVCXFA [0x0000000141785309+169]
	??@2b14910fe4cc3866ca68e10bfed05253@ [0x000000013F7CCB09+185]
	CJX_Object::SetAttributeValue [0x000000013FAE0F7E+302]
	v8::internal::compiler::Int64Lowering::LowerNode [0x000000013FBB03F6+31718]
	CXFA_Document::DataMerge_UpdateBindingRelations [0x00000001417A48DA+1066]
	CXFA_Document::DataMerge_UpdateBindingRelations [0x00000001417A4ED8+2600]
	CXFA_Document::DataMerge_UpdateBindingRelations [0x00000001417A4ED8+2600]
	CXFA_Document::DataMerge_UpdateBindingRelations [0x00000001417A4ED8+2600]
	CXFA_Document::DataMerge_UpdateBindingRelations [0x00000001417A4ED8+2600]
	CXFA_Document::DataMerge_UpdateBindingRelations [0x00000001417A4512+98]
	CXFA_Document::DoDataMerge [0x00000001417A5FF8+3240]
	CXFA_FFDocView::StartLayout [0x00000001416BECCD+93]
	CPDFXFA_Context::LoadXFADoc [0x000000014169162D+669]
	FPDF_LoadXFA [0x0000000140AC5A2A+42]
	std::_Compressed_pair<std::default_delete<CFWL_WidgetProperties>,CFWL_WidgetProperties * __ptr64,1>::_Get_first [0x000000013F72A648+1432]
	main [0x000000013F727C05+1685]
	invoke_main [0x0000000141CDB924+52] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:79)
	__scrt_common_main_seh [0x0000000141CDB817+295] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
	__scrt_common_main [0x0000000141CDB6DE+14] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:326)
	mainCRTStartup [0x0000000141CDB9B9+9] (f:\dd\vctools\crt\vcstartup\src\startup\exe_main.cpp:17)
	BaseThreadInitThunk [0x0000000077415A4D+13]
	RtlUserThreadStart [0x000000007764B831+33]
FAILURE: static_list_box_caption.pdf; Command '['E:\\b\\build\\slave\\windows_xfa\\build\\pdfium\\out\\debug_xfa_v8\\pdfium_test.exe', '--send-events', '--png', '--md5', 'E:\\b\\build\\slave\\windows_xfa\\build\\pdfium\\out\\debug_xfa_v8\\gen\\pdfium\\testing\\pixel\\static_list_box_caption.pdf']' returned non-zero exit status -1073741819
Rendering PDF file E:\b\build\slave\windows_xfa\build\pdfium\out\debug_xfa_v8\gen\pdfium\testing\pixel\standard_symbols.pdf.

==== C stack trace ===============================

	std::tuple<CXFA_Node * __ptr64 const & __ptr64>::tuple<CXFA_Node * __ptr64 const & __ptr64> [0x0000000141785700+48]
FAILURE: resolve_nodes.pdf; Command '['E:\\b\\build\\slave\\windows_xfa\\build\\pdfium\\out\\debug_xfa_v8\\pdfium_test.exe', '--send-events', '--png', '--md5', 'E:\\b\\build\\slave\\windows_xfa\\build\\pdfium\\out\\debug_xfa_v8\\gen\\pdfium\\testing\\pixel\\resolve_nodes.pdf']' returned non-zero exit status -1073741819
	??@875fbed02c72fe689afc0ad24a247bf8@ [0x000000013F7BB443+131]
	??$_Buynode@AEBUpiecewise_construct_t@std@@V?$tuple@AEBQEAVCXFA_Node@@@2@V?$tuple@$$V@2@@?$_Tree_comp_alloc@V?$_Tmap_traits@PEAVCXFA_Node@@V?$unique_ptr@V?$map@IV?$unique_ptr@U?$pair@V?$set@PEAVCXFA_Node@@U?$less@PEAVCXFA_Node@@@std@@V?$allocator@PEAVCXFA [0x0000000141785309+169]
	??@2b14910fe4cc3866ca68e10bfed05253@ [0x000000013F7CCB09+185]
	CJX_Object::SetAttributeValue [0x000000013FAE0F7E+302]
	v8::internal::compiler::Int64Lowering::LowerNode [0x000000013FBB04C0+31920]
	CXFA_Document::DataMerge_CopyContainer [0x00000001417A43A3+9603]
	CXFA_Document::DataMerge_CopyContainer [0x00000001417A2048+552]
	CXFA_Document::DataMerge_CopyContainer [0x00000001417A3436+5654]
	CXFA_Document::DataMerge_CopyContainer [0x00000001417A1FEE+462]
	CXFA_Document::DoDataMerge [0x00000001417A5F96+3142]
	CXFA_FFDocView::StartLayout [0x00000001416BECCD+93]
	CPDFXFA_Context::LoadXFADoc [0x000000014169162D+669]
	FPDF_LoadXFA [0x0000000140AC5A2A+42]
	std::_Compressed_pair<std::default_delete<CFWL_WidgetProperties>,CFWL_WidgetProperties * __ptr64,1>::_Get_first [0x000000013F72A648+1432]
	main [0x000000013F727C05+1685]
	invoke_main [0x0000000141CDB924+52] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:79)
	__scrt_common_main_seh [0x0000000141CDB817+295] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
	__scrt_common_main [0x0000000141CDB6DE+14] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:326)
	mainCRTStartup [0x0000000141CDB9B9+9] (f:\dd\vctools\crt\vcstartup\src\startup\exe_main.cpp:17)
	BaseThreadInitThunk [0x0000000077415A4D+13]
	RtlUserThreadStart [0x000000007764B831+33]
FAILURE: standard_symbols.pdf; Command '['E:\\b\\build\\slave\\windows_xfa\\build\\pdfium\\out\\debug_xfa_v8\\pdfium_test.exe', '--send-events', '--png', '--md5', 'E:\\b\\build\\slave\\windows_xfa\\build\\pdfium\\out\\debug_xfa_v8\\gen\\pdfium\\testing\\pixel\\standard_symbols.pdf']' returned non-zero exit status -1073741819

Original change's description:
> Remove CXFA nodes instead of CFX_XML nodes
> 
> When we set data into the CJX nodes, we need to update the backing store
> of the XFA nodes, which are the XML nodes. When we update the backing
> store, there are cases where we need to remove the children (setting
> CData on a node or setting the value of an Attribute which is backed by
> a real node).
> 
> Currently we call DeleteChildren on the XML node. This causes issues
> when we cleanup the tree as XFA nodes may point to these deleted XML
> nodes.
> 
> Instead, this CL cleans up the XFA nodes which will in turn clean up the
> XML nodes. This seems like the right thing to do as we no longer have
> the backing nodes, so we should no longer have the XFA nodes on top of
> them.
> 
> Bug:  chromium:813155 
> Change-Id: I63d53fd56999ec9d3b3af2ab5952b374663f040c
> Reviewed-on: https://pdfium-review.googlesource.com/29950
> Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
> Commit-Queue: dsinclair <dsinclair@chromium.org>

TBR=thestig@chromium.org,dsinclair@chromium.org,hnakashima@chromium.org

Change-Id: Ic6b2fbeb9f8217292da0a722e143d96f9972056a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:813155 
Reviewed-on: https://pdfium-review.googlesource.com/29974
Reviewed-by: dsinclair <dsinclair@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/eb9502ffdfb27e61ea587cbfbb03092ded2b5630/xfa/fxfa/parser/cxfa_node.cpp

Status: Started (was: Fixed)
Fix bounced ....
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 9 2018

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

commit f0997abfb303af95065493b9e859207d625c2b5c
Author: pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Mon Apr 09 21:57:22 2018

Roll src/third_party/pdfium/ 6058efdbd..d45f99809 (8 commits)

https://pdfium.googlesource.com/pdfium.git/+log/6058efdbdc18..d45f9980995a

$ git log 6058efdbd..d45f99809 --date=short --no-merges --format='%ad %ae %s'
2018-04-09 tsepez Make StringViewTemplate be based on pdfium::span<>.
2018-04-09 dsinclair Revert "Remove CXFA nodes instead of CFX_XML nodes"
2018-04-09 tsepez Make pdfium::span<> be based off of UnownedPtr<>.
2018-04-09 thestig Sort test cases in test_runner.py.
2018-04-09 dsinclair Move the CFX_XMLParser out of CXFA_SimpleParser
2018-04-09 hnakashima Remove m_pOldFocusWidget from CXFA_FFDocView.
2018-04-09 dsinclair Remove CXFA nodes instead of CFX_XML nodes
2018-04-09 dsinclair Move code to set XML value to CXFA_Node

Created with:
  roll-dep src/third_party/pdfium
BUG= chromium:813155 , chromium:813155 , chromium:813155 


The AutoRoll server is located here: https://pdfium-roll.skia.org

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

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=dsinclair@chromium.org

Change-Id: Iabd3650fc069fbbb35f9b4daa58dd8801abbd3c6
Reviewed-on: https://chromium-review.googlesource.com/1003097
Reviewed-by: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#549292}
[modify] https://crrev.com/f0997abfb303af95065493b9e859207d625c2b5c/DEPS

Project Member

Comment 12 by ClusterFuzz, May 3 2018

ClusterFuzz has detected this issue as fixed in range 555504:555559.

Detailed report: https://clusterfuzz.com/testcase?key=6462963138166784

Fuzzer: libFuzzer_pdfium_xfa_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Heap-use-after-free READ 1
Crash Address: 0x607000010160
Crash State:
  fxcrt::UnownedPtr<CFX_XMLNode>::ProbeForLowSeverityLifetimeIssue
  fxcrt::UnownedPtr<CFX_XMLNode>::operator=
  fxcrt::MaybeOwned<CFX_XMLNode, std::__1::default_delete<CFX_XMLNode> >::Reset
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=537116:537122
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=555504:555559

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6462963138166784

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 13 by ClusterFuzz, May 3 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase 6462963138166784 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 14 by sheriffbot@chromium.org, May 3 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 15 by sheriffbot@chromium.org, Aug 9

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