1464063 - [LibFuzzer] SDP: global-buffer-overflow [@sdp_getchoosetok]

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Christoph Diehl [:posidron]]

The following testcase crashes on mozilla-central revision 20180523-d36cd8bdbc5c

See attachment.

Backtrace:

==35==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffc16cfd230 at pc 0x7f07512f988e bp 0x7ffc16cfcf20 sp 0x7ffc16cfcf18
READ of size 1 at 0x7ffc16cfd230 thread T0
SCARINESS: 27 (1-byte-read-stack-buffer-overflow)
    #0 0x7f07512f988d in sdp_getchoosetok /builds/worker/workspace/build/src/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c:452:15
    #1 0x7f07512f1b4e in sdp_parse_media /builds/worker/workspace/build/src/media/webrtc/signaling/src/sdp/sipcc/sdp_token.c:1165:13
    #2 0x7f07512e8b92 in sdp_parse /builds/worker/workspace/build/src/media/webrtc/signaling/src/sdp/sipcc/sdp_main.c:1138:18
    #3 0x7f0751378a6f in mozilla::SipccSdpParser::Parse(std::string const&) /builds/worker/workspace/build/src/media/webrtc/signaling/src/sdp/SipccSdpParser.cpp:68:25
    #4 0x7f075c50e51c in RunSdpParserFuzzing(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/media/webrtc/signaling/fuzztest/sdp_parser_libfuzz.cpp:28:20
    #5 0x5893fd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #6 0x588c7b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:442:3
    #7 0x58a16d in fuzzer::Fuzzer::MutateAndTestOne() /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:650:19
    #8 0x58ab25 in fuzzer::Fuzzer::Loop(std::vector<std::string, fuzzer::fuzzer_allocator<std::string> > const&) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:773:5
    #9 0x582185 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:754:6
    #10 0x7f075b3f81e5 in mozilla::FuzzerRunner::Run(int*, char***) /builds/worker/workspace/build/src/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #11 0x7f075b337b33 in XREMain::XRE_mainStartup(bool*) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:3916:35
    #12 0x7f075b348f45 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4857:12
    #13 0x7f075b34a6d4 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4964:21
    #14 0x4f4ef5 in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:233:22
    #15 0x4f4ef5 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:306
    #16 0x7f0772b4a1c0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x211c0)
    #17 0x42476c in _start (/home/worker/firefox/firefox+0x42476c)

DEDUP_TOKEN: sdp_getchoosetok
Address 0x7ffc16cfd230 is located in stack of thread T0 at offset 656 in frame
    #0 0x7f07512f130f in sdp_parse_media /builds/worker/workspace/build/src/media/webrtc/signaling/src/sdp/sipcc/sdp_token.c:1107

DEDUP_TOKEN: sdp_parse_media
  This frame has 5 object(s):
    [32, 48) 'num' (line 1110)
    [64, 68) 'result' (line 1112)
    [80, 336) 'tmp' (line 1115)
    [400, 656) 'port' (line 1116) <== Memory access at offset 656 overflows this variable
    [720, 728) 'port_ptr' (line 1117)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /builds/worker/workspace/build/src/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c:452:15 in sdp_getchoosetok
Shadow bytes around the buggy address:
  0x100002d979f0: 00 00 00 00 f1 f1 f1 f1 00 00 f2 f2 04 f2 00 00
  0x100002d97a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100002d97a10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f2 f2
  0x100002d97a20: f2 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00
  0x100002d97a30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x100002d97a40: 00 00 00 00 00 00[f2]f2 f2 f2 f2 f2 f2 f2 00 f3
  0x100002d97a50: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
  0x100002d97a60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100002d97a70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100002d97a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100002d97a90: f1 f1 f1 f1 f8 f3 f3 f3 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb

Command: ./firefox/firefox -print_pcs=1 -handle_segv=0 -handle_bus=0 -handle_abrt=0 ./corpora/ -handle_ill=0 -handle_fpe=0

==35==ABORTING

Comment 1

[Christoph Diehl [:posidron]]

Attachment: Testcase

Comment 2

[Daniel Veditz [:dveditz]]

If this is an off-by-one it's probably sec-moderate. If it reads a lot or an arbitrary amount then sec-high.

Is this in the child process? or does this live in the parent with other networking stuff?

Comment 3

[Nils Ohlmeier [:drno]]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> If this is an off-by-one it's probably sec-moderate. If it reads a lot or an
> arbitrary amount then sec-high.
> 
> Is this in the child process? or does this live in the parent with other
> networking stuff?

This still lives in the child/content process (and is always going to be).

Comment 4

[Nils Ohlmeier [:drno]]

I'm failing to repro this with a plain ASAN build. Need to run this through the fuzzer.

Comment 5

[Christoph Diehl [:posidron]]

HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

we do not use custom stack unwinders or swapcontext

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

testcase is 269 bytes (include m=)
#define SDP_MAX_STRING_LEN      256  /* Max len for SDP string       */
in sdp_token.c: sdp_result_e sdp_parse_media():
    char                  tmp[SDP_MAX_STRING_LEN];
    char                  port[SDP_MAX_STRING_LEN];

This is very suspicious and likely where the issue is.  The exact line being reported may be tweaked due to the wonders of optimizers.

Comment 8

[Nils Ohlmeier [:drno]]

Attachment: bug1464063.patch

Comment 9

[Nils Ohlmeier [:drno]]

So it turns out with the testcase from the fuzzer you can't repro this is a plain ASAN build, because we have a UTF8 filter in PeerConnection.js which rejects this.

But to dump my brain here while it is fresh in my mind: one have to get the parser into parsing port parameters of exactly length 255 which ends with a '$'. But since the port parameter parser then steps outside of the boundaries of the array you can get it in the next round to read further beyond the one char outside of the array which the ASAN build fails on.

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8984314 [details] [diff] [review]
bug1464063.patch

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

Seems to me that the real bug is here:

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c#477

and also here:

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c#484

because we already increment here:

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c#474

I don't see other tokenizer functions that point str_end _after_ the null terminator, the rest seem to point it _at_ the null terminator.

Comment 11

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #10)
> Comment on attachment 8984314 [details] [diff] [review]
> bug1464063.patch
> 
> Review of attachment 8984314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems to me that the real bug is here:
> 
> https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/
> sipcc/sdp_utils.c#477

Yes. Based on the comment in line 477 I assume that there is the assumption that \r and \n are skipped by this function. But skipping over \0 is clearly the bug here. Because the buffer fed into the function is null terminated. So if the buffer end on '$\0' this will skip over the null termination.
Since I'm not sure if other code depends on \r and \n getting skipped I decided to do the special case handling of \0 only.

> and also here:
> 
> https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/
> sipcc/sdp_utils.c#484

Initially I put that same check for \0 here as well. But decided against it, because the if condition does not contain the dangerous \0 limiter. Only if you would feed \0 as one of the delimiters into the function this part would also cause problems. But if someone feeds \0 as a token delimiter I think that is a bigger problem.
I'm not opposed to add extra safety here as well.

> because we already increment here:
> 
> https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/
> sipcc/sdp_utils.c#474
> 
> I don't see other tokenizer functions that point str_end _after_ the null
> terminator, the rest seem to point it _at_ the null terminator.

Right. That's why the sdp_utils.c part of my patch tries to ensure it returns on the null terminator.

The part where I modified sdp_token.c is basically additional safety (I verified that both modifications by them self fix this issue). As the code in sdp_parse_media() knows the buffer size, but there is no way to tell sdp_getchoosetok() where the end is. Which I think is the real root cause here. But fixing all of that in sipcc would a major project....

The other big question is: what is this code searching for '$' actually for? I'm not aware of any special meaning of '$' in the parts of SDP we care about/use. Maybe in some Cisco dialect on can use '$' at the end of a line to indicate that the line continues on the next line?
So another option might be to completely rip out https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c#472-489

Comment 12

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #11)
> The other big question is: what is this code searching for '$' actually for?
> I'm not aware of any special meaning of '$' in the parts of SDP we care
> about/use. Maybe in some Cisco dialect on can use '$' at the end of a line
> to indicate that the line continues on the next line?
> So another option might be to completely rip out
> https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/
> sipcc/sdp_utils.c#472-489

I would be totally fine with ripping that bit out entirely.

Comment 13

[Nils Ohlmeier [:drno]]

Attachment: bug1464063.patch

Just removed the whole special case handling for '$'.

Comment 14

[Nils Ohlmeier [:drno]]

Attachment: bug1464063.patch

This is the patch we talked about which removes sdp_getchoosetok() all together.

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8985241 [details] [diff] [review]
bug1464063.patch

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

LGTM

Comment 16

[Nils Ohlmeier [:drno]]

Comment on attachment 8985241 [details] [diff] [review]
bug1464063.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Since we removed a whole function which contained the buggy code I think it's not that easy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? Everything up to release and both ESR branches.

If not all supported branches, which bug introduced the flaw? Probably the original landing on the sipcc parser in Firefox 3x.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch should apply cleanly on all branches.

How likely is this patch to cause regressions; how much testing does it need? It is not very likely to cause regressions, since it's only for parsing a special feature in SDP Firefox WebRTC implementation doesn't use.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Ryan, how do you feel about taking this on 61 and ESR branches since we're doing RCs next week?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Given the risk assessment, I'd be OK taking it.

Comment 19

[Nils Ohlmeier [:drno]]

Comment on attachment 8985241 [details] [diff] [review]
bug1464063.patch

Approval Request Comment
[Feature/Bug causing the regression]: Landing of original sipcc parser code in Firefox 3x.
[User impact if declined]: Potential crashes
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: The patch removes code from the parser, which is not used by WebRTC at all.
[String changes made/needed]: N/A

Comment 20

[Nils Ohlmeier [:drno]]

Comment on attachment 8985241 [details] [diff] [review]
bug1464063.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Potential crashes
Fix Landed on Version: Not landed yet, probably 62
Risk to taking this patch (and alternatives if risky): Not very risky, as the patch only removes a function which is not used by the WebRTC code.
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 21

[Nils Ohlmeier [:drno]]

Comment on attachment 8985241 [details] [diff] [review]
bug1464063.patch

[Approval Request Comment]
See comment #20

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8985241 [details] [diff] [review]
bug1464063.patch

approvals given

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87163f9d6bc7670d074512cf96062ea01193ffb2

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87163f9d6bc7

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3c164d94ac53
https://hg.mozilla.org/releases/mozilla-esr60/rev/46efbe00b7c0

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/9d4d31b2630d