Closed Bug 750932 Opened 8 years ago Closed 6 years ago

ASAN: Test test_multipart_streamconv_missing_lead_boundary.js triggers error

Categories

(Core :: Networking, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox23 --- affected
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: decoder, Assigned: mcmanus)

References

(Blocks 1 open bug)

Details

(Keywords: sec-moderate, testcase, Whiteboard: [asan][asan-test-failure][adv-main24+])

Attachments

(2 files, 1 obsolete file)

Attached file AddressSanitizer Log
This xpcshell test "test_multipart_streamconv_missing_lead_boundary.js" fails with AddressSanitizer on mozilla-central revision 6e34995a746e. The output of the command

make SOLO_FILE="test_multipart_streamconv_missing_lead_boundary.js" -C netwerk/test/ check-one 2>&1 | asan_symbolize.py | c++filt

is attached, which includes the backtrace of the problem. Not sure which component this belongs to exactly, please move as appropriate :) Marking this s-s until triaged and confirmed to be safe.
Component: General → Networking
QA Contact: general → networking
Josh, any thoughts on who could look at this one?
Whiteboard: [asan] → [asan][asan-test-failure]
cc+ Brian Smith. Does this look like a networking bug?
looks like we're over-reading from a buffer we realloc'd only 35 lines before. worst case maybe leaking other memory into a document? Unless there's some reason to believe we'd have stopped at that one byte instead of keeping going.
Keywords: sec-moderate
Whiteboard: [asan][asan-test-failure] → [asan][asan-test-failure][orange]
I'm still seeing this on today's try push, causing orange on xpcshell tests.
Blocks: 438871
No longer blocks: 438871
Whiteboard: [asan][asan-test-failure][orange] → [asan][asan-test-failure]
I am going to take this so I can investigate it. Christian, if I run this under ASAN should I be expecting a failure every time, or only intermittently?
Assignee: nobody → bsmith
Over to Patrick to take a look.
Assignee: bsmith → mcmanus
Blocks: asan-tests
This is still showing up daily on TBPL and I can reproduce locally.
I took a quick look at this and it seems to me that we're reading out of bounds in the line in question:

>        if (*(token+mTokenLen+1) == '-') {

What guarantees that there is something after token at all? My debugging seems to indicate that we're at the end of the string (token+mTokenLen pointing at the last '-', so token+mTokenLen+1 is OOB if these strings aren't null-terminated).
Flags: needinfo?(mcmanus)
Correction to the last comment (I had modified the test): With the original test, the "--" at the end of the string in question doesn't seem to be even there. We die while parsing this string:

> var multipartBody = "\r\nSome text\r\n--boundary\r\n\r\n<?xml version='1.0'?><root/>\r\n--boundary--";


Breakpoint 1, __asan_report_error (pc=46912521199766, bp=140737488320688, sp=140737488320680, addr=106034152709708, is_write=false, access_size=1) at /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_report.cc:675
675                              uptr addr, bool is_write, uptr access_size) {

(gdb) x /16bx 106034152709708-8
0x607000019e44: 0x62    0x6f    0x75    0x6e    0x64    0x61    0x72    0x79 <- 'boundary'
0x607000019e4c: 0x00    0x00    0x00    0x00    0xb9    0x96    0x6e    0xcc <- no dashes


So the ASan error is perfectly correct, we're reading out of bounds.
Blocks: 905636
No longer blocks: asan-tests
Attached patch multipart delimiter check (obsolete) — Splinter Review
I endeavored to confirm the bug and the fix using a asan try run based on the instructions here: https://developer.mozilla.org/en-US/docs/Building_Firefox_with_Address_Sanitizer

but I failed to get it to build: https://tbpl.mozilla.org/?tree=Try&rev=6dc544ff626a

therefore I'm going to ask :decoder to build the patch for me and verify the fix.
Flags: needinfo?(mcmanus)
Attachment #792407 - Flags: feedback?(choller)
Thanks for looking into this.

Actually, your try run looks good and is still running. The orange on the opt-build can be ignored, it's a buildbot thing. The red on debug is because there seems to be --enable-valgrind in the config while the builder doesn't have valgrind installed. But we don't need that debug build to confirm the bug.

xpcshell is running and should orange because the test is marked as failing and with the patch it should pass. So let's just wait until that is finished :)
thanks christian.. I was just noticing that.. distracted by all the red :)
13:41:09  WARNING -  TEST-UNEXPECTED-PASS | /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_multipart_streamconv_missing_lead_boundary.js | test failed (with xpcshell return code: 0), see following log:

UNEXPECTED-PASS.. that's funny :)
Comment on attachment 792407 [details] [diff] [review]
multipart delimiter check

I had to familiarize myself with this code.

I'm 98% sure the case where the - and the normal boundary string are in different ODA events works ok because it is followed by an OnStopRequest event.. but even if that isn't true that's a pre-existing (non security) bug which isn't regressed by this change.
Attachment #792407 - Flags: feedback?(choller) → review?(jduell.mcbugs)
(In reply to Patrick McManus [:mcmanus] from comment #14)

> UNEXPECTED-PASS.. that's funny :)

Yep :) Looks like your fix is working. If you push it like this to inbound, please also re-enable the test in the same push by removing the AddressSanitizer specific "fail-if" in xpcshell.ini. Thanks!
Comment on attachment 792407 [details] [diff] [review]
multipart delimiter check

Review of attachment 792407 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for figuring this out Patrick!
Attachment #792407 - Flags: review?(jduell.mcbugs) → review+
Attachment #793464 - Flags: review+
Attachment #792407 - Attachment is obsolete: true
Comment on attachment 793464 [details] [diff] [review]
multipart delimiter check

Thanks!
Attachment #793464 - Flags: review+
this is sec-moderate so I just pushed it without sec-approval

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8ae314d872
Comment on attachment 793464 [details] [diff] [review]
multipart delimiter check

[Approval Request Comment]
Bug caused by (feature/regressing bug #): latent original sin
User impact if declined: possible info leak between documents
Testing completed (on m-c, etc.): on m-i, contains an ASAN test
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #793464 - Flags: approval-mozilla-beta?
Attachment #793464 - Flags: approval-mozilla-aurora?
Comment on attachment 793464 [details] [diff] [review]
multipart delimiter check

low risk patch contains an ASAN test and we still have a few more beta's to deal with fallouts if any.Hence approving this for uplift on aurora/beta.
Attachment #793464 - Flags: approval-mozilla-beta?
Attachment #793464 - Flags: approval-mozilla-beta+
Attachment #793464 - Flags: approval-mozilla-aurora?
Attachment #793464 - Flags: approval-mozilla-aurora+
trivial osx -Werror .. will need to reland.

../../../../../netwerk/streamconv/converters/nsMultiMixedConv.cpp:568:43: error: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Werror,-Wsign-compare]
https://hg.mozilla.org/mozilla-central/rev/3a10f2f0401a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Whiteboard: [asan][asan-test-failure] → [asan][asan-test-failure][adv-main24+]
Comment on attachment 793464 [details] [diff] [review]
multipart delimiter check

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): latent original sin
User impact if declined: possible info leak between documents
Testing completed (on m-c, etc.): Shipping on desktop since Fx24. Backporting was trivial and is green on Try.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #793464 - Flags: approval-mozilla-b2g18?
Attachment #793464 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Group: core-security
You need to log in before you can comment on or make changes to this bug.