1216837 - (CVE-2016-1970) Underflow in srtp_unprotect could cause memory-safety bug

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[q1]

srtp_unprotect (netwerk\srtp\src\srtp\srtp.c) can experience an integer underflow. If it does, it calls a decryption function with a buffer pointer pointing to memory to which it has no right, and with a very large buffer length. This call could scramble large portions of memory, causing incorrect and possibly insecure behavior.

It is unclear whether the bug can be invoked, because that depends upon behavior of a decryption function that I don't currently understand.

(There is also a similar bug in srtp_unprotect_rtcp).


Details
-------

The bug is in this code:

950: err_status_t
951: srtp_unprotect(srtp_ctx_t *ctx, void *srtp_hdr, int *pkt_octet_len) {
...
1073:   if (stream->rtp_services & sec_serv_conf) {
1074:     enc_start = (uint32_t *)hdr + uint32s_in_rtp_header + hdr->cc;  
1075:     if (hdr->x == 1) {
1076:       srtp_hdr_xtnd_t *xtn_hdr = (srtp_hdr_xtnd_t *)enc_start;
1077:       enc_start += (ntohs(xtn_hdr->length) + 1);
1078:     }  
1079:     enc_octet_len = (uint32_t)(*pkt_octet_len - tag_len 
1080: 			       - ((enc_start - (uint32_t *)hdr) << 2));
1081:   } else {
1082:     enc_start = NULL;
1083:   }
...
1162:   if (enc_start) {
1163:     status = cipher_decrypt(stream->rtp_cipher, 
1164: 			    (uint8_t *)enc_start, &enc_octet_len);
1165:     if (status)
1166:       return err_status_cipher_fail;
1167:   }

which does not check whether |hdr->cc| (line 1074) and |xtn_hdr->length| (line 1077) are such that any portion of the region described by |enc_start| and |enc_octet_len| lies outside of the packet that begins at |hdr| and extends for |*pkt_octet_len| bytes.

When using Firefox Hello, |cipher_decrypt| devolves to |aes_icm_encrypt_ismacryp| (netwerk\srtp\src\crypto\cipher\aes_icm.c), which checks the decryption buffer length thusly:

344: err_status_t
345: aes_icm_encrypt_ismacryp(aes_icm_ctx_t *c,
346:               unsigned char *buf, unsigned int *enc_len, 
347:               int forIsmacryp) {
348:   unsigned int bytes_to_encr = *enc_len;
349:   unsigned int i;
350:   uint32_t *b;
351:
352:   /* check that there's enough segment left but not for ismacryp*/
353:   if (!forIsmacryp && (bytes_to_encr + htons(c->counter.v16[7])) > 0xffff)
354:     return err_status_terminus;
...

with |forIsmacryp| == false. I don't understand how |c->counter.v16[7]| is used. If it can ever be >= 0x0400 (>= 4 when decoded), then an attacking peer can send a malformed packet that yields an |*enc_len| <= 0xfffffffc, which will pass the test on line 353 and cause aes_icm_encrypt_ismacryp to scramble |*enc_len| bytes of memory.

|c->counter.v16[7]| has never been nonzero in my testing using Firefox Hello.

Comment 1

[Andrew McCreight [:mccr8]]

Randell, it looks like you are the last person to touch this code, by landing it from somewhere else in 2012. Could you take a look? Thanks.

Comment 2

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

This is libsrtp (upstream); I'll check into this.  I'll verify if this can actually fail or not.  My suspicion is that it can't, but I'll have to go over the code carefully to verify.  (I'm also an author/peer for libsrtp, though I haven't done anything there recently other than monitor it.)

Moving to Unconfirmed as it's not clear if there's really a bug here yet, per comment 0.

Thanks for the report!  I'll also check it against latest upstream source.  libsrtp has been very stable, overall, though there is some ongoing work (largely cleanup and modernization).

Comment 3

[q1]

> Thanks for the report!

You're welcome. Here is one way to manifest the bug. RtpHeaderParser::Parse (media\webrtc\trunk\webrtc\modules\rtp_rtcp\source\rtp_utility.cc) can overflow while parsing the packet extension:

378:  if (X) {
379:    /* RTP header extension, RFC 3550.
370:     0                   1                   2                   3
381:     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
382:    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
383:    |      defined by profile       |           length              |
384:    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
385:    |                        header extension                       |
386:    |                             ....                              |
387:    */
388:    const ptrdiff_t remain = _ptrRTPDataEnd - ptr;
389:    if (remain < 4) {
390:      return false;
391:    }
392:
393:    header.headerLength += 4;
394:
395:    uint16_t definedByProfile = *ptr++ << 8;
396:    definedByProfile += *ptr++;
397:
398:    uint16_t XLen = *ptr++ << 8;
399:    XLen += *ptr++; // in 32 bit words
400:    XLen *= 4; // in octs
401:
402:    if (remain < (4 + XLen)) {
403:      return false;
404:    }
405:    if (definedByProfile == kRtpOneByteHeaderExtensionId) {
406:      const uint8_t* ptrRTPDataExtensionEnd = ptr + XLen;
407:      ParseOneByteExtensionHeader(header,
408:                                  ptrExtensionMap,
409:                                  ptrRTPDataExtensionEnd,
410:                                  ptr);
411:    }
412:    header.headerLength += XLen;
413:  }
414:  return true;
415:}

This can occur if the 2 length bytes fetched in lines 398-99 total >= 0x4000. Choosing exactly 0x4000, line 400 then computes 0. This passes the test on line 402, has no impact on lines 405-11, |leaves header.headerLength| on line 412 unchanged, and causes success status to be returned. (Should I file this as a separate bug?)

Later, when srtp_unprotect is called, it blithely fetches the same length bytes from the packet on line 1077 (above), adds 1 (computing the value 0x4001), then adds them to |enc_start|. This then causes an underflow in the computation of |enc_octet_len| on lines 1079-80.

You can verify this scenario by modifying the extension header length in the debugger when control is at the beginning of MediaPipeline::PacketReceived (before any parsing) and then stepping the relevant code. You have to skip the auth checking in srtp_unprotect lines 1139-40 because you've modified the packet. However, an attacking peer would simply send the nasty packet with the correct auth data, so this doesn't invalidate the test case. Also maybe it's possible for |sec_serv_auth| to be disabled?

Comment 4

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

(In reply to q1 from comment #0)
> srtp_unprotect (netwerk\srtp\src\srtp\srtp.c) can experience an integer
> underflow. If it does, it calls a decryption function with a buffer pointer
> pointing to memory to which it has no right, and with a very large buffer
> length. This call could scramble large portions of memory, causing incorrect
> and possibly insecure behavior.
> 
> It is unclear whether the bug can be invoked, because that depends upon
> behavior of a decryption function that I don't currently understand.
> 
> (There is also a similar bug in srtp_unprotect_rtcp).
> 
> 
> Details
> -------
> 
> The bug is in this code:
> 
> 950: err_status_t
> 951: srtp_unprotect(srtp_ctx_t *ctx, void *srtp_hdr, int *pkt_octet_len) {
> ...
> 1073:   if (stream->rtp_services & sec_serv_conf) {
> 1074:     enc_start = (uint32_t *)hdr + uint32s_in_rtp_header + hdr->cc;  
> 1075:     if (hdr->x == 1) {
> 1076:       srtp_hdr_xtnd_t *xtn_hdr = (srtp_hdr_xtnd_t *)enc_start;
> 1077:       enc_start += (ntohs(xtn_hdr->length) + 1);
> 1078:     }  
> 1079:     enc_octet_len = (uint32_t)(*pkt_octet_len - tag_len 
> 1080: 			       - ((enc_start - (uint32_t *)hdr) << 2));
> 1081:   } else {
> 1082:     enc_start = NULL;
> 1083:   }
> ...
> 1162:   if (enc_start) {
> 1163:     status = cipher_decrypt(stream->rtp_cipher, 
> 1164: 			    (uint8_t *)enc_start, &enc_octet_len);
> 1165:     if (status)
> 1166:       return err_status_cipher_fail;
> 1167:   }
> 
> which does not check whether |hdr->cc| (line 1074) and |xtn_hdr->length|
> (line 1077) are such that any portion of the region described by |enc_start|
> and |enc_octet_len| lies outside of the packet that begins at |hdr| and
> extends for |*pkt_octet_len| bytes.

hdr->cc and xtn_hdr->length are both unsigned (:4 and uint16_t respectively).
Clearly this could push enc_start into the packet, or past the end of the packet, since they're not checked.

Then enc_octet_len is calculated by dropping the tag len from the count, and then calculating how many 32-bit words enc_start is after hdr, and converting to a byte count.  Then it's subtracted from *pkt_octet_len.  If that's past the end of the packet, these subtractions will underflow (it's a uint32_t), yielding a very large value.

As discussed, this will run decryption on memory past the end of the packet.  Auth will be run, but auth is not run using enc_start or enc_octet_len; it uses hdr and tag_len and pkt_octet_len, so doing this won't fail auth.
Running decryption over this memory will (effectively) scramble it; it's *close* to the equivalent of writing random values into that memory.  There's still the issue of getting counter.v16[7] to be non-zero... so see below.

Upstream code has already resolved this by adding:
    if (!((uint8_t*)enc_start < (uint8_t*)hdr + *pkt_octet_len))
      return srtp_err_status_parse_err;

There's a similar (though not exactly the same) issue in RTCP handling, where a check was added upstream for negative lengths/etc

> 
> When using Firefox Hello, |cipher_decrypt| devolves to
> |aes_icm_encrypt_ismacryp| (netwerk\srtp\src\crypto\cipher\aes_icm.c), which
> checks the decryption buffer length thusly:
> 
> 344: err_status_t
> 345: aes_icm_encrypt_ismacryp(aes_icm_ctx_t *c,
> 346:               unsigned char *buf, unsigned int *enc_len, 
> 347:               int forIsmacryp) {
> 348:   unsigned int bytes_to_encr = *enc_len;
> 349:   unsigned int i;
> 350:   uint32_t *b;
> 351:
> 352:   /* check that there's enough segment left but not for ismacryp*/
> 353:   if (!forIsmacryp && (bytes_to_encr + htons(c->counter.v16[7])) >
> 0xffff)
> 354:     return err_status_terminus;
> ...
> 
> with |forIsmacryp| == false. I don't understand how |c->counter.v16[7]| is
> used. If it can ever be >= 0x0400 (>= 4 when decoded), then an attacking
> peer can send a malformed packet that yields an |*enc_len| <= 0xfffffffc,
> which will pass the test on line 353 and cause aes_icm_encrypt_ismacryp to
> scramble |*enc_len| bytes of memory.
> 
> |c->counter.v16[7]| has never been nonzero in my testing using Firefox Hello.

I believe the input to aes_icm_encrypt_ismacryp() of c->counter.v16[7] is *always* zero in our usage in aes_icm in libsrtp - before encrypting/decrypting each packet, it calls cipher_set_iv() -> aes_icm_set_iv() with a nonce which has 0's in those 16 bits always (built from est<<16 - and est is a 16-bit sequence number (est = (xtd_seq_num_t) ntohs(hdr->seq);)/  aes_icm_set_iv xors the context's offset value with the nonce to generate the new counter value.  The offset is created with this comment:   /* force last two octets of the offset to be left zero (for srtp compatibility) */, and it only sets offset.v8[0-13], leaving [14] and [15] 0.  Thus, set_iv always leaves counter.v16[7] == 0, so it's always 0 at entry to that routine.  v16[7] does get incremented within the routine, but it's reset by cipher_set_iv() before re-entering.

Thus this: "If it can ever be >= 0x0400 (>= 4 when decoded)" can't happen.

Ugh, what a pain tracing this though.  And I can see why they added a clear failure mode/return for this sort of error to upstream.

Comment 5

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

Just to clarify:

> Thus this: "If it can ever be >= 0x0400 (>= 4 when decoded)" can't happen.

That's true... 

> with |forIsmacryp| == false. I don't understand how |c->counter.v16[7]| is
> used. If it can ever be >= 0x0400 (>= 4 when decoded), then an attacking
> peer can send a malformed packet that yields an |*enc_len| <= 0xfffffffc,
> which will pass the test on line 353 and cause aes_icm_encrypt_ismacryp to
> scramble |*enc_len| bytes of memory.

The code is: if (!forIsmacryp && (bytes_to_encr + htons(c->counter.v16[7])) > 0xffff) 
return an error.  So it will return an error if a) forIsmcryp is 0, and b) bytes_to_encr+0 > 0xffff.
a) is true, and b will be true on any overflow where we wrap around, unless we go so far around that we're under 0xffff.  To do that, enc_start would have to be almost 4G *past* hdr, which can't be done with the 16-bits-of-32-bit-words header extension length.

I believe this is the (unstated) reason why the quoted text says it's a bug if v16[7] can be large enough 0x4, since that could let a small overflow sneak past this check.

Comment 6

[q1]

(In reply to Randell Jesup [:jesup] from comment #4)
...

> Upstream code has already resolved this by adding:
>    if (!((uint8_t*)enc_start < (uint8_t*)hdr + *pkt_octet_len))
>      return srtp_err_status_parse_err;

That's not good enough. It also has to check whether |enc_start + enc_octet_len < (uint8_t*)hdr + *pkt_octet_len)|. Try doctoring the length bytes to yield a length of 5 in the debugger. With | *pkt_octet_len == 0x27| (a typical value), |enc_start| ends up == |hdr + *pkt_octet_len - 3|, so it passes the new upstream test. However, |enc_octet_len| ends up == 0x27 - 0xa - (9 << 2) == 0xfffffff9.

> Ugh, what a pain tracing this though.

Thank you for doing so.

> And I can see why they added a clear
> failure mode/return for this sort of error to upstream.

Yes. As much as reasonable within well-defined constraints (e.g., performance), code should protect itself from insecure behavior. Along those lines, I will file the bug in RtpHeaderParser::Parse that I noted in comment 3 separately.

Comment 7

[q1]

Oops, |enc_start + enc_octet_len < (uint8_t*)hdr + *pkt_octet_len)| should read |(uint8 *)enc_start + enc_octet_len < (uint8_t*)hdr + *pkt_octet_len)|. The point is still the same, however.

Comment 8

[q1]

(In reply to q1 from comment #6)
> I will file the bug in RtpHeaderParser::Parse that I noted in comment
> 3 separately.

https://bugzilla.mozilla.org/show_bug.cgi?id=1219814

Comment 9

[q1]

srtp_unprotect also should check whether |*pkt_octet_len| is sufficiently long to accommodate an auth string of length |tag_len|. At present this is coincidentally covered by the check of |*pkt_octet_len| against |octets_in_rtp_header| because all of the crypto policies (see srtp.c) use <= 10 auth bytes, but if anyone uses more than 12 auth bytes (which appears to be allowed by RFC 3711), that check will no longer be sufficient.

Comment 10

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

(In reply to q1 from comment #9)
> srtp_unprotect also should check whether |*pkt_octet_len| is sufficiently
> long to accommodate an auth string of length |tag_len|. At present this is
> coincidentally covered by the check of |*pkt_octet_len| against
> |octets_in_rtp_header| because all of the crypto policies (see srtp.c) use
> <= 10 auth bytes, but if anyone uses more than 12 auth bytes (which appears
> to be allowed by RFC 3711), that check will no longer be sufficient.

If so, it would be a bug in upstream libsrtp, but not in our usage of it.  I'd suggest filing an issue on the tracker at github.com/cisco/libsrtp

Comment 11

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

[Tracking Requested - why for this release]:
sec bug, though very hard to exploit.

(In reply to q1 from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #4)
> ...
> 
> > Upstream code has already resolved this by adding:
> >    if (!((uint8_t*)enc_start < (uint8_t*)hdr + *pkt_octet_len))
> >      return srtp_err_status_parse_err;
> 
> That's not good enough. It also has to check whether |enc_start +
> enc_octet_len < (uint8_t*)hdr + *pkt_octet_len)|. Try doctoring the length
> bytes to yield a length of 5 in the debugger. With | *pkt_octet_len == 0x27|
> (a typical value), |enc_start| ends up == |hdr + *pkt_octet_len - 3|, so it
> passes the new upstream test. However, |enc_octet_len| ends up == 0x27 - 0xa
> - (9 << 2) == 0xfffffff9.

So if it passes the new check, hdr <= enc_start < hdr + *pkt_octet_len.  So there are 0 or more bytes following enc_start that are in the packet.  Then we calculate
    enc_octet_len = (uint32_t)(*pkt_octet_len - tag_len -
                               ((uint8_t*)enc_start - (uint8_t*)hdr));
so worst case (enc_start+1 == hdr + *pkt_octet_len, 0 bytes following enc_start in the packet): you would end up with -tag_len bytes - which is a negative value, which means underflow in uint32_t land.

So the upstream test is insufficient, but only because it doesn't take tag_len into account.  Change upstream to 
    if (!((uint8_t*)enc_start < (uint8_t*)hdr + (*pkt_octet_len - tag_len)))
      return err_status_parse_err;
and we're good - if enc_start is as large as possible to pass that, enc_octet_len will be 0.  It appears that aes_icm_encrypt_ismacryp() handles an input length of 0, though we could tighten it if it makes sense.   

There's another upstream bug (small) where if tag_len > length-of-packet it could in theory read before the start of the packet.  I think in practice this is impossible unless you create a crypto profile with a tag longer than an RTP header - and the worst case an attacker could do there would be to cause it to crash on a read.

> > Ugh, what a pain tracing this though.
> 
> Thank you for doing so.

No problem; thanks for finding it.  We'll coordinate our landing with an upstream patch for this (don't want to 0-day anyone, not that this would be very exploitable - mostly just unpredictably(?) scrambling some memory following the packet.)  The attacker can predict the keys/etc used by the decryption call, so it knows what operation will happen on the memory.  But it doesn't know what the values in that memory are (though it may be able to guess some, since it knows it's a fresh allocation), and the operation is an encrypt operation, so tends to be very randomizing.  Very (extremely!) hard to exploit beyond simple denial-of-service, though probably not *quite* impossible (very close though).

Also, underflowing the length by a small number bytes means a Very Large number of bytes will be randomized, not just a few.  (MAX_UNSIGNED minus a few).  This means a large amount of memory will be trashed, all the more likely leading to a fast random/unpredictable crash.

Based on that, I'm going to sec-moderate.  Also this analysis gives a reduced risk to other libsrtp users, though I still plan to coordinate a fix and landing with them.

Thanks!!

Comment 12

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

Attachment: add explicit error checks for packet length in srtp

Comment 13

[q1]

(In reply to Randell Jesup [:jesup] from comment #11)
> [Tracking Requested - why for this release]:
> sec bug, though very hard to exploit.
> 
> (In reply to q1 from comment #6)
> > (In reply to Randell Jesup [:jesup] from comment #4)
> > ...
> > 
> > > Upstream code has already resolved this by adding:
> > >    if (!((uint8_t*)enc_start < (uint8_t*)hdr + *pkt_octet_len))
> > >      return srtp_err_status_parse_err;
> > 
> > That's not good enough. It also has to check whether |enc_start +
> > enc_octet_len < (uint8_t*)hdr + *pkt_octet_len)|. Try doctoring the length
> > bytes to yield a length of 5 in the debugger. With | *pkt_octet_len == 0x27|
> > (a typical value), |enc_start| ends up == |hdr + *pkt_octet_len - 3|, so it
> > passes the new upstream test. However, |enc_octet_len| ends up == 0x27 - 0xa
> > - (9 << 2) == 0xfffffff9.
> ...
> So the upstream test is insufficient, but only because it doesn't take
> tag_len into account.  Change upstream to 
>     if (!((uint8_t*)enc_start < (uint8_t*)hdr + (*pkt_octet_len - tag_len)))
>       return err_status_parse_err;
> and we're good - if enc_start is as large as possible to pass that,
> enc_octet_len will be 0.  It appears that aes_icm_encrypt_ismacryp() handles
> an input length of 0, though we could tighten it if it makes sense.   

OK, that works, though only because enc_start can't overflow to < hdr, which is only because hdr->cc is 4 (unsigned) bits and xtn_hdr is 16 (unsigned) bits. I'd be more comfortable with a double-ended test, which guarantees an error if any portion of [enc_start, enc_start+enc_octet_len) lies outside [hdr, hdr+*pkt_octet_len); it's more future-proof.
 
> There's another upstream bug (small) where if tag_len > length-of-packet it
> could in theory read before the start of the packet.  I think in practice
> this is impossible unless you create a crypto profile with a tag longer than
> an RTP header - and the worst case an attacker could do there would be to
> cause it to crash on a read.

It wouldn't necessarily crash; it (meaning the auth code on line 1139) could read data from before the packet's beginning. It also would cause other code to malfunction (probably causing leaks and possibly out-of-bounds writing) because of the |*pkt_octet_len -= tag_len| on line 1221. But as you say, you need to create a nondefault crypto profile to make all that happen.

Comment 14

[q1]

By "leaks" in comment 13, I mean leakage of information from one session into another, not memory leaks.

Comment 15

[q1]

There are also similar bugs in srtp_unprotect_rtcp that I will file separately.

Comment 16

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

Fixed upstream in https://github.com/cisco/libsrtp/commit/2583150aa9729fccfdcd58e9811cc7ed161b1ccd
after I chatted with them via email.
They don't have much of a flow (ok, none) for handling sec issues; luckily there have been few.  I'll queue up an import of the 1.5 branch once this patch lands there.

Comment 17

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

Attachment: add explicit error checks for packet length in srtp

Patrick - can you review ASAP (it's simple)?  I could rs= based on cherry-picking upstream, but that seems wrong (as I effectively gave the fix to them).  Thanks

Comment 18

[q1]

Why isn't the check in srtp_protect identical to the one in srtc_unprotect?

Comment 19

[q1]

(In reply to q1 from comment #18)
> Why isn't the check in srtp_protect identical to the one in srtc_unprotect?

srtp_unprotect.

Comment 20

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

(In reply to q1 from comment #18)
> Why isn't the check in srtp_protect identical to the one in srtc_unprotect?

Because on srtp_protect, we're going to add the tag to the packet (after this point).  We've calculated what the tag_len will be, but it's not there yet, just the header and payload data.

Comment 21

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

We did (in upstream) move the check in srtp_protect() out of the "if (hdr->x == 1)" (it was already outside of it in srtp_protect_aead() IIRC).  In the code here, I simply added it to the right place.

Comment 22

[q1]

(In reply to Randell Jesup [:jesup] from comment #20)
> (In reply to q1 from comment #18)
> > Why isn't the check in srtp_protect identical to the one in srtc_unprotect?
> 
> Because on srtp_protect, we're going to add the tag to the packet (after
> this point).  We've calculated what the tag_len will be, but it's not there
> yet, just the header and payload data.

But how, then, do you know how much space is really available in the packet? I see that srtp_protect is called from SrtpFlow::ProtectRtp/ProtectRtcp, which doesn't pass it its |max_len| argument, so it appears that srpt_protect has no way to know how long its |rtp_hdr| buffer really is. Apparently the |max_len| limit is enforced only by ProtectR*'s |MOZ_ASSERT(len <= max_len);| and by a related assert in MediaPipeline::PipelineTransport::SendRtpRtcpPacket_s . That's rather fragile.

Comment 23

[Patrick McManus [:mcmanus]]

Comment on attachment 8683192 [details] [diff] [review]
add explicit error checks for packet length in srtp

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

as far as I understand this code the check makes sense to me, but this isn't code I'm familiar with so I don't know that it has a a lot of value.. and q1 poses an interesting question.

so I'll f+ it and delegate the review based on reaching consensus with the reporter.

Thanks for this!

Comment 24

[q1]

Comment on attachment 8683192 [details] [diff] [review]
add explicit error checks for packet length in srtp

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

(In reply to Patrick McManus [:mcmanus] from comment #23)
> Comment on attachment 8683192 [details] [diff] [review]
> add explicit error checks for packet length in srtp
> 
> Review of attachment 8683192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> as far as I understand this code the check makes sense to me, but this isn't
> code I'm familiar with so I don't know that it has a a lot of value.. and q1
> poses an interesting question.
> 
> so I'll f+ it and delegate the review based on reaching consensus with the
> reporter.
> 
> Thanks for this!

And thank you. I think the patch is OK as far as it goes. However, relying on asserts to enforce correctness always makes me uneasy, because you then have to test all the applicable code paths in the debug build. So I would tend to want ProtectRtp to pass its |max_len| argument to srtp_protect, which should then ensure that it doesn't add enough to the packet to exceed that many octets. On the other hand, every time you modify a line of code you can add a bug (or three). Since I'm really a newbie with this code, I'm going to bounce the ultimate decision back to Randell.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Too late for 42, we have time for 43.

Comment 26

[q1]

Comment on attachment 8683192 [details] [diff] [review]
add explicit error checks for packet length in srtp

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

Fobbing the review back to Mr. McManus. Maybe you can suggest someone who's more qualified in this code?

Comment 27

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

I'll note that the equivalent patch has landed upstream (though not in the 1.5 branch yet).  They decided to just patch it immediately without checking with me; there's no real sec policy in the upstream project.  While the concern level for us is low, upstream is concerned with servers using this code being DOSed (which wouldn't be hard).

Comment 28

[q1]

(In reply to Randell Jesup [:jesup] from comment #27)
> I'll note that the equivalent patch has landed upstream (though not in the
> 1.5 branch yet).  They decided to just patch it immediately without checking
> with me; [***]there's no real sec policy in the upstream project.[***]

In this day and age?

> While the
> concern level for us is low, upstream is concerned with servers using this
> code being DOSed (which wouldn't be hard).

Then Mozilla should patch it soon; I might not have found the best way to exploit it. As I said above, I think the patch is OK, but I am a total newbie in the code, so I'd be more comfortable with a qualified Mozillian looking it over.

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/35d0e5235621

Note upstream landed an equivalent patch (and a follow-up fix to their patch that doesn't affect us until we update to 1.5.x)

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35d0e5235621

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Since this didn't make it into 44 but is fixed in 45, let's uplift this when we build 38.7.0esr.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

Missed ESR 38.7 and is a sec-moderate.