866362 - SSL_SetStapledOCSPResponses needs an argument to specify which certificate the OCSP responses are for.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

An SSL server may have an RSA certificate and an ECC certificate.
So SSL_SetStapledOCSPResponses needs an argument to specify which
certificate (RSA or ECC) the OCSP responses are for.

More info:

SSL_ConfigSecureServer can be called multiple times to configure
an SSL server with muliple types of certificate. The certificate
type is specified by the "SSL3KEAType kea" argument:
http://mxr.mozilla.org/nss/ident?i=SSL_ConfigSecureServer

So, I think we should also add an "SSL3KEAType kea" argument to
SSL_SetStapledOCSPResponses.

I also suggest that we remove the "takeOwnership" argument. We
should decide whether the function should take ownership of the
OCSP responses. I think it should not take ownership, which matches
how selfserv is using the function (takeOwnership=PR_FALSE) and
matches what SSL_ConfigSecureServer does.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

If I understand correctly, Bob was worried about passing an array.
However, Wan-Teh has proposed to call the function multiple times, as needed.

Wan-Teh, do you think it should be done like this?

Note this patch has a TODO - I haven't yet researched how to do that detail. Do you know how to do it?

This patch builds - but is untested.

Comment 2

[Wan-Teh Chang]

Comment on attachment 744744 [details] [diff] [review]
Patch v1

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

This patch still needs work. I suggest some changes. Thanks.

::: cmd/selfserv/selfserv.c
@@ +1290,5 @@
>  	if (multiOcspResponses) {
> +	    SSLKEAType kea = kt_null;
> +	    if (certForStatusWeakReference) {
> +		kea = NSS_FindCertKEAType(certForStatusWeakReference);
> +	    }

If you search for "cert[kt_" in selfserv.c, you will see how the server
certs can be specified using command-line options. It seems that only
RSA cert (cert[kt_rsa]) and ECC cert (cert[kt_ecdh]) can be specified.

The stapled OCSP responses should be specified in a similar way.
You can use an array of SECItemArray* pointers to store all the
stapled OCSP responses specified on the command line, and then
add a SSL_SetStapledOCSPResponses call to this for loop:

    for (kea = kt_rsa; kea < kt_kea_size; kea++) {
        if (cert[kea] != NULL) {
            if (!certForStatusWeakReference)
                certForStatusWeakReference = cert[kea];

            secStatus = SSL_ConfigSecureServer(model_sock,
                        cert[kea], privKey[kea], kea);
            if (secStatus != SECSuccess)
                errExit("SSL_ConfigSecureServer");
        }
    }

::: lib/ssl/ssl.h
@@ +422,4 @@
>   */
>  SSL_IMPORT SECStatus
> +SSL_SetStapledOCSPResponses(PRFileDesc *fd, SSLKEAType kea,
> +			    const SECItemArray *responses);

I suggest matching SSL_ConfigSecureServer and making "SSLKEAType kea"
the last argument.

::: lib/ssl/ssl3con.c
@@ +8447,5 @@
>      SECStatus            rv;
>      int                  len 		= 0;
> +    int                  i;
> +    PRBool               haveStatus = PR_FALSE;
> +    SECItemArray        *statusToSend = NULL;

I think haveStatus can be omitted because statusToSend != NULL implies
haveStatus is true.

@@ +8458,5 @@
>  
>      if (!ssl3_ExtensionNegotiated(ss, ssl_cert_status_xtn))
>  	return SECSuccess;
>  
> +    for (i=kt_null; i < kt_kea_size; i++) {

Nit: add spaces around '='.

@@ +8461,5 @@
>  
> +    for (i=kt_null; i < kt_kea_size; i++) {
> +	if (ss->certStatusArray[i]) {
> +	    haveStatus = PR_TRUE;
> +	    

Move "haveStatus = PR_TRUE;" under the TODO comment because it is part
of the code that needs to be improved in the future.

This blank line has some spaces (visible in the Splinter Review) that
should be removed.

::: lib/ssl/ssl3ext.c
@@ +681,5 @@
>  {
>      PRInt32 extension_length;
>      SECStatus rv;
> +    int       i;
> +    PRBool    haveStatus = PR_FALSE;

No need to align the variable declarations in this function.

@@ +686,2 @@
>  
> +    for (i=kt_null; i < kt_kea_size; i++) {

Nit: add spaces around '='.

This code should be marked as a temporary workaround. The correct code needs
to see if we have an OCSP response for the server certificate being used,
rather than if we have any OCSP response.

::: lib/ssl/sslimpl.h
@@ +1175,5 @@
>      /* Configuration state for server sockets */
>      /* server cert and key for each KEA type */
>      sslServerCerts        serverCerts[kt_kea_size];
> +    /* each cert needs its own status */
> +    SECItemArray *        certStatusArray[kt_kea_size];

Yes, this is what I suggested. We can also consider adding a
certStatusArray member to the sslServerCerts structure.  This
should simplify most of the code in sslsock.c.

Comment 3

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #2)
> If you search for "cert[kt_" in selfserv.c, you will see how the server
> certs can be specified using command-line options. It seems that only
> RSA cert (cert[kt_rsa]) and ECC cert (cert[kt_ecdh]) can be specified.
> 
> The stapled OCSP responses should be specified in a similar way.
> You can use an array of SECItemArray* pointers to store all the
> stapled OCSP responses specified on the command line, 

Note they aren't specified on the command line.
selfserv creates them on its own.


> and then
> add a SSL_SetStapledOCSPResponses call to this for loop:
> 
>     for (kea = kt_rsa; kea < kt_kea_size; kea++) {
>         if (cert[kea] != NULL) {
>             if (!certForStatusWeakReference)
>                 certForStatusWeakReference = cert[kea];
> 
>             secStatus = SSL_ConfigSecureServer(model_sock,
>                         cert[kea], privKey[kea], kea);
>             if (secStatus != SECSuccess)
>                 errExit("SSL_ConfigSecureServer");
>         }
>     }

Your proposal would change selfserv to create the cert status response(s) only once. The current behaviour is to create a fresh response, each time the server accepts a new connection.

However, that change seems fine.

Nevertheless, if selfserv supports two "kea"s, then function handle_connection() must make the right decision, which stapled response it should use.


> I suggest matching SSL_ConfigSecureServer and making "SSLKEAType kea"
> the last argument.

ok


> I think haveStatus can be omitted because statusToSend != NULL implies
> haveStatus is true.

right


> This code should be marked as a temporary workaround. The correct code needs
> to see if we have an OCSP response for the server certificate being used,
> rather than if we have any OCSP response.

Agreed, cross-referencing comments added to both affected functions.

Comment 4

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #3)
> Nevertheless, if selfserv supports two "kea"s, then function
> handle_connection() must make the right decision, which stapled response it
> should use.

Never mind, you suggested that we create all responses once, and have handle_connection add both responses to the socket, and let the protocol code select the correct one (which will currently use our workaround to use the first one).

Comment 5

[Kai Engert (:KaiE:)]

Attachment: patch v2

this patch passes tests.

Comment 6

[Wan-Teh Chang]

Comment on attachment 744805 [details] [diff] [review]
patch v2

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

::: cmd/selfserv/selfserv.c
@@ +91,5 @@
>  		      use empty status if server is unavailable */
>  } ocspStaplingMode = osm_disabled;
>  typedef enum ocspStaplingModeEnum ocspStaplingModeType;
>  static char *ocspStaplingCA = NULL;
> +SECItemArray *certStatus[kt_kea_size] = { NULL };

I think this array should be added to the main() function, next to
these two arrays:

    CERTCertificate *    cert   [kt_kea_size] = { NULL };
    SECKEYPrivateKey *   privKey[kt_kea_size] = { NULL };

@@ +1107,5 @@
>  }
>  
>  SECItemArray *
>  makeSignedOCSPResponse(PRArenaPool *arena, ocspStaplingModeType osm,
> +		       secuPWData *pwdata, CERTCertificate *cert)

The wincx/pwarg argument is usually the last function argument by
convention. It would be nice to follow that convention in this
function and the setupCertStatus function.

@@ +1288,5 @@
>      } else {
>  	ssl_sock = tcp_sock;
>      }
>  
>      arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);

The |arena| variable in handle_connection() should be removed now.

@@ +1294,5 @@
>  	errExit("cannot allocate arena");
>  
> +    for (kea = kt_rsa; kea < kt_kea_size; kea++) {
> +	if (certStatus[kea] != NULL) {
> +	    SSL_SetStapledOCSPResponses(ssl_sock, certStatus[kea], kea);

Why don't you call SSL_SetStapledOCSPResponses() on model_sock in the
main() function?  I think the cert status array will be copied from
model_sock to ssl_sock, right?

::: lib/ssl/ssl.h
@@ +416,5 @@
>  SSL_IMPORT const SECItemArray * SSL_PeerStapledOCSPResponses(PRFileDesc *fd);
>  
>  /* SSL_SetStapledOCSPResponses stores an array of one or multiple OCSP responses
>   * in the fd's data, which may be sent as part of a server side cert_status
>   * handshake message.

Please document the |kea| argument. You can say |responses| is for the
server certificate of the key exchange type |kea|.

::: lib/ssl/ssl3con.c
@@ +8444,5 @@
>  static SECStatus
>  ssl3_SendCertificateStatus(sslSocket *ss)
>  {
>      SECStatus            rv;
>      int                  len 		= 0;

Nit: just do "len = 0" without the extra spaces before '='.

Actually, I think that when we add new functions to libSSL, we should not
format the variable declarations. They are extra work to maintain.

@@ +8459,5 @@
>  	return SECSuccess;
>  
> +    for (i = kt_null; i < kt_kea_size; i++) {
> +	if (ss->certStatusArray[i] && ss->certStatusArray[i]->len) {
> +	    /* TODO: The following code should look at the server certificate

The answer to this TODO is in ssl3_SendCertificate().

    if (ss->sec.isServer) {
        sslServerCerts * sc = NULL;

        /* XXX SSLKEAType isn't really a good choice for 
         * indexing certificates (it breaks when we deal
         * with (EC)DHE-* cipher suites. This hack ensures
         * the RSA cert is picked for (EC)DHE-RSA.
         * Revisit this when we add server side support
         * for ECDHE-ECDSA or client-side authentication
         * using EC certificates.
         */
        if ((ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa) ||
            (ss->ssl3.hs.kea_def->kea == kea_dhe_rsa)) {
            certIndex = kt_rsa;
        } else {
            certIndex = ss->ssl3.hs.kea_def->exchKeyType;
        }
        sc                    = ss->serverCerts + certIndex;
        certChain             = sc->serverCertChain;
        ss->sec.authKeyBits   = sc->serverKeyBits;
        ss->sec.authAlgorithm = ss->ssl3.hs.kea_def->signKeyType;
        ss->sec.localCert     = CERT_DupCertificate(sc->serverCert);
    } else {

We can probably add a new member to ss->sec to remember the
certStatusArray we should be sending.  Alternatively, we can copy this
part of the code to ssl3_SendCertificateStatus:

    SSL3KEAType          certIndex;

    if (ss->sec.isServer) {
        if ((ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa) ||
            (ss->ssl3.hs.kea_def->kea == kea_dhe_rsa)) {
            certIndex = kt_rsa;
        } else {
            certIndex = ss->ssl3.hs.kea_def->exchKeyType;
        }
        statusToSend = ss->certStatusArray[certIndex];
    }

@@ +8468,5 @@
> +	     *       See also: ssl3_ServerSendStatusRequestXtn
> +	     */
> +	    if (!statusToSend) {
> +		statusToSend = ss->certStatusArray[i];
> +	    }

This inner if statement can be replaced by these two lines:

    statusToSend = ss->certStatusArray[i];
    break;

The !statusToSend test is not necessary if we break out of the for
loop.

::: lib/ssl/ssl3ext.c
@@ +686,5 @@
>  
> +    for (i = kt_null; i < kt_kea_size; i++) {
> +	/* TODO: This is a temporary workaround.
> +	 *       The correct code needs to see if we have an OCSP response for
> +	 *       the server certificate being used, rather than if we have any 

Nit: Splinter review shows there is a space at the end of the line.

::: lib/ssl/sslsock.c
@@ +1679,2 @@
>              if (!sc->serverCertChain)
>                  goto loser;

This null pointer test for sc->serverCertChain should not be separated
from "sc->serverCertChain = CERT_DupCertList(mc->serverCertChain);".

@@ +2245,5 @@
>  	SSL_DBG(("%d: SSL[%d]: bad socket in SSL_SetStapledOCSPResponses",
>  		 SSL_GETPID(), fd));
>  	return SECFailure;
>      }
> +    

Nit: splinter review still shows some spaces on this blank line.

Comment 7

[Wan-Teh Chang]

Comment on attachment 744805 [details] [diff] [review]
patch v2

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

::: cmd/selfserv/selfserv.c
@@ +2598,5 @@
>  	    }
>  	    fprintf(stderr, "selfserv: %s can%s bypass\n", nickName,
>  		    bypassOK ? "" : "not");
>  	}
> +	setupCertStatus(certStatusArena, ocspStaplingMode, &pwdata, 

Nit: splinter review shows a space at the end of this line and the other
setupCertStatus call on line 2629.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Patch v2 no longer applied to NSS trunk.
Patch v3 is a merged version.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 750575 [details] [diff] [review]
Patch v3

Carrying forward r=wtc on this initial patch, which Wan-Teh has proposed to check in, despite the change requests.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Incremental patch v4 (on top of v3)

> > +SECItemArray *certStatus[kt_kea_size] = { NULL };
> 
> I think this array should be added to the main() function

But certStatus is accessed by function setupCertStatus.
Your request would require several additional changes.

(I actually had started to make changes, because of your other request to move the call to SSL_SetStapledOCSPResponses(). But I have concluded that moving the call to main() is not possible. Given this isn't library code, but only tool code, IMHO it should be fine to have this global variable.)


> The wincx/pwarg argument is usually the last function argument by
> convention. It would be nice to follow that convention in this
> function and the setupCertStatus function.

ok


> The |arena| variable in handle_connection() should be removed now.

ok


> Why don't you call SSL_SetStapledOCSPResponses() on model_sock in the
> main() function?  I think the cert status array will be copied from
> model_sock to ssl_sock, right?

The socket doesn't exist yet in main().
The socket gets created in server_main().

I could call SSL_SetStapledOCSPResponses() in server_main(), but that will still require that the certStatus array is global.

I seems the suggested change isn't a simplification.


> >  /* SSL_SetStapledOCSPResponses stores an array of one or multiple OCSP responses
> >   * in the fd's data, which may be sent as part of a server side cert_status
> >   * handshake message.
> 
> Please document the |kea| argument. You can say |responses| is for the
> server certificate of the key exchange type |kea|.

ok


> >      int                  len 		= 0;
> 
> Nit: just do "len = 0" without the extra spaces before '='.

That wasn't my new code, but I changed it anyway.


> @@ +8459,5 @@
> >  	return SECSuccess;
> >  
> > +    for (i = kt_null; i < kt_kea_size; i++) {
> > +	if (ss->certStatusArray[i] && ss->certStatusArray[i]->len) {
> > +	    /* TODO: The following code should look at the server certificate
> 
> The answer to this TODO is in ssl3_SendCertificate().

Thanks a lot for researching this detail.
I used your alternative proposal.

Because that function is only used by server code, I propose to convert the "if" test into an assert/error.


> > +	    if (!statusToSend) {
> > +		statusToSend = ss->certStatusArray[i];
> > +	    }
> 
> This inner if statement can be replaced by these two lines:
> 
>     statusToSend = ss->certStatusArray[i];
>     break;

ok


> @@ +1679,2 @@
> >              if (!sc->serverCertChain)
> >                  goto loser;
> 
> This null pointer test for sc->serverCertChain should not be separated
> from "sc->serverCertChain = CERT_DupCertList(mc->serverCertChain);".

ok.
Also, I've added a NULL test for certStatusArray.

Comment 11

[Wan-Teh Chang]

Comment on attachment 750577 [details] [diff] [review]
Incremental patch v4 (on top of v3)

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

r=wtc.

Comment 12

[Kai Engert (:KaiE:)]

I checked in both patches in one shot:
https://hg.mozilla.org/projects/nss/rev/d0bc27856053

Comment 13

[Kai Engert (:KaiE:)]

We need a followup patch that addresses the TODO in function ssl3_ServerSendStatusRequestXtn.

Comment 14

[Kai Engert (:KaiE:)]

Reopening because of comment 13

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Additional incremental patch v5

Comment 16

[Wan-Teh Chang]

Comment on attachment 751302 [details] [diff] [review]
Additional incremental patch v5

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

r=wtc. Thanks.

::: lib/ssl/ssl3ext.c
@@ +695,4 @@
>      }
> +
> +    statusToSend = ss->certStatusArray[certIndex];
> +    if (!statusToSend || !statusToSend->len)

You should change ssl3_SendCertificateStatus (in ssl3con.c) to do exactly
the same as these two lines. Right now ssl3_SendCertificateStatus does this:

    if (ss->certStatusArray[certIndex] && ss->certStatusArray[certIndex]->len) {
        statusToSend = ss->certStatusArray[certIndex];
    }
    if (!statusToSend)

Alternatively, change this function to match ssl3_SendCertificateStatus.

Comment 17

[Wan-Teh Chang]

Attachment: Additional incremental patch v5.1

Kai: I made the change to ssl3_SendCertificateStatus that
I suggested, and checked this in.  Please review it.  Thanks.

https://hg.mozilla.org/projects/nss/rev/60da07951c17

Comment 18

[Wan-Teh Chang]

Comment on attachment 752500 [details] [diff] [review]
Additional incremental patch v5.1

This patch causes selfserv to crash in ssl3_ServerSendStatusRequestXtn
because ss->ssl3.hs.kea_def is NULL.  ss->ssl3.hs.kea_def is not set up
until ssl3_SetupPendingCipherSpec is called at the end of ssl3_SendServerHello.

A possible fix is to set ss->ssl3.hs.kea_def whenever we set
ss->ssl3.hs.cipher_suite and ss->ssl3.hs.suite_def.

I reverted this patch: https://hg.mozilla.org/projects/nss/rev/c499082b9a15

The call stack of the crash is:

>	ssl3.dll!ssl3_ServerSendStatusRequestXtn(sslSocketStr * ss=0x007ad5d8, int append=0, unsigned int maxBytes=65535)  Line 691 + 0x9 bytes	C
 	ssl3.dll!ssl3_CallHelloExtensionSenders(sslSocketStr * ss=0x007ad5d8, int append=0, unsigned int maxBytes=65535, const ssl3HelloExtensionSender * sender=0x007b0404)  Line 1695 + 0x14 bytes	C
 	ssl3.dll!ssl3_SendServerHello(sslSocketStr * ss=0x007ad5d8)  Line 7569 + 0x22 bytes	C
 	ssl3.dll!ssl3_SendServerHelloSequence(sslSocketStr * ss=0x007ad5d8)  Line 6487 + 0x9 bytes	C
 	ssl3.dll!ssl3_HandleClientHello(sslSocketStr * ss=0x007ad5d8, unsigned char * b=0x032d50ba, unsigned int length=0)  Line 7305 + 0x9 bytes	C
 	ssl3.dll!ssl3_HandleHandshakeMessage(sslSocketStr * ss=0x007ad5d8, unsigned char * b=0x032d5054, unsigned int length=102)  Line 9592 + 0x11 bytes	C
 	ssl3.dll!ssl3_HandleHandshake(sslSocketStr * ss=0x007ad5d8, sslBufferStr * origBuf=0x007ad848)  Line 9744 + 0x19 bytes	C
 	ssl3.dll!ssl3_HandleRecord(sslSocketStr * ss=0x007ad5d8, SSL3Ciphertext * cText=0x0116ca18, sslBufferStr * databuf=0x007ad848)  Line 10382 + 0xd bytes	C
 	ssl3.dll!ssl3_GatherCompleteHandshake(sslSocketStr * ss=0x007ad5d8, int flags=0)  Line 366 + 0x16 bytes	C
 	ssl3.dll!ssl_GatherRecord1stHandshake(sslSocketStr * ss=0x007ad5d8)  Line 1226 + 0xb bytes	C
 	ssl3.dll!ssl_Do1stHandshake(sslSocketStr * ss=0x007ad5d8)  Line 119 + 0xf bytes	C
 	ssl3.dll!ssl_SecureRecv(sslSocketStr * ss=0x007ad5d8, unsigned char * buf=0x0116cc44, int len=10239, int flags=0)  Line 1133 + 0x9 bytes	C
 	ssl3.dll!ssl_SecureRead(sslSocketStr * ss=0x007ad5d8, unsigned char * buf=0x0116cc44, int len=10239)  Line 1152 + 0x13 bytes	C
 	ssl3.dll!ssl_Read(PRFileDesc * fd=0x033bc548, void * buf=0x0116cc44, int len=10239)  Line 2152 + 0x17 bytes	C
 	nspr4.dll!PR_Read(PRFileDesc * fd=0x033bc548, void * buf=0x0116cc44, int amount=10239)  Line 109 + 0x16 bytes	C
 	selfserv.exe!handle_connection(PRFileDesc * tcp_sock=0x033bc548, PRFileDesc * model_sock=0x00705588, int requestCert=0)  Line 1329 + 0x1e bytes	C
 	selfserv.exe!jobLoop(PRFileDesc * a=0x00000000, PRFileDesc * b=0x00000000, int c=0)  Line 631 + 0x1a bytes	C
 	selfserv.exe!thread_wrapper(void * arg=0x00776aa8)  Line 599 + 0x1c bytes	C
 	nspr4.dll!_PR_NativeRunThread(void * arg=0x00778cd8)  Line 397 + 0xf bytes	C
 	nspr4.dll!pr_root(void * arg=0x00778cd8)  Line 90 + 0xf bytes	C
 	msvcr110.dll!_callthreadstartex()  Line 354 + 0x6 bytes	C
 	msvcr110.dll!_threadstartex(void * ptd=0x00778e28)  Line 332 + 0x5 bytes	C

Comment 19

[Kai Engert (:KaiE:)]

Comment on attachment 752500 [details] [diff] [review]
Additional incremental patch v5.1

I've filed bug 876393 for implementing a correct fix.