750932 - ASAN: Test test_multipart_streamconv_missing_lead_boundary.js triggers error

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christian Holler (:decoder)]

Attachment: 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.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Josh, any thoughts on who could look at this one?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

cc+ Brian Smith. Does this look like a networking bug?

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[Christian Holler (:decoder)]

I'm still seeing this on today's try push, causing orange on xpcshell tests.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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?

Comment 6

[Josh Aas]

Over to Patrick to take a look.

Comment 7

[Christian Holler (:decoder)]

This is still showing up daily on TBPL and I can reproduce locally.

Comment 8

[Christian Holler (:decoder)]

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).

Comment 9

[Christian Holler (:decoder)]

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.

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: multipart delimiter check

Comment 11

[Patrick McManus [:mcmanus]]

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.

Comment 12

[Christian Holler (:decoder)]

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 :)

Comment 13

[Patrick McManus [:mcmanus]]

thanks christian.. I was just noticing that.. distracted by all the red :)

Comment 14

[Patrick McManus [:mcmanus]]

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 15

[Patrick McManus [:mcmanus]]

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.

Comment 16

[Christian Holler (:decoder)]

(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 17

[Jason Duell]

Comment on attachment 792407 [details] [diff] [review]
multipart delimiter check

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

Thanks for figuring this out Patrick!

Comment 18

[Patrick McManus [:mcmanus]]

Attachment: multipart delimiter check

Comment 19

[Christian Holler (:decoder)]

Comment on attachment 793464 [details] [diff] [review]
multipart delimiter check

Thanks!

Comment 20

[Patrick McManus [:mcmanus]]

this is sec-moderate so I just pushed it without sec-approval

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8ae314d872

Comment 21

[Patrick McManus [:mcmanus]]

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

Comment 22

[bhavana bajaj [:bajaj]]

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.

Comment 23

[Patrick McManus [:mcmanus]]

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]

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage on all platforms.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8ae314d872

https://tbpl.mozilla.org/php/getParsedLog.php?id=26822633&tree=Mozilla-Inbound

Comment 25

[Patrick McManus [:mcmanus]]

asan retry: https://tbpl.mozilla.org/?tree=Try&rev=7dee5d7f5e8c
desktop retry https://tbpl.mozilla.org/?tree=Try&rev=2290a97bf799

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a10f2f0401a

Comment 26

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b7986713641d
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/f935e4a1c6dd

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a10f2f0401a

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1f6e89d119a9

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/1f6e89d119a9