Closed
Bug 814825
Opened 12 years ago
Closed 6 years ago
WebRTC RTP/RTCP Fuzzer Patch
Categories
(Core :: WebRTC, defect, P4)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
backlog | parking-lot |
People
(Reporter: posidron, Assigned: posidron)
References
Details
(Keywords: sec-other, stalled, Whiteboard: [WebRTC], [blocking-webrtc-])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jesup
:
review-
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•12 years ago
|
Attachment #684834 -
Flags: review?(ekr)
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Attachment #684834 -
Attachment is obsolete: true
Attachment #684834 -
Flags: review?(ekr)
Attachment #685106 -
Flags: review?(ekr)
Comment 3•12 years ago
|
||
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•12 years ago
|
||
(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!)
Assignee | ||
Comment 5•12 years ago
|
||
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.
Assignee: nobody → cdiehl
Attachment #685106 -
Attachment is obsolete: true
Attachment #685106 -
Flags: review?(ekr)
Attachment #685648 -
Flags: review?(ekr)
Updated•12 years ago
|
Priority: -- → P1
Whiteboard: [WebRTC], [blocking-webrtc-]
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•12 years ago
|
||
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.
Attachment #685648 -
Flags: review-
Updated•10 years ago
|
Attachment #685648 -
Flags: review?(ekr)
Updated•9 years ago
|
Severity: critical → normal
backlog: --- → parking-lot
Priority: P1 → --
Updated•9 years ago
|
Group: core-security → media-core-security
Updated•7 years ago
|
Has Regression Range: --- → irrelevant
Comment 8•6 years ago
|
||
Christoph are still planing on landing this, or can we close it?
Assignee | ||
Comment 9•6 years ago
|
||
Nä, the approach wasn't good and was only run as WebRTC got built into Firefox. We should stick with LibFuzzer now.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(cdiehl)
Resolution: --- → FIXED
Updated•6 years ago
|
Group: media-core-security → core-security-release
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•