1373843 - Add a fuzztest for STUN parsing

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, enhancement, P3)

Description

[Nils Ohlmeier [:drno]]

Let's add a libfuzzer test for fuzzing the STUN parser in nICEr as the first step.

Note: marking this as security to be on the safe side until we know if we are going to find any problems with it.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: bug1373843.patch

Dan this is the libfuzzer test for nICEr's STUN parsing function I have come up with.

I have been running this now for several hours on my Linux desktop machine, without any major finding so far.

Comment 2

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

Mass change P3->P4 to align with new Mozilla triage process.

Comment 3

[Christoph Diehl [:posidron]]

Nils can we land this so it can run in our fuzzing cloud?

Comment 4

[Dan Minor [:dminor]]

I don't have anything in my notes to remind me of what else we wanted to do here. The attached patch still applies and builds cleanly. Nils, please let me know what the next steps are here and I'd be happy to pick this up again.

Comment 5

[Nils Ohlmeier [:drno]]

I know the other fuzz client I wrote http://searchfox.org/mozilla-central/source/media/webrtc/signaling/fuzztest landed with some problems, which got fixed after it landed. Maybe double check that I did not made the same mistakes here.

And then we only need to find someone for review.

Comment 6

[Dan Minor [:dminor]]

Attachment: Fuzz stun parser

The original patch was missing some build system integration; now builds and runs fine on my local system.

Comment 7

[Christian Holler (:decoder)]

Comment on attachment 8909312 [details] [diff] [review]
Fuzz stun parser

Looks good to me. Before landing this, please confirm that it doesn't hit bugs immediately. We wouldn't want to 0-day ourselves :D

Comment 8

[Dan Minor [:dminor]]

I'll run it overnight and see what happens. So far, I've only hit an assertion which could be changed to be an error.

Comment 9

[Dan Minor [:dminor]]

Attachment: Change assert to error in stun_codec.c

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8909945 [details] [diff] [review]
Change assert to error in stun_codec.c

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

With an update error message: LGTM

::: media/mtransport/third_party/nICEr/src/stun/stun_codec.c
@@ +737,5 @@
>      header->length = htons(length);
>  
>      /* make sure FINGERPRINT is final attribute in message */
> +    if (length + sizeof(*header) != buflen) {
> +        r_log(NR_LOG_STUN, LOG_WARNING, "Unable to compute fingerprint");

Lets change the text here to something about the fingerprint not being at the end of the message, so we can distinguish from the next error one down.

Comment 11

[Dan Minor [:dminor]]

I left this to run overnight without finding anything more.

Comment 12

[Dan Minor [:dminor]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34ddff9dde50
https://hg.mozilla.org/integration/mozilla-inbound/rev/94f776f0a816

Comment 13

[Dan Minor [:dminor]]

Attachment: Set nicer.gyp cflags for fuzzing

Comment 14

[Christoph Diehl [:posidron]]

Comment on attachment 8910484 [details] [diff] [review]
Set nicer.gyp cflags for fuzzing

Yup. I recall we had similar issues as we tried to land the SdpParser fuzzer.

Comment 15

[Dan Minor [:dminor]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51711f2c9cae02483300eaaf12badc73a39e24b1

Comment 16

[Christoph Diehl [:posidron]]

Successfully running in our cloud. Thanks!

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/34ddff9dde50
https://hg.mozilla.org/mozilla-central/rev/94f776f0a816

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51711f2c9cae