New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 738947 link

Starred by 2 users

Issue metadata

Status: Duplicate
Merged: issue 768151
Owner:
Closed: Oct 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

Stack-overflow in xmlParseElement

Project Member Reported by ClusterFuzz, Jul 3 2017

Issue description

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

Fuzzer: libFuzzer_libxml_xml_read_memory_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7ffde08b7f78
Crash State:
  xmlParseElement
  xmlParseContent
  
Sanitizer: address (ASAN)

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


Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
 
Cc: msrchandra@chromium.org
Labels: M-60 Test-Predator-Wrong
Owner: dominicc@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL did not provide any possible suspects.
Assigning to the concern owner who might be related to your changes.

@dominicc -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.
Labels: -Pri-1 OS-Android OS-Chrome OS-Fuchsia OS-Mac OS-Windows Pri-3
Labels: ClusterFuzz-Ignore
Mergedinto: 768151
Owner: joelhockey@chromium.org
Status: Duplicate (was: Assigned)
Reproduced locally.  Program runs OK with 64MB stack.  Direct duplicate of  issue 768151  again.

Comment 4 by mmoroz@google.com, Oct 11 2017

Cc: och...@chromium.org mmoroz@chromium.org mbarbe...@chromium.org kcc@chromium.org metzman@chromium.org
Joel, we need to find a way to prevent the fuzzer from crashing with these cases.

I've seen your suggestion to call setrlimit (https://bugs.chromium.org/p/chromium/issues/detail?id=768151#c3). That might work, though I'm not sure whether it's expected to work well with ASan and other memory tools. CCing folks who might know better.

Is there any other way to prevent stack overflows? Does libxml have an option to configure maximum depth?
Cc: noel@chromium.org
I wouldn't have thought that it is a valid goal to want to stop fuzzer from crashing.  If you increase stack size, wont fuzzer just generate a longer input that will cause a crash again?

My understanding is that we just want to identify which types of crashes should be ignored, and tell fuzzer to stop creating inputs that cause those types of crashes.
As for as using setrlimit to increase stack size, this is something I did locally to understand the crash better.  I don't think it is a good idea to modify the code to include setrlimit.

I wanted to know whether the crash was some sort of real error with an infinite loop that will overflow the stack regardless of size, or whether it was a lesser concern where the given input just required a larger stack.

I was expecting that I should be able to use some compile/link flags to be able to set the stack size during compilation, but I couldn't find anything that worked on my linux workstation.  The runtime call to setrlimit did work for me though.

Comment 7 by mmoroz@chromium.org, Oct 12 2017

>> If you increase stack size, wont fuzzer just generate a longer input that will cause a crash again?

Yes, most likely it would happen at some point.


>> My understanding is that we just want to identify which types of crashes should be ignored, and tell fuzzer to stop creating inputs that cause those types of crashes.

It would be awesome, but we cannot just tell fuzzer to stop creating inputs that cause some type of crashes. We have to do something in order to prevent generation of such inputs. To give you an example, sqlite goes crazy if you try to fuzz it and inputs contains REGEXP keyword. With that, we had to completely ignore such inputs: https://cs.chromium.org/chromium/src/third_party/sqlite/fuzz/sqlite3_prepare_v2_fuzzer.cc?l=41


Comment 8 by mmoroz@chromium.org, Oct 12 2017

>> I wanted to know whether the crash was some sort of real error with an infinite loop that will overflow the stack regardless of size, or whether it was a lesser concern where the given input just required a larger stack.

That's a good question. I would say that either way it's a bug if target program crashes due to stack overflow that can be triggered remotely. Due to the nature of XML, issues like stack-overflow should be expected, and there should be some way to control the maximum depth.

Another example (sqlite again): there are lots of ways to trigger out of memory, but there are also some ways to configure the library in a properly before building and shipping it:
https://cs.chromium.org/chromium/src/third_party/sqlite/BUILD.gn?type=cs&q=sqlite3_prepare_v2_fuzzer.cc&l=135

Comment 9 by noel@chromium.org, Oct 12 2017

The bug is a deeply nested XML document can DOS a renderer (just load any of the test cases in this issues and duped issues in chrome).

  "and there should be some way to control the maximum depth."

We're not libxml experts, but a quick scan of the code [1]

[1] https://cs.chromium.org/chromium/src/third_party/libxml/src/parser.c?rcl=2ca6cb5eba12474c29cd80ef46c6f0ec5d2c1a29&l=1795

    if ((((unsigned int) ctxt->nodeNr) > xmlParserMaxDepth) &&
        ((ctxt->options & XML_PARSE_HUGE) == 0)) {
	xmlFatalErrMsgInt(ctxt, XML_ERR_INTERNAL_ERROR,
		 "Excessive depth in document: %d use XML_PARSE_HUGE option\n",
			  xmlParserMaxDepth);
	xmlHaltParser(ctxt);
	return(-1);
    }

So provided the XML_PARSE_HUGE options is off, the global xmlParserMaxDepth can be used set a deep limit.

The default value is 256, which seems about right to me.

We could try lowering it for fuzz tests I suppose, but then it would not match the chrome renderer *sigh*.
Thanks for investigating, Noel!

Joel, may I ask you to test that suggestion, since you've taken the heavy duty of fixing XML fuzzers? :)

I have created crrev.com/c/720537 to always disable XML_PARSE_HUGE for the fuzzer.  It appears that chrome always sets XML_PARSE_HUGE on so that it has no size restrictions.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp?l=692&rcl=f614725ae66fe23c3e485b1d2bb29cf8e690cfa1

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 16 2017

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

commit 5bfbfdcca78ad9e307078923b8a31cb9ff8c4971
Author: Joel Hockey <joelhockey@chromium.org>
Date: Mon Oct 16 14:48:01 2017

Always disable XML_PARSE_HUGE option for libxml fuzzing.

Large inputs cause fuzz crashes with stack overflow.  These crashes are
not useful and best to be avoided.  By leaving XML_PARSE_HUGE option
unset, libxml will detect and refuse very large inputs.

Bug:  738947 
Change-Id: Ic7ff5ae3b7ad4a55356549c116d0a1e03e24ea34
Reviewed-on: https://chromium-review.googlesource.com/720537
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509043}
[modify] https://crrev.com/5bfbfdcca78ad9e307078923b8a31cb9ff8c4971/testing/libfuzzer/fuzzers/libxml_xml_read_memory_fuzzer.cc

Project Member

Comment 13 by ClusterFuzz, Oct 17 2017

ClusterFuzz has detected this issue as fixed in range 509033:509059.

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

Fuzzer: libFuzzer_libxml_xml_read_memory_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Stack-overflow
Crash Address: 0x7ffd68213ff8
Crash State:
  NULL
Sanitizer: address (ASAN)

Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=509033:509059

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

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

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
That works, you guys rock!

Comment 15 by noel@chromium.org, Oct 18 2017

Thanks.  Note we loose some coverage doing it this way ...

Sign in to add a comment