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..6db68bdbbe7fa3dbd4ad10e90cb08ec4decb52fa
--- /dev/null
+++ b/testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc
@@ -0,0 +1,131 @@
+// 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 <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.
+
+enum LibSrtpFuzzerCryptoPolicy {
+  kLibSrtpFuzzerNone,

[mmoroz, 2016/07/05 14:20:19]

Chromium coding style says to use MACRO_STYLE naming for enum elements:
https://www.chromium.org/developers/coding-style#TOC-Naming

[katrielc, 2016/07/05 15:04:39]

Done.

+  kLibSrtpFuzzerLikeWebRTC,
+  kLibSrtpFuzzerLikeWebRTCWithoutAuth,
+  kLibSrtpFuzzerAESGCM,
+  kLibSrtpFuzzerNumberOfPolicies,
+};
+
+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;
+};
+
+struct Environment {
+  srtp_t session;
+  srtp_policy_t policy;
+
+  void setCryptoPolicy(LibSrtpFuzzerCryptoPolicy crypto_policy) {

[mmoroz, 2016/07/05 14:20:19]

setCryptoPolicy -> SetCryptoPolicy

[katrielc, 2016/07/05 15:04:39]

Done.

+    switch (crypto_policy) {
+      case kLibSrtpFuzzerNumberOfPolicies:
+      case kLibSrtpFuzzerNone:
+        crypto_policy_set_null_cipher_null_auth(&policy.rtp);
+        crypto_policy_set_null_cipher_null_auth(&policy.rtcp);
+        break;
+      case kLibSrtpFuzzerLikeWebRTC:
+        crypto_policy_set_aes_cm_128_hmac_sha1_80(&policy.rtp);
+        crypto_policy_set_aes_cm_128_hmac_sha1_80(&policy.rtcp);
+      case kLibSrtpFuzzerLikeWebRTCWithoutAuth:
+        crypto_policy_set_aes_cm_128_null_auth(&policy.rtp);
+        crypto_policy_set_aes_cm_128_null_auth(&policy.rtcp);
+        break;
+      case kLibSrtpFuzzerAESGCM:
+        // 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;
+    }

[mmoroz, 2016/07/05 14:20:19]

Missing 'default' case:
https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

[katrielc, 2016/07/05 15:04:40]

Style guide says "if not conditional on an enumerated value", so this is OK I
think? There will be a compiler error if not all cases are handled.

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

Good point! I misread that, sorry.

+  };
+
+  Environment() {
+    srtp_init();
+
+    memset(&policy, 0, sizeof(policy));
+    policy.key = (unsigned char*) "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234";

[mmoroz, 2016/07/05 14:20:18]

Please prefer C++ type casting (i.e. static_cast<unsigned char*>())

[katrielc, 2016/07/05 15:04:40]

Done.

+    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;
+
+    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();

[mmoroz, 2016/07/05 14:20:18]

Add an empty line to separate this from the function below.

[katrielc, 2016/07/05 15:04:40]

Done.

+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;
+  LibSrtpFuzzerCryptoPolicy policy = static_cast<LibSrtpFuzzerCryptoPolicy>(
+      data[0] % kLibSrtpFuzzerNumberOfPolicies);
+  data += 1;
+  size -= 1;
+
+  // Read some more bytes and use them to choose a key.
+  if (size <= SRTP_MASTER_KEY_LEN)
+    return 0;
+  env->policy.key = const_cast<unsigned char*>(data);

[mmoroz, 2016/07/05 14:20:19]

I think it's worth to create another object (vector or another container) with
size of SRTP_MASTER_KEY_LEN and values copied from (data, data +
SRTP_MASTER_KEY_LEN).

Otherwise, ASan will miss out-of-bounds READ to the right (assume that some
function working with |env->policy.key| will read (SRTP_MASTER_KEY_LEN + 1)
bytes from the pointer -> this is definitely bad, but ASan may miss it, because
|data| points to a bigger number of bytes -> ASan doesn't know that (data +
SRTP_MASTER_KEY_LEN + 1) is out-of-bounds).

If you will see a significant drop of execution speed, I can recommend to use
static stack buffer of key size and just copy key value there every time. From
my experience it could be the fastest option, but it's always better to try and
experiment a bit :)

[katrielc, 2016/07/05 15:04:39]

Yes, you're right -- I didn't think about OOB reads of the key, but we should
certainly check for them :)

Done.

+  data += SRTP_MASTER_KEY_LEN;
+  size -= SRTP_MASTER_KEY_LEN;
+
+  // Create a session with our chosen key and policy.
+  srtp_t session;
+  env->setCryptoPolicy(policy);
+  srtp_create(&session, &env->policy);

[mmoroz, 2016/07/05 14:20:19]

Don't we need to check the result here?

[katrielc, 2016/07/05 15:04:40]

Done.

+
+  // 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;
+
+    // This guard is necessary because libsrtp assumes it.
+    if (packet_size < sizeof(srtp_hdr_t))
+      continue;
+
+    int out_len = static_cast<int>(packet_size);
+    rtp_msg_t* message = (rtp_msg_t*)(data + 1);

[mmoroz, 2016/07/05 14:20:19]

C++ type cast please.

+ the same thing as for line 103, would be better to use another buffer with
only |packet_size| bytes copied there.

[katrielc, 2016/07/05 15:04:39]

Done.

+    srtp_unprotect(session, &message->header, &out_len);
+
+    data += packet_size + 1;
+  }
+
+  srtp_dealloc(session);
+  return 0;
+}