814825 - WebRTC RTP/RTCP Fuzzer Patch

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P4)

Description

[Christoph Diehl [:posidron]]

Attachment: RTP-RTCP_fuzzer_v1

Eric and I decided to open a separate bug for this, to potentially implement this patch into the unit-test upstream. This bug shall remain hidden until further action has taken place.

Usage:
$ export MOZ_WEBRTC_TESTS=1
$ export DYLD_LIBRARY_PATH=<OBJDIR>dist/lib

Supported Environment Variables
$ export RTP_FUZZER
$ export RTCP_FUZZER
$ export RANDOM_SEED

Deactivate
$ unset RTP_FUZZER
$ unset RTCP_FUZZER

Execution
$ <OBJDIR>media/webrtc/signaling/test/mediaconduit_unittests

Edit/Compile
$ media/webrtc/signaling/test/mediaconduit_unittests.cpp
$ cd <OBJDIR>/media/webrtc/signaling/test && make mediaconduit


Tested with m-c changeset: 114084:0d373cf880fd

Comment 1

[Eric Rescorla (:ekr)]

Comment on attachment 684834 [details] [diff] [review]
RTP-RTCP_fuzzer_v1

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

A bunch of suggested improvements below.

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +407,5 @@
>   *  to the conduit for eventual decoding and rendering.
>   */
> +unsigned int RandomNumber(unsigned int max)
> +{
> +    return (unsigned int)(rand() % max);

You might consider using the PRNG in this code so that you can guarantee deterministic results in case someone else uses rand().

Please use C++-style casts throughout this patch.

@@ +412,5 @@
> +}
> +
> +template <class T> T RandomNumberRange(T min, T max)
> +{
> +    T x = (T)rand() % (max - min) + min;

Suggest a compile-time assert here that sizeof(T) <= sizeof(int). This ensures you are getting the full range of the PRNG.

@@ +420,5 @@
> +void Hexdump(unsigned char *buf, int len)
> +{
> +  int i, j;
> +
> +  for (i = 0; i < len; i += 16) {

Ordinarily, I would suggest restructuring this code to build one line at a time and
then emit it. But in test code it doesn't really matter.

@@ +458,5 @@
>    ~FakeMediaTransport()
>    {
>    }
>  
> +  const void *MutatePacket(const void *data, int len, const char* type)

Suggest changing this code to take "uint8_t *data" and then change the caller to do the const-cast.

Better yet, honestly, would be to have the caller copy the data. Generally un-consting a const variable so that you can mutate it is really bad news as you're explicitly violating the interface contract. I usually try to use const_cast only to bridge interface boundaries where I know that the other side will not modify the data, but just isn't const correct.

@@ +465,5 @@
> +    std::cerr << "Seed: " << seed << std::endl;
> +    std::cerr << "Packet: " << std::endl;
> +    unsigned char *packet = (unsigned char*)data;
> +    Hexdump(packet, len);
> +    

Blank space.

@@ +468,5 @@
> +    Hexdump(packet, len);
> +    
> +    uint32_t maxMutations = RandomNumber(len);
> +    for (uint32_t i = 0; i < maxMutations; ++i) {
> +      uint32_t position = RandomNumberRange<uint32_t>(0, len);

Don't we want int rather than uint32_t because otherwise we are doing an unsigned-signed conversion.

@@ +469,5 @@
> +    
> +    uint32_t maxMutations = RandomNumber(len);
> +    for (uint32_t i = 0; i < maxMutations; ++i) {
> +      uint32_t position = RandomNumberRange<uint32_t>(0, len);
> +      *(packet+position) = RandomNumberRange<unsigned char>(0, 255);

suggest packet[position]

@@ +485,5 @@
>      if(mAudio)
>      {
> +      if (isRTPFuzzerEnabled)
> +      {
> +        const void *packet = MutatePacket(data,len, "ReceiveRTPPacketAudio");

I would suggest copying into a temporary. We know (through external knowledge) that WebRTC will never produce a packet bigger than the Ethernet MTU. So, have a member variable like so:

unsigned char mBuffer[1500];


Then here do:
  if (isRTPFuzzerEnabled) {
    MOZ_ASSERT(len <= sizeof(mBuffer));
    int temp_len = std::min(len, sizeof(mBuffer));

    memcpy(mBuffer, data, temp_len);
    MutatePacket(mBuffer, temp_len);
    data = mBuffer;
    len = temp_len;
 }

And then you won't need the else.

Also, you can push this code up above the if (mAudio), because you are replacing data, thus making the code simpler.

@@ +844,5 @@
>  {
>    // This test can cause intermittent oranges on the builders
>    CHECK_ENVIRONMENT_FLAG("MOZ_WEBRTC_TESTS")
>  
> +  // Initialize RTP/RTCP Fuzzer

I would suggest that instead of using environment variables, you simply make these
a standard part of the test harness.

I.e., add a member function to TransportConduitTest like:

void EnableFuzzing(bool aRTP, bool aRTCP);

And then you can simply have new tests, like:


TEST_F(TransportConduitTest, TestDummyAudioWithTransportFuzzingRTP) {
  EnableFuzzing(true, false);
  TestDummyAudioAndTransport();
}
That way people can just use the standard gtest controls.

And you want to just fuzz but do it a lot? No problem:

mediaconduit_unittests '--gtest_filter=*Fuzz*' --gtest_repeat=100

I would suggest also making the seed an argument rather than an env variable, but that's more of a taste thing.

Comment 2

[Christoph Diehl [:posidron]]

Attachment: RTP-RTCP_fuzzer_v2

Thanks for reviewing Eric. I have applied those recommendations to the new patch.

New command to execute:

<OBJDIR>/media/webrtc/signaling/test/mediaconduit_unittests --gtest_filter=*FuzzingRTCP* --gtest_repeat=1000 --seed 123456

Whereby --seed is optional and gtest_filter can be adjusted to FuzzingRTP, FuzzingRTCP or FuzzingRTPAndRTCP.

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 685106 [details] [diff] [review]
RTP-RTCP_fuzzer_v2

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

This is much improved. One real bug and one potential bug below, plus a 
bunch of minor comments.

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +29,5 @@
> +//RTP/RTCP Fuzzer
> +#define MAX_SIZE_MTU 1500
> +unsigned int seed = 0;
> +bool isRTPFuzzerEnabled;
> +bool isRTCPFuzzerEnabled;

These two should be member variables of  TransportConduitTest or, perahaps of FakeMediaTransport but frobbed via TCTest. They will also need sensible initializors (false, false).

Note this is currently a bug since if you run the entire test harness on repeat, it will misbehave in the non-fuzzing tests.

@@ +408,5 @@
>   *  to the conduit for eventual decoding and rendering.
>   */
> +uint32_t RandomNumber(int max)
> +{
> +  return static_cast<uint32_t>(random() % max);

random(3) returns long, so probably this should return long as well.

@@ +413,5 @@
> +}
> +
> +template <class T> T RandomNumberRange(T min, T max)
> +{
> +  MOZ_ASSERT(sizeof(T) <= sizeof(int));

This should be "long" for the reasons above.

Also MOZ_ASSERT(max < LONG_MAX) and MOZ_ASSERT(min < max).

@@ +414,5 @@
> +
> +template <class T> T RandomNumberRange(T min, T max)
> +{
> +  MOZ_ASSERT(sizeof(T) <= sizeof(int));
> +  T x = static_cast<T>(random() % (max - min) + min);

This assignment is unnecessary, since you can just do "return static_cast<>..."

@@ +460,5 @@
>    ~FakeMediaTransport()
>    {
>    }
>  
> +  const void *MutatePacket(unsigned char*packet, int len, const char* type)

You never use the return value of MutatePacket How about have it return void.

@@ +485,5 @@
> +          packet[position] = '\xff';
> +      }
> +    }
> +
> +    cerr << "Packet (Mutated Fields: " << maxMutations << "): " << endl;

Note that this potentially returns the wrong answer if you coincidentally mutate the same field twice.

@@ +496,5 @@
>    {
>      ++numPkts;
> +    unsigned char mBuffer[MAX_SIZE_MTU];
> +    MOZ_ASSERT(len <= sizeof(mBuffer));
> +    memset(mBuffer, 0, sizeof(mBuffer));

What is this memset for?

@@ +503,3 @@
>      if(mAudio)
>      {
> +      if (isRTPFuzzerEnabled)

If you pull these above the mAudio check you could save some duplication.

@@ +523,5 @@
>    virtual nsresult SendRtcpPacket(const void* data, int len)
>    {
> +    unsigned char mBuffer[MAX_SIZE_MTU];
> +    MOZ_ASSERT(len <= sizeof(mBuffer));
> +    memset(mBuffer, 0, sizeof(mBuffer));

Why the memset?

@@ +911,5 @@
> +        return 1;
> +      }
> +    }
> +  }
> +  srandom(seed);

This does not behave the way you would expect in the case of repeats. Do you want to refactor it so that you re-srandom() on every individual test? (You do this with SetUp in the test class).

Comment 4

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

(In reply to Eric Rescorla from comment #3)

> @@ +413,5 @@
> > +}
> > +
> > +template <class T> T RandomNumberRange(T min, T max)
> > +{
> > +  MOZ_ASSERT(sizeof(T) <= sizeof(int));
> 
> This should be "long" for the reasons above.
> 
> Also MOZ_ASSERT(max < LONG_MAX) and MOZ_ASSERT(min < max).

I'd add MOZ_ASSERT(min >= 0) as well.


> 
> @@ +496,5 @@
> >    {
> >      ++numPkts;
> > +    unsigned char mBuffer[MAX_SIZE_MTU];
> > +    MOZ_ASSERT(len <= sizeof(mBuffer));
> > +    memset(mBuffer, 0, sizeof(mBuffer));
> 
> What is this memset for?

I'm guessing general cleanliness and to allow easier visibility of what's been modified in a debugger (without actually looking at the code block, so I could be wrong!)

Comment 5

[Christoph Diehl [:posidron]]

Attachment: RTP-RTCP_fuzzer_v3

I used memset() to prevent having uninitialized memory in the buffer. I have attached a new version with the latest recommendations applied. The command to run the fuzzer has not changed.

Comment 6

[Suhas]

I had a basic question ... What are the mutations on the RTP/RTCP packet data supposed to test with respect to the conduit .. Sorry if I am missing something fundamental  here

Comment 7

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

Comment on attachment 685648 [details] [diff] [review]
RTP-RTCP_fuzzer_v3

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

r- for me due to the "how do I find which packet caused what error issue" (i.e. fuzz on receive not send.)

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +479,5 @@
> +    isRTPFuzzerEnabled = aRTP;
> +    isRTCPFuzzerEnabled = aRTCP;
> +  }
> +
> +  void MutatePacket(unsigned char*packet, int len)

char *packet, or if the file prefers this style char* packet

@@ +482,5 @@
> +
> +  void MutatePacket(unsigned char*packet, int len)
> +  {
> +    cerr << "Seed: " << seed << endl;
> +    cerr << "Packet:" << endl;

If this is used with data in both directions, we won't be able to tell which direction this is for except very indirectly (RTP header values in the packet dump).

@@ +484,5 @@
> +  {
> +    cerr << "Seed: " << seed << endl;
> +    cerr << "Packet:" << endl;
> +
> +    Hexdump(packet, len);

Realize that's a TON of data for video, and a lot for audio.  But it's handy for tracking down why it failed.  Note that the failure will be on the OTHER side, so if this is used in a non-loopback way, you need to look at the *other* side for the data - and then you may not be able to figure out which packet killed it, as the sender will happily continue.

Thus, I think it should mutate (and log) on *reception* (after decrypt), not on send.

@@ +499,5 @@
> +        case 1:
> +          packet[position] = '\x00';
> +          break;
> +        default:
> +          packet[position] = '\xff';

So, 50% 0xFF, 25% 0x00, 25% 0-0xFF

Mutations like this will likely cause toss-away in the RTP layer (if they hit the RTP header, or many mutations in RTCP) due to violating constraints.  Still useful though, especially as a start.  A more detailed fuzzer would know reasonable mutations of RTP/RTCP so as to stress more than the "invalid packet" logic, and also have a mode to fuzz codec data only or RTP/RTCP data only.

Comment 8

[Nils Ohlmeier [:drno]]

Christoph are still planing on landing this, or can we close it?

Comment 9

[Christoph Diehl [:posidron]]

Nä, the approach wasn't good and was only run as WebRTC got built into Firefox. We should stick with LibFuzzer now.