737178 - Implement RFC5764 (DTLS-SRTP)

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Eric Rescorla (:ekr)]

Attachment: Patch

The following patch implements RFC 5764 (DTLS-SRTP) negotiation.

This has been through one round of review by Wan-Teh and I have
interop tested with OpenSSL.

Comment 1

[Eric Rescorla (:ekr)]

Sorry about this not being a CVS patch. It has a dependency on the DTLS patch and so I had to take it as a diff off of my DTLS patch (681065)

Comment 2

[Wan-Teh Chang]

Comment on attachment 607296 [details] [diff] [review]
Patch

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

I haven't reviewed ssl3_HandleUseSRTPXtn.  Overall this
looks good.  Most of my comments are coding style nits.

::: mozilla/security/nss/cmd/tstclnt/tstclnt.c
@@ +125,5 @@
> +    SRTP_AES128_CM_SHA1_32,                     /* B */
> +    SRTP_NULL_SHA1_80,                          /* C */
> +    SRTP_NULL_SHA1_32,                          /* D */
> +};
> +#define NUM_SRTP_CIPHER_SUITES (PR_ARRAY_SIZE(srtpCipherSuites))

Remove the outer parentheses.

@@ +182,5 @@
>  	cert = NULL;
>      }
> +    result = SSL_GetSRTPCipher(fd, &srtpCipher);
> +    if (result == SECSuccess) {
> +	fprintf(stderr, "SRTP ciphers negotiated: number = %d\n",

SRTP ciphers => SRTP cipher

@@ +250,5 @@
> +"A   SRTP_AES128_CM_SHA1_80\n"
> +"B   SRTP_AES128_CM_SHA1_32\n"
> +"C   SRTP_NULL_SHA1_80\n"
> +"D   SRTP_NULL_SHA1_32\n"
> +	    "\n");	    

Please align these five string literals with 'stderr'.

@@ +1036,5 @@
> +	    if (rv != SECSuccess) {
> +		SECU_PrintError(progName, "error enabling SRTP ciphers");
> +		return 1;
> +	    }
> +

Delete this blank line.

::: mozilla/security/nss/lib/ssl/ssl.h
@@ +736,5 @@
>                        struct SECKEYPrivateKeyStr **pRetKey);
>  
> +
> +/*
> +** Configure DTLS-SRTP (RFC 5764) preferences.

Add "cipher suite" before "preferences".

@@ +743,5 @@
> +** extension to be negotiated.
> +*/
> +SSL_IMPORT SECStatus SSL_SetSRTPCiphers(PRFileDesc *socket,
> +					const PRUint16 *ciphers,
> +					unsigned int num_ciphers);

Nit: num_ciphers => numCiphers

On line 752 below: SECFailed => SECFailure

::: mozilla/security/nss/lib/ssl/ssl3ext.c
@@ +84,5 @@
>  			PRUint16 ex_type, SECItem *data);
>  static PRInt32 ssl3_ClientSendNextProtoNegoXtn(sslSocket *ss, PRBool append,
>  					       PRUint32 maxBytes);
> +static SECStatus ssl3_SendUseSRTPXtn(sslSocket *ss, PRBool append, PRUint32 maxBytes);
> +SECStatus ssl3_HandleUseSRTPXtn(sslSocket * ss, PRUint16 ex_type, SECItem *data);

Line 87 seems longer than 80 characters.

I think ssl3_HandleUseSRTPXtn can be static.

@@ +1558,4 @@
>      int i;
>  
>      if (!sender) {
> +    	sender = (ss->version > SSL_LIBRARY_VERSION_3_0 || IS_DTLS(ss)) ?

I believe we don't need || IS_DTLS(ss).

@@ +1665,5 @@
> +
> +    if (!ss->sec.isServer) {
> +	/* Client */
> +
> +	if (!ss->ssl3.dtlsSRTPCipherCt)

The use_srtp extension must only be used with DTLS.
Should we also check IS_DTLS(ss), or is that check
implied by the !ss->ssl3.dtlsSRTPCipherCt check?

@@ +1776,5 @@
> +    /* Server side */
> +    if (!data->data || data->len < 5) {
> +	/* malformed */	
> +	return SECFailure;
> +    }

On the server side, I think we need to disallow
or ignore this extension unless the socket is in
DTLS mode.

::: mozilla/security/nss/lib/ssl/sslimpl.h
@@ +640,4 @@
>  	    int                   cipherType;
>  	    SECItem               cipherArg;
>  	    int                   keyBits;
> +															    int                   secretKeyBits;

Please undo this whitespace change.

@@ +918,5 @@
> +    /*
> +     * DTLS-SRTP cipher suite preferences (if any)
> +     */
> +    PRUint16         dtlsSRTPCiphers[MAX_DTLS_SRTP_CIPHER_SUITES];
> +    PRUint16         dtlsSRTPCipherCt;

dtlsSRTPCipherCt => dtlsSRTPCipherCount

Note: numDtlsSRTPCiphers is probably more in line
with the NSS naming convention, but we'd need to
deal with the awkward capitalization of "DtlsSRTP".

@@ +1826,4 @@
>  extern void dtls_RehandshakeCleanup(sslSocket *ss);
>  
>  
> +

Delete this blank line.

::: mozilla/security/nss/lib/ssl/sslsock.c
@@ +221,5 @@
>  #define LOCKSTATUS_OFFSET 10 /* offset of ENABLED */
>  
> +static PRUint16 srtpCiphers[] = {
> +    SRTP_AES128_CM_SHA1_80,
> +    SRTP_AES128_CM_SHA1_32,

Why are the two null encryption ciphers not in
this array?  I think you allow tstclnt.c to set
them using the letters C and D.

@@ +1581,5 @@
> +	PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +	return SECFailure;
> +    }
> +
> +    for (i=0; i<num_ciphers; i++) {

Add spaces around '=' and '<'.

@@ +1620,5 @@
> +	return SECFailure;
> +    }
> +
> +    if (!ss->ssl3.dtlsSRTPCipherSuite)
> +	return SECFailure;

Set an error code with PORT_SetError().  I don't
know what's the right error code here.  We can
use SEC_ERROR_INVALID_ARGS as a placeholder.

Comment 3

[Wan-Teh Chang]

I'm using http://codereview.chromium.org/9982019/ to review
Ekr's patch (attachment 607296 [details] [diff] [review]).

(In reply to Wan-Teh Chang from comment #2)
> Comment on attachment 607296 [details] [diff] [review]
>
> ::: mozilla/security/nss/lib/ssl/sslsock.c
> @@ +221,5 @@
> >  #define LOCKSTATUS_OFFSET 10 /* offset of ENABLED */
> >  
> > +static PRUint16 srtpCiphers[] = {
> > +    SRTP_AES128_CM_SHA1_80,
> > +    SRTP_AES128_CM_SHA1_32,
> 
> Why are the two null encryption ciphers not in
> this array?  I think you allow tstclnt.c to set
> them using the letters C and D.

Ekr suggested (in http://codereview.chromium.org/9982019/) that
we remove SRTP_NULL_SHA1_80 and SRTP_NULL_SHA1_32 from tstclnt.c.

Comment 4

[Wan-Teh Chang]

Attachment: Add SSL_SetSRTPCiphers and SSL_GetSRTPCipher

This is the main part of Ekr's patch (attachment 607296 [details] [diff] [review]).
It doesn't include the changes to tstclnt.c.  This patch was
reviewed at https://chromiumcodereview.appspot.com/9982019/.

The new functions are not yet exported in ssl.def because
they may change.  Specifically SSL_SetSRTPCiphers may be
replaced by a SSL_UseSRTP function (or a SSL_USE_SRTP
SSL socket option) and a SSL_SRTPCipherPrefSet function.
See http://code.google.com/p/chromium/issues/detail?id=120938#c4

Patch checked in on the NSS trunk (NSS 3.14).

Checking in ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.57; previous revision: 1.56
done
Checking in ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.24; previous revision: 1.23
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.105; previous revision: 1.104
done
Checking in sslproto.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslproto.h,v  <--  sslproto.h
new revision: 1.20; previous revision: 1.19
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.89; previous revision: 1.88
done
Checking in sslt.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v  <--  sslt.h
new revision: 1.23; previous revision: 1.22
done

Comment 5

[Wan-Teh Chang]

Attachment: Ekr's patch for tstclnt.c

This patch depends on the DTLS patch for tstclnt.c in bug 681065.

Comment 6

[Wan-Teh Chang]

Attachment: Fix compiler warnings about signed/unsigned comparisons

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 631258 [details] [diff] [review]
Fix compiler warnings about signed/unsigned comparisons

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

wan-teh, do you see a problem comparing a unsigned int to a PRUint16?

::: mozilla/security/nss/lib/ssl/ssl3ext.c
@@ +1689,5 @@
>  ssl3_HandleUseSRTPXtn(sslSocket * ss, PRUint16 ex_type, SECItem *data)
>  {
>      SECStatus rv;
>      SECItem ciphers = {siBuffer, NULL, 0};
> +    unsigned int i;

i is used as to index against dtlsSRTPCipherCount, which is a PRUint16. I think this is OK, but wanted to point it out.

Comment 8

[Wan-Teh Chang]

Attachment: Fix compiler warnings about signed/unsigned comparisons, v2

Ekr: good point. I think it should be fine to compare
an unsigned int with a PRUint16. I may have chosen an
unsigned int because it is more efficient, but I changed
it to a PRUint16 to match the type of dtlsSRTPCipherCount.

Comment 9

[Eric Rescorla (:ekr)]

Comment on attachment 663219 [details] [diff] [review]
Fix compiler warnings about signed/unsigned comparisons, v2

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

lgtm

Comment 10

[Wan-Teh Chang]

Comment on attachment 663219 [details] [diff] [review]
Fix compiler warnings about signed/unsigned comparisons, v2

Patch checked in on the NSS trunk (NSS 3.14).

Checking in ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.28; previous revision: 1.27
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.95; previous revision: 1.94
done

Comment 11

[Nils Ohlmeier [:drno]]

I believe we have this working and this could be closed, or am I missing something here?