Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2123553002:
Add a fuzzer for srtp_unprotect in libsrtp. (Closed)
DescriptionAdd a fuzzer for srtp_unprotect in libsrtp.
This is called on any SRTP packet received, and decrypts it in
place. For now the fuzzer uses a hard-coded key and SSRC, but future
modifications could generalise this in a number of ways.
It does read more than one packet, since there could be bugs lurking
in the state maintained by the parser (for example, the ID of each
packet is stored for replay protection).
Committed: https://crrev.com/346b92100d3ebfe43ec8c226d8bf0fa226c22527
Cr-Commit-Position: refs/heads/master@{#403893}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Move comment along with moved code. #Patch Set 4 : MODULAR IS ALWAYS BETTER #
Total comments: 17
Patch Set 5 : Code review #
Total comments: 8
Patch Set 6 : Use a global message and ASAN poison it. #
Total comments: 6
Patch Set 7 : Don't manually poison; MSAN will catch it. #
Total comments: 6
Patch Set 8 : Don't waste bytes. #Messages
Total messages: 19 (4 generated)
katrielc@chromium.org changed reviewers: + mmoroz@chromium.org
New fuzzer for you :) The trybot doesn't like it, but I'm guessing it's an infra problem. If it doesn't go away tomorrow I'll take a closer look.
https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:19: kLibSrtpFuzzerNone, Chromium coding style says to use MACRO_STYLE naming for enum elements: https://www.chromium.org/developers/coding-style#TOC-Naming https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:39: void setCryptoPolicy(LibSrtpFuzzerCryptoPolicy crypto_policy) { setCryptoPolicy -> SetCryptoPolicy https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:58: } Missing 'default' case: https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:65: policy.key = (unsigned char*) "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234"; Please prefer C++ type casting (i.e. static_cast<unsigned char*>()) https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:90: Environment* env = new Environment(); Add an empty line to separate this from the function below. https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:103: env->policy.key = const_cast<unsigned char*>(data); 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 :) https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:110: srtp_create(&session, &env->policy); Don't we need to check the result here? https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:123: rtp_msg_t* message = (rtp_msg_t*)(data + 1); 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.
https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:19: kLibSrtpFuzzerNone, On 2016/07/05 14:20:19, mmoroz wrote: > Chromium coding style says to use MACRO_STYLE naming for enum elements: > https://www.chromium.org/developers/coding-style#TOC-Naming Done. https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:39: void setCryptoPolicy(LibSrtpFuzzerCryptoPolicy crypto_policy) { On 2016/07/05 14:20:19, mmoroz wrote: > setCryptoPolicy -> SetCryptoPolicy Done. https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:58: } On 2016/07/05 14:20:19, mmoroz wrote: > Missing 'default' case: 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. https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:65: policy.key = (unsigned char*) "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234"; On 2016/07/05 14:20:18, mmoroz wrote: > Please prefer C++ type casting (i.e. static_cast<unsigned char*>()) Done. https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:90: Environment* env = new Environment(); On 2016/07/05 14:20:18, mmoroz wrote: > Add an empty line to separate this from the function below. Done. https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:103: env->policy.key = const_cast<unsigned char*>(data); On 2016/07/05 14:20:19, mmoroz wrote: > I think it's worth to create another object (vector or another container) with > size of SRTP_MASTER_KEY_LEN Yes, you're right -- I didn't think about OOB reads of the key, but we should certainly check for them :) Done. https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:110: srtp_create(&session, &env->policy); On 2016/07/05 14:20:19, mmoroz wrote: > Don't we need to check the result here? Done. https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:123: rtp_msg_t* message = (rtp_msg_t*)(data + 1); On 2016/07/05 14:20:19, mmoroz wrote: > 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. Done.
https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:58: } On 2016/07/05 15:04:40, katrielc wrote: > On 2016/07/05 14:20:19, mmoroz wrote: > > Missing 'default' case: > > 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. Good point! I misread that, sorry. https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:43: unsigned char key[30] = {0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, 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? https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:136: unsigned char raw_packet[packet_size]; 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); https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:138: srtp_unprotect(session, &reinterpret_cast<rtp_msg_t*>(raw_packet)->header, 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?
https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:43: unsigned char key[30] = {0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, On 2016/07/05 16:06:21, mmoroz wrote: > 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? Absolutely, yes :) https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:136: unsigned char raw_packet[packet_size]; On 2016/07/05 16:06:21, mmoroz wrote: > Looks like variable-length arrays are not allowed: Haha, I should have read the style guide better! libsrtp is written in plain C so I keep switching between the two :$ :) https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:138: srtp_unprotect(session, &reinterpret_cast<rtp_msg_t*>(raw_packet)->header, On 2016/07/05 16:06:21, mmoroz wrote: > 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, 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.)
https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:138: srtp_unprotect(session, &reinterpret_cast<rtp_msg_t*>(raw_packet)->header, On 2016/07/05 16:26:03, katrielc wrote: > On 2016/07/05 16:06:21, mmoroz wrote: > > 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, > > 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.) 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).
https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:138: srtp_unprotect(session, &reinterpret_cast<rtp_msg_t*>(raw_packet)->header, On 2016/07/05 16:40:10, mmoroz wrote: > On 2016/07/05 16:26:03, katrielc wrote: > > On 2016/07/05 16:06:21, mmoroz wrote: > > > 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, > > > > 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.) > > 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). What do you think of this? I think it's important to catch OOB reads even if using a long global array for messages.
https://codereview.chromium.org/2123553002/diff/100001/testing/libfuzzer/fuzz... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/100001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:161: size_t body_size = packet_size - std::min(packet_size, header_size); I think we can use packet_size - header_size. It will be >= 0. https://codereview.chromium.org/2123553002/diff/100001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:163: unpoison(&env->message.header, header_size); Manual poisoning is not the best approach, but I agree that we would like to catch OOB to right in the message body. Though it will not be OOB actually, it may be use-of-uninitialized value -- this is what MSan for. Given that, my suggestion for now is: 1) declare |rtp_msg_t message| object inside the loop 2) do not initialize it with zeroes 3) copy |header_size| bytes into |message.header| 4) copy |body_size| bytes into |message.body| That allows us: A) have a valid object in memory B) pass packets of different size C) detect usage of last bytes of the message body (bytes from body_size to RTP_MMAX_BUF_LEN) with MSan Also this will make code a bit simpler. https://codereview.chromium.org/2123553002/diff/100001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:171: std::vector<unsigned char> packet(data, data + packet_size); For what do we need the same data in both env-> message and packet? Looks like env->message is not used anywhere.
Thanks for the detailed reviewing! https://codereview.chromium.org/2123553002/diff/100001/testing/libfuzzer/fuzz... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/100001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:161: size_t body_size = packet_size - std::min(packet_size, header_size); On 2016/07/06 09:44:22, mmoroz wrote: > I think we can use packet_size - header_size. It will be >= 0. Haha, I stared at this code for five minutes and still didn't notice that. Good point. https://codereview.chromium.org/2123553002/diff/100001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:163: unpoison(&env->message.header, header_size); On 2016/07/06 09:44:22, mmoroz wrote: > Manual poisoning is not the best approach, but I agree that we would like to > catch OOB to right in the message body. Though it will not be OOB actually, it > may be use-of-uninitialized value -- this is what MSan for. > > Given that, my suggestion for now is: > 1) declare |rtp_msg_t message| object inside the loop > 2) do not initialize it with zeroes > 3) copy |header_size| bytes into |message.header| > 4) copy |body_size| bytes into |message.body| > > That allows us: > A) have a valid object in memory > B) pass packets of different size > C) detect usage of last bytes of the message body (bytes from body_size to > RTP_MMAX_BUF_LEN) with MSan > > Also this will make code a bit simpler. Done. https://codereview.chromium.org/2123553002/diff/100001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:171: std::vector<unsigned char> packet(data, data + packet_size); On 2016/07/06 09:44:22, mmoroz wrote: > For what do we need the same data in both env-> message and packet? Looks like > env->message is not used anywhere. Oops. Fixed.
LGTM with a couple of comments, feel to land after addressing them. https://codereview.chromium.org/2123553002/diff/120001/testing/libfuzzer/fuzz... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/120001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:93: return 0; May be let's return |size - 1| here and use all the bytes left in |data|? https://codereview.chromium.org/2123553002/diff/120001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:119: Comment from previous patchset (lines 152-153) makes sense, could you please restore it. https://codereview.chromium.org/2123553002/diff/120001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:130: // any reads beyond the provided data. Actually MSan catches not simple reads, but usage of those uninitialized bytes, just a FYI. Catching of simple reads produce too many false positives and doesn't make much sense.
Will land this in a moment. https://codereview.chromium.org/2123553002/diff/120001/testing/libfuzzer/fuzz... File testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc (right): https://codereview.chromium.org/2123553002/diff/120001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:93: return 0; On 2016/07/06 12:13:14, mmoroz wrote: > May be let's return |size - 1| here and use all the bytes left in |data|? Done. https://codereview.chromium.org/2123553002/diff/120001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:119: On 2016/07/06 12:13:14, mmoroz wrote: > Comment from previous patchset (lines 152-153) makes sense, could you please > restore it. Done. https://codereview.chromium.org/2123553002/diff/120001/testing/libfuzzer/fuzz... testing/libfuzzer/fuzzers/libsrtp_fuzzer.cc:130: // any reads beyond the provided data. On 2016/07/06 12:13:14, mmoroz wrote: > Actually MSan catches not simple reads, but usage of those uninitialized bytes, > just a FYI. Catching of simple reads produce too many false positives and > doesn't make much sense. Acknowledged.
The CQ bit was checked by katrielc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmoroz@chromium.org Link to the patchset: https://codereview.chromium.org/2123553002/#ps140001 (title: "Don't waste bytes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add a fuzzer for srtp_unprotect in libsrtp. This is called on any SRTP packet received, and decrypts it in place. For now the fuzzer uses a hard-coded key and SSRC, but future modifications could generalise this in a number of ways. It does read more than one packet, since there could be bugs lurking in the state maintained by the parser (for example, the ID of each packet is stored for replay protection). ========== to ========== Add a fuzzer for srtp_unprotect in libsrtp. This is called on any SRTP packet received, and decrypts it in place. For now the fuzzer uses a hard-coded key and SSRC, but future modifications could generalise this in a number of ways. It does read more than one packet, since there could be bugs lurking in the state maintained by the parser (for example, the ID of each packet is stored for replay protection). Committed: https://crrev.com/346b92100d3ebfe43ec8c226d8bf0fa226c22527 Cr-Commit-Position: refs/heads/master@{#403893} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/346b92100d3ebfe43ec8c226d8bf0fa226c22527 Cr-Commit-Position: refs/heads/master@{#403893} |
