Add a fuzzer for srtp_unprotect in libsrtp.

testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc

Index: testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc
diff --git a/testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc b/testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0a463111999d624f8f7e68635d7b5df5a1c84275
--- /dev/null
+++ b/testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc
@@ -0,0 +1,146 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <assert.h>
+#include <stddef.h>
+#include <stdint.h>
+
+#include "third_party/libsrtp/srtp/include/rtp.h"
+#include "third_party/libsrtp/srtp/include/rtp_priv.h"
+#include "third_party/libsrtp/srtp/include/srtp.h"
+
+// TODO(katrielc) Also test the authenticated path, which is what
+// WebRTC uses.  This is nontrivial because you need to bypass the MAC
+// check. Two options: add a UNSAFE_FUZZER_MODE flag to libsrtp (or
+// the chromium fork of it), or compute the HMAC of whatever gibberish
+// the fuzzer produces and write it into the packet manually.
+
+namespace LibSrtpFuzzer {
+enum CryptoPolicy {
+  NONE,
+  LIKE_WEBRTC,
+  LIKE_WEBRTC_WITHOUT_AUTH,
+  AES_GCM,
+  NUMBER_OF_POLICIES,
+};
+}
+
+void crypto_policy_set_null_cipher_null_auth(crypto_policy_t* p) {
+  p->cipher_type = NULL_CIPHER;
+  p->cipher_key_len = 0;
+  p->auth_type = NULL_AUTH;
+  p->auth_key_len = 0;
+  p->auth_tag_len = 0;
+  p->sec_serv = sec_serv_none;
+};
+
+// Environment holds some structs and buffers so that we don't need to
+// reallocate them on every fuzzer iteration.
+struct Environment {
+  srtp_t session;
+  srtp_policy_t policy;
+  unsigned char key[30] = {0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48,

[mmoroz, 2016/07/05 16:06:21]

Would be better to use SRTP_MASTER_KEY_LEN instead of hardcoded |30| value.

Does it make sense to have that default value here, or may be { 0 } is enough?

[katrielc, 2016/07/05 16:26:02]

Absolutely, yes :)

+                           0x49, 0x4a, 0x4b, 0x4c, 0x4d, 0x4e, 0x4f, 0x50,
+                           0x51, 0x52, 0x53, 0x54, 0x55, 0x56, 0x57, 0x58,
+                           0x59, 0x5a, 0x31, 0x32, 0x33, 0x34};
+
+  srtp_policy_t GetCryptoPolicy(LibSrtpFuzzer::CryptoPolicy crypto_policy,
+                                const unsigned char* replacement_key) {
+    // Update some values and return the resulting srtp_policy_t.
+
+    switch (crypto_policy) {
+      case LibSrtpFuzzer::NUMBER_OF_POLICIES:
+      case LibSrtpFuzzer::NONE:
+        crypto_policy_set_null_cipher_null_auth(&policy.rtp);
+        crypto_policy_set_null_cipher_null_auth(&policy.rtcp);
+        break;
+      case LibSrtpFuzzer::LIKE_WEBRTC:
+        crypto_policy_set_aes_cm_128_hmac_sha1_80(&policy.rtp);
+        crypto_policy_set_aes_cm_128_hmac_sha1_80(&policy.rtcp);
+      case LibSrtpFuzzer::LIKE_WEBRTC_WITHOUT_AUTH:
+        crypto_policy_set_aes_cm_128_null_auth(&policy.rtp);
+        crypto_policy_set_aes_cm_128_null_auth(&policy.rtcp);
+        break;
+      case LibSrtpFuzzer::AES_GCM:
+        // There was a security bug in the GCM mode in libsrtp 1.5.2.
+        crypto_policy_set_aes_gcm_128_8_auth(&policy.rtp);
+        crypto_policy_set_aes_gcm_128_8_auth(&policy.rtcp);
+        break;
+    }
+
+    memcpy(key, replacement_key, SRTP_MASTER_KEY_LEN);
+    return policy;
+  };
+
+  Environment() {
+    srtp_init();
+
+    memset(&policy, 0, sizeof(policy));
+    policy.ssrc.type = ssrc_any_inbound;
+    policy.ssrc.value = 0xdeadbeef;
+    policy.window_size = 1024;
+    policy.allow_repeat_tx = 1;
+    policy.ekt = nullptr;
+    policy.next = nullptr;
+
+    // Set some defaults in case we don't want to override them later.
+    policy.key = key;
+    crypto_policy_set_null_cipher_null_auth(&policy.rtp);
+    crypto_policy_set_null_cipher_null_auth(&policy.rtcp);
+  }
+};
+
+size_t ReadLength(const uint8_t* data, size_t size) {
+  // Read one byte of input and check that that many bytes remain.
+  if (size == 0)
+    return 0;
+  size_t n = static_cast<size_t>(data[0]);
+
+  if (n > size - 1)
+    return 0;
+  else
+    return n;
+}
+
+Environment* env = new Environment();
+
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
+  // Read one byte and use it to choose a crypto policy.
+  if (size <= 1)
+    return 0;
+  LibSrtpFuzzer::CryptoPolicy policy = static_cast<LibSrtpFuzzer::CryptoPolicy>(
+      data[0] % LibSrtpFuzzer::NUMBER_OF_POLICIES);
+  data += 1;
+  size -= 1;
+
+  // Read some more bytes to use as a key.
+  if (size <= SRTP_MASTER_KEY_LEN)
+    return 0;
+  srtp_policy_t srtp_policy = env->GetCryptoPolicy(policy, data);
+  data += SRTP_MASTER_KEY_LEN;
+  size -= SRTP_MASTER_KEY_LEN;
+
+  // Create a session with the above data.
+  srtp_t session;
+  err_status_t error = srtp_create(&session, &srtp_policy);
+  assert(error == err_status_ok);
+
+  // Read one byte as a packet length N, then feed the next N bytes
+  // into srtp_unprotect. Keep going until we run out of data.
+  size_t packet_size;
+  while ((packet_size = ReadLength(data, size)) > 0) {
+    size -= packet_size + 1;
+
+    int out_len = static_cast<int>(packet_size);
+    unsigned char raw_packet[packet_size];

[mmoroz, 2016/07/05 16:06:21]

Looks like variable-length arrays are not allowed:
https://google.github.io/styleguide/cppguide.html#Variable-Length_Arrays_and_...

Let's use something like:

std::vector<unsigned char> raw_packet(data + 1, data + 1 + packet_size);

[katrielc, 2016/07/05 16:26:03]

Haha, I should have read the style guide better! libsrtp is written in plain C
so I keep switching between the two :$ :)

+    memcpy(raw_packet, data + 1, packet_size);
+    srtp_unprotect(session, &reinterpret_cast<rtp_msg_t*>(raw_packet)->header,

[mmoroz, 2016/07/05 16:06:21]

I cannot understand:
1) we read some "random" size |packet_size|
2) then we get |packet_size| bytes from |data| to our buffer (|raw_packet|)
3) then we cast that buffer of random bytes to an object of |rtp_msg_t| type,
which should have some strictly defined size

Shouldn't we read |sizeof(rtp_msg_t)| bytes from |data| to |raw_packet|, and
then cast it to |rtp_msg_t| object? Otherwise how is real size of the packet
controleld?

Another idea - if we need to pass only |->header| member of |rtp_msg_t|, why
don't read |sizeof(rtp_hdr_t)| bytes from |data| and cast it to |rtp_hdr_t|
object?

[katrielc, 2016/07/05 16:26:03]

rtp_msg_t is a C struct of a srtp_something_t and a char[MAX_PACKET_SIZE], but
you don't have to use all the bytes in the char[]. srtp_unprotect takes an
rtp_msg_t and a length N, and reads the header and N bytes of the array. (Aside:
N is used both for the input and the output length. Ugh.)

A pointer to the first element of a struct is the same as a pointer to the
struct, so I think it was equivalent to write foo and foo->header -- but you're
right it's cleaner to omit the former. Changed.

(My pure C is rather rusty though, so please correct me if I'm being silly.)

[mmoroz, 2016/07/05 16:40:10]

Hm, |out_len| is also used as an input length -- I missed that. Makes sense. But
anyway we cannot interpret M bytes as an object of size of N bytes, when M < N.
Let's do the following:

1) declare global |static rtp_msg_t message = { 0 };|
2) fill |message.header| with |sizeof(rtp_hdr_t)| bytes from |data|
3) copy |packet_size - sizeof(rtp_hdr_t)| into |message.body|

Thus we will have a valid object in memory (without attempting to guess the
order of members, it is not specified AFAIK and is definitely a bad idea).

[katrielc, 2016/07/05 17:47:31]

What do you think of this? I think it's important to catch OOB reads even if
using a long global array for messages.

+                   &out_len);
+
+    data += packet_size + 1;
+  }
+
+  srtp_dealloc(session);
+  return 0;
+}