Potential buffer overflow in srtp_protect() #24

Closed
jfigus opened this Issue May 28, 2013 · 15 comments

Comments

Projects
None yet
5 participants
Contributor

jfigus commented May 28, 2013

Applications invoking srtp_protect() need to ensure the write buffer has additional memory available at the end to hold the authentication tag. The documentation should be updated to warn users of this potential problem.

jfigus was assigned May 28, 2013

Contributor

jfigus commented May 30, 2013

Please credit Fernando Russ from Groundworkstech for finding this vulnerability. His contact information is fruss@groundworkstech.com

Contributor

tvsriram commented May 30, 2013

Do we already have an existing patch for this?

On Thu, May 30, 2013 at 9:48 AM, John Foley notifications@github.comwrote:

Please credit Fernando Russ from Groundworkstech for finding this
vulnerability. His contact information is fruss@groundworkstech.com


Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/issues/24#issuecomment-18693101
.

Contributor

jfigus commented May 30, 2013

I'm not aware of anyone providing a patch. I've fixed this and submitted a pull request.

Contributor

tvsriram commented May 30, 2013

Awesome. I was still catching up on my mails.

Sriram

On Thu, May 30, 2013 at 9:57 AM, John Foley notifications@github.comwrote:

I'm not aware of anyone providing a patch. I've fixed this and submitted a
pull request.


Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/issues/24#issuecomment-18693778
.

Contributor

jfigus commented May 30, 2013

Looking at the following code, is this a bug as well? Notice the 32 bit tag cases are using the 80 bit policy.

err_status_t
crypto_policy_set_from_profile_for_rtcp(crypto_policy_t *policy,
srtp_profile_t profile) {

/* set SRTP policy from the SRTP profile in the key set /
switch(profile) {
case srtp_profile_aes128_cm_sha1_80:
crypto_policy_set_aes_cm_128_hmac_sha1_80(policy);
break;
case srtp_profile_aes128_cm_sha1_32:
crypto_policy_set_aes_cm_128_hmac_sha1_80(policy);
break;
case srtp_profile_null_sha1_80:
crypto_policy_set_null_cipher_hmac_sha1_80(policy);
break;
case srtp_profile_aes256_cm_sha1_80:
crypto_policy_set_aes_cm_256_hmac_sha1_80(policy);
break;
case srtp_profile_aes256_cm_sha1_32:
crypto_policy_set_aes_cm_256_hmac_sha1_80(policy);
break;
/
the following profiles are not (yet) supported */
case srtp_profile_null_sha1_32:
default:
return err_status_bad_param;
}

return err_status_ok;
}

Contributor

tvsriram commented May 30, 2013

yes looks like it is a problem too.

On Thu, May 30, 2013 at 10:03 AM, John Foley notifications@github.comwrote:

Looking at the following code, is this a bug as well? Notice the 32 bit
tag cases are using the 80 bit policy.

err_status_t
crypto_policy_set_from_profile_for_rtcp(crypto_policy_t *policy,
srtp_profile_t profile) {

/* set SRTP policy from the SRTP profile in the key set /
switch(profile) {
case srtp_profile_aes128_cm_sha1_80:
crypto_policy_set_aes_cm_128_hmac_sha1_80(policy);
break;
case srtp_profile_aes128_cm_sha1_32:
crypto_policy_set_aes_cm_128_hmac_sha1_80(policy);
break;
case srtp_profile_null_sha1_80:
crypto_policy_set_null_cipher_hmac_sha1_80(policy);
break;
case srtp_profile_aes256_cm_sha1_80:
crypto_policy_set_aes_cm_256_hmac_sha1_80(policy);
break;
case srtp_profile_aes256_cm_sha1_32:
crypto_policy_set_aes_cm_256_hmac_sha1_80(policy);
break;
/
the following profiles are not (yet) supported */
case srtp_profile_null_sha1_32:
default:
return err_status_bad_param;
}

return err_status_ok;
}


Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/issues/24#issuecomment-18694222
.

Hi John,

From: John Foley <notifications@github.commailto:notifications@github.com>
Reply-To: cisco/libsrtp <reply@reply.github.commailto:reply@reply.github.com>
Date: Thursday, May 30, 2013 1:03 PM
To: cisco/libsrtp <libsrtp@noreply.github.commailto:libsrtp@noreply.github.com>
Subject: Re: [libsrtp] Potential buffer overflow in srtp_protect() (#24)

Looking at the following code, is this a bug as well? Notice the 32 bit tag cases are using the 80 bit policy.

err_status_t
crypto_policy_set_from_profile_for_rtcp(crypto_policy_t *policy,
srtp_profile_t profile) {

/* set SRTP policy from the SRTP profile in the key set /
switch(profile) {
case srtp_profile_aes128_cm_sha1_80:
crypto_policy_set_aes_cm_128_hmac_sha1_80(policy);
break;
case srtp_profile_aes128_cm_sha1_32:
crypto_policy_set_aes_cm_128_hmac_sha1_80(policy);

Yes, this looks like a bug.

As an aside, the srtp_profile was originally meant to implement the profiles defined in Section 4.1.2 of rfc 5764 and the corresponding IANA registry http://www.iana.org/assignments/srtp-protection/srtp-protection.xml Only these profiles are defined in that standard:

SRTP_AES128_CM_HMAC_SHA1_80
SRTP_AES128_CM_HMAC_SHA1_32
SRTP_NULL_HMAC_SHA1_80
SRTP_NULL_HMAC_SHA1_32

So srtp_profile_aes256_cm_sha1_80 and srtp_profile_aes256_cm_sha1_32 apparently need to have a specification written for them and sent to IANA, so that they can be added to the registry. (Though what they are is apparent enough from the names :-)

David

break;
case srtp_profile_null_sha1_80:
crypto_policy_set_null_cipher_hmac_sha1_80(policy);
break;
case srtp_profile_aes256_cm_sha1_80:
crypto_policy_set_aes_cm_256_hmac_sha1_80(policy);
break;
case srtp_profile_aes256_cm_sha1_32:
crypto_policy_set_aes_cm_256_hmac_sha1_80(policy);
break;
/ the following profiles are not (yet) supported */
case srtp_profile_null_sha1_32:
default:
return err_status_bad_param;
}

return err_status_ok;
}


Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/issues/24#issuecomment-18694222.

Contributor

jfigus commented May 30, 2013

I'll fix this problem as well and include it in the pull request.

Thanks John.

David

From: John Foley <notifications@github.commailto:notifications@github.com>
Reply-To: cisco/libsrtp <reply@reply.github.commailto:reply@reply.github.com>
Date: Thursday, May 30, 2013 1:31 PM
To: cisco/libsrtp <libsrtp@noreply.github.commailto:libsrtp@noreply.github.com>
Cc: David McGrew <mcgrew@cisco.commailto:mcgrew@cisco.com>
Subject: Re: [libsrtp] Potential buffer overflow in srtp_protect() (#24)

I'll fix this problem as well and include it in the pull request.


Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/issues/24#issuecomment-18696129.

Contributor

tvsriram commented May 30, 2013

Thanks John.

Sriram

On Thu, May 30, 2013 at 10:31 AM, John Foley notifications@github.comwrote:

I'll fix this problem as well and include it in the pull request.


Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/issues/24#issuecomment-18696129
.

Contributor

jesup commented May 30, 2013

From RFC 3711:

. SRTCP MUST NOT be used with weak (or NULL) authentication.

SRTP MAY be used with weak authentication (e.g., a 32-bit
authentication tag), or with no authentication (the NULL
authentication algorithm).

I believe the code as stated is correct: 32-bit SRTP tags means using 80 bit RTCP tags. The MUST NOT says 32-bit auth tags for RTCP as not to be allowed. I've run into this before.

Hi Randell,

Very good observation; essentially this comes down to ignoring the "32" in crypto_policy_set_from_profile_for_rtcp. It would be great to add a comment in the code pointing this out. Apologies for not catching this earlier today.

David

From: jesup <notifications@github.commailto:notifications@github.com>
Reply-To: cisco/libsrtp <reply@reply.github.commailto:reply@reply.github.com>
Date: Thursday, May 30, 2013 2:38 PM
To: cisco/libsrtp <libsrtp@noreply.github.commailto:libsrtp@noreply.github.com>
Cc: David McGrew <mcgrew@cisco.commailto:mcgrew@cisco.com>
Subject: Re: [libsrtp] Potential buffer overflow in srtp_protect() (#24)

From RFC 3711:

. SRTCP MUST NOT be used with weak (or NULL) authentication.

SRTP MAY be used with weak authentication (e.g., a 32-bit
authentication tag), or with no authentication (the NULL
authentication algorithm).

I believe the code as stated is correct: 32-bit SRTP tags means using 80 bit RTCP tags. The MUST NOT says 32-bit auth tags for RTCP as not to be allowed. I've run into this before.


Reply to this email directly or view it on GitHubhttps://github.com/cisco/libsrtp/issues/24#issuecomment-18699351.

Contributor

jfigus commented May 30, 2013

OK, I'll back out this change and add the comments. Github seems to be
flaky today, it's been unresponsive at times. I'll fix this tomorrow.

On 05/30/2013 02:57 PM, davidmcgrew wrote:

Hi Randell,

Very good observation; essentially this comes down to ignoring the
"32" in crypto_policy_set_from_profile_for_rtcp. It would be great to
add a comment in the code pointing this out. Apologies for not
catching this earlier today.

David

From: jesup <notifications@github.commailto:notifications@github.com>
Reply-To: cisco/libsrtp
<reply@reply.github.commailto:reply@reply.github.com>
Date: Thursday, May 30, 2013 2:38 PM
To: cisco/libsrtp
<libsrtp@noreply.github.commailto:libsrtp@noreply.github.com>
Cc: David McGrew <mcgrew@cisco.commailto:mcgrew@cisco.com>
Subject: Re: [libsrtp] Potential buffer overflow in srtp_protect() (#24)

From RFC 3711:

. SRTCP MUST NOT be used with weak (or NULL) authentication.

SRTP MAY be used with weak authentication (e.g., a 32-bit
authentication tag), or with no authentication (the NULL
authentication algorithm).

I believe the code as stated is correct: 32-bit SRTP tags means using
80 bit RTCP tags. The MUST NOT says 32-bit auth tags for RTCP as not
to be allowed. I've run into this before.


Reply to this email directly or view it on
GitHubhttps://github.com/cisco/libsrtp/issues/24#issuecomment-18699351.


Reply to this email directly or view it on GitHub
#24 (comment).

fruss commented Jun 4, 2013

Hi, my name is Fernando Russ, and I'm a Security Researcher at Groundworks Technologies, as follow up of this issue we have published a related security advisory. Thanks.

http://seclists.org/fulldisclosure/2013/Jun/10

Some additional information to help people understand whether or not
their use of libSRTP might hit this bug:

The buffer overflow would only occur if the function
crypto_policy_set_from_profile_for_rtp() is called directly by the user
(it is not called from any other function in libSRTP). That function
implements the SRTPprofile used in DTLS-SRTP, so if you do not implement
that particular key management method, then you should not be using that
function anyway. If you have implemented DTLS-SRTP, then you should
check to see if you are using this particular function.

David

On Tue, 2013-06-04 at 05:52 -0700, Fernando Russ wrote:

Hi, my name is Fernando Russ, and I'm a Security Researcher at
Groundworks Technologies, as follow up of this issue we have published
a related security advisory. Thanks.

http://seclists.org/fulldisclosure/2013/Jun/10


Reply to this email directly or view it on GitHub.

jfigus closed this Jul 23, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment