731478 - Cache the peer's intermediate CA certificates in SSL session ID

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

Today the sslSessionID structure stores only the peer's leaf certificate.
So if an SSL session is resumed and we call SSL_PeerCertificate, NSS may
not be able to recreate the certificate chain from the peer certificate.

The proposed patch caches the peer's intermediate CA certificates in
SSL session ID, so that they're available when we resume a session.

For simplicity I store the peer's intermediate CA certificates in a
fixed-sized array rather than a linked list of ssl3CertNode's allocated
from an arena.

Brian: please be the primary reviewer.

Bob: if you are busy, please review the two design decisions:
- If we cache the intermediate CA certificates, the memory usage could
  go up.  Is this OK?  But this patch is required for correctness.
- Is it enough to store at most 8 CA certificates?

Comment 1

[Robert Relyea]

Bob: if you are busy, please review the two design decisions:
> - If we cache the intermediate CA certificates, the memory usage could
>   go up.  Is this OK?  But this patch is required for correctness.
Why not make the generic cache interface take CERTCertificates?
The clients could cache references to the CERTCertificate.
The Server cache could decide to dispense with the client's intermediates or store them out in full.

> - Is it enough to store at most 8 CA certificates?
Depends on the failure mode. 90% of the time 8 certs are fine. If the failure mode is we have to renegotiate, then I think that's OK. If the failure is we mysteriously start returning errors, then that's another matter.

Comment 2

[Robert Relyea]

Oh, you are using the CERTCertificate pointers already. We should have memory issues then unless we have lots of unique intermediates on our connections.

As for as the fixed array, I'd say just use a dynamic array. It's just one alloc, though you have to count your certs.

Your patch has completely left sslsnce.c out. That's probably OK. the sid it creates zeros the fields it doesn't explicitly touch. This means we are implementing what I described as "the server cache dispensing with the client's intermediates", which is probably an OK semantic. We definately have some comments there, though showing that we thought about the problem and make this explicit decision rather than accidentally leaving out the functionality.

bob

Comment 3

[Robert Relyea]

"We should have memory issues" is missing the "not": "We should not have memory issues"

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Do you think it is necessary for the application to rebuild and re-validate the chain during a resumed session? I got the impression from the discussion in bug 695511 that you did not think it was necessary to do so, as long the application itself cached the built chain and/or the validation result.

Why do we need ssl3_CopyPeerCertsFromSID? From looking at something similar before, I remember thinking that we could remove ss->ssl3.peerCertArena completely, if we store the cert chain in ss->sid.

I think that, even with this patch, there will still be some cases where you will not be able to reconstruct a cert chain that you were able to construct during the initial handshake. In particular, let's say that the initial connection sent certificate EE1 (only), and during that initial connection, you also had a session open for a server with cert chain with cert EE2 and cert CA1, such that CA1 issued both EE1 and EE2. Further, assume that all your sessions for the site with cert EE2 are deleted. Then, CA1 will no longer be in memory, and when you try to re-validate EE1 during a session resumption, the resumption would fail. This makes me think that it would be better to fix libpkix's cert_po_certList mechanism, and then use *that* cert list in the application (mapping the EE cert -> cert chain).

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #4)
> This makes me think that it would be
> better to fix libpkix's cert_po_certList mechanism, and then use *that* cert
> list in the application (mapping the EE cert -> cert chain).

Alternatively, it might be a good idea to add a new input parameter to CERT_PKIXVerifyCert to tell it to ignore all *non-permanent* certificates except those that are passed in in a new cert_pi_hintCerts parameter. This would allow all certificate validations to be independent of each other--the certs sent by the server on connection X would not be able to influence the validation of the cert chain sent on connection Y. I think this would be a very good property for browsers to have.

Comment 6

[Robert Relyea]

>  you also had a session open for a server with cert chain with cert EE2 and cert CA1,
> such that CA1 issued both EE1 and EE2. Further, assume that all your sessions for the site
> with cert EE2 are deleted. Then, CA1 will no longer be in memory, and when you try to
> re-validate EE1 during a session resumption, the resumption would fail. This makes me think
> that it would be better to fix libpkix's cert_po_certList mechanism, and then use *that* cert
> list in the application (mapping the EE cert -> cert chain).

Good point. We probably should store the validated chain rather than the certs that were sent on the connection unless we need the actual certs to diagnose issues.

bob

Comment 7

[Wan-Teh Chang]

(Argh, I forgot that I already attached the patches to bug 651996.  I didn't
find that bug because it isn't filed under the product NSS.  Perhaps this
bug should be marked as a duplicate.)

The purpose of this patch is not to verify the peer's certificate on session
resumption.  An application may need to get the peer's certificate chain for
the UI (the certificate viewer dialog) or archival.  It needs to be able to
get the peer's certificate chain even when the SSL connection set up by the
full handshake is gone.

Today an application has to call SSL_PeerCertificate and then call an NSS
function to build the chain.  The new SSL_PeerCertificateChain function
proposed in bug 731485 will allow an application to get the peer's
certificate chain in one shot.  For either method to work, NSS needs to keep
the other certificates presented by the peer.

Re: storing the validated chain vs. the certs that were sent on the connection

Storing the certs that were sent on the connection is more flexible because
the validated chain can be recreated if necessary.

This patch is neutral to which chain is stored even though it currently stores
the certs that were sent on the connection.  We do need to specify which chain
is returned by the proposed SSL_PeerCertificateChain function.  If we work hard
enough, I'm sure we can store both chains efficiently.

Comment 8

[Wan-Teh Chang]

(In reply to Brian Smith (:bsmith) from comment #4)
>
> Why do we need ssl3_CopyPeerCertsFromSID? From looking at something similar
> before, I remember thinking that we could remove ss->ssl3.peerCertArena
> completely, if we store the cert chain in ss->sid.

I guess by ss->sid you meant ss->sec.ci.sid?  And I guess we can also remove
ss->ssl3.peerCertChain.  I will look into that.  Thank you for the suggestion.
(I did notice that ss->ssl3.peerCertArena is a wasteful way to allocate
ss->ssl3.peerCertChain.)

Bob: thank you for pointing out sslsnce.c also needs changing.  When I wrote
this patch, I searched for certain patterns (e.g., all assignment statements
involving ss->sec.peerCert and ss->sec.ci.sid->peerCert) to identify the
places that I needed to change.  Perhaps I didn't do a thorough job, or I
excluded sslsnce.c intentionally.

I will change the fixed-size array to a dynamic array.  I was trying to reduce
the number of memory allocations, but I think these days people no longer care
that much about that and instead invest their resources on a fast memory allocator
such as jemalloc and tcmalloc.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 601479 [details] [diff] [review]
Proposed patch

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

Clearing review request since Wan-Teh is going to submit a new patch.

Comment 10

[Robert Relyea]

Comment on attachment 601479 [details] [diff] [review]
Proposed patch

clearing review flag on this patch

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Replace ss->sec.peerCert and ss->sec.ci.sid->peerCert with ss->sec.ci.sid->peerCertChain

Wan-Teh,

One of the concerns I had about your patch was whether the peer cert chain is kept in sync with peerCert. In this patch, I remove both peerCert members so that the end-entity and intermediate certificates are always stored in one CERTCertList structure. This required a lot of small edits, but in the process of making these edits it became clearer when we can get out of sync. For example, sometimes we do not copy the intermediate certs along with the end-entity certificates when dealing with session tickets and the persistent session cache. Also, if you duplicate a socket then ss->sec.peerCert can get out of sync with ss->sec.ci.sid->peerCert; this probably isn't a real problem because the application shouldn't be duplicating sockets that have actually been connected (but then why does ssl_DupSocket bother copying ss->sec.peerCert at all?).

This patch doesn't actually implement the public function for returning the cert chain. That is easy to do, but I want to get feedback on this approach first.

In order to avoid wasting too much memory, I shrunk the arena chunk size CERT_NewCertList uses. I went through all the callers, and PK11_ListCerts & PK11_ListCertsInSlot are the ones most likely to be harmed by the smaller chunk size. This might be a problem for pkix_pl_Pk11CertStore_CertQuery's use of PK11_ListCerts. I am not sure how much effort is warranted for reducing the memory usage here.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Wan-Teh - is there some way I can help move this bug along? We need it for a new feature we're working on in Firefox. Brian said it would be helpful if I finished the patch, but it looks like we've got two approaches here, and I wasn't sure which one to go with.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: Brian's patch, updated

Hi Wan-Teh: Brian's patch needed updating, so I took care of that. If you could review it, that would be great.
It passed the automated tests on my local machine.

Comment 14

[Wan-Teh Chang]

Attachment: Brian's patch, updated v2

I updated Brian's patch to the current NSS tip. I also included the newly
added SSL_PeerCertificateChain function.

Comment 15

[Wan-Teh Chang]

Attachment: Brian's patch, updated v2.1

The only difference from v2 should be tabs vs. spaces.

Comment 16

[Wan-Teh Chang]

Comment on attachment 812302 [details] [diff] [review]
Brian's patch, updated v2.1

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

Brian: I made a pass through your patch. It seems correct. I have some questions.

The patch assumes that ss->sec.ci.sid is already pointing to an appripriate sslSessionID
whenever the original code accesses ss->sec.peerCert. Is it easy to explain why this is
true?

::: lib/certdb/certdb.c
@@ +2518,5 @@
> +    /* Most cert chains are less than 4 certs long, not including the root, so
> +     * allocate much less memory than the default DER_DEFAULT_CHUNKSIZE we
> +     * usually pick.
> +     */
> +    arena = PORT_NewArena(sizeof(CERTCertList) + 4 * sizeof(CERTCertListNode));

This is a good change. Note that CERTCertList is not only used for certificate chains,
so we may want to assume a typical list size a bit larger than 4.

CERTCertList consists of 3 pointers. CERTCertListNode consists of 4 pointers. So your
current arena chunk size is about 20 pointers. If a pointer is 8 bytes, this is about
160 bytes. I think we can go up to the next power of 2 (256), so we should be able to
change 4 to 7.

::: lib/ssl/ssl3con.c
@@ -3095,5 @@
> -	}
> -	CERT_DestroyCertificate(ss->sec.peerCert);
> -	ss->sec.peerCert = NULL;
> -    }
> -    ssl3_CleanupPeerCerts(ss);

Why do we NOT need to destroy the peerKey and peer certificate chain?

@@ +9400,5 @@
>  	goto alert_loser;
>      }
>  
> +    PORT_Assert(ss->sec.ci.sid->peerCertChain == NULL);
> +    PORT_Assert(ss->sec.peerKey == NULL);

Why do we NOT need to destroy the peerKey and peer certificate chain?

Why are ss->sec.ci.sid->peerCertChain and ss->sec.peerKey NULL in a renegotiation
if the initial handshake did client authentication?

@@ +9456,5 @@
> +	    ss->sec.ci.sid->peerCertChain = CERT_NewCertList();
> +	    if (ss->sec.ci.sid->peerCertChain == NULL) {
> +		goto ambiguous_err;
> +	    }
> +	}

It seems better to create ss->sec.ci.sid->peerCertChain before entering the while loop.

::: lib/ssl/ssl3ext.c
@@ +1530,5 @@
>  
>  	    /* Copy over client cert from session ticket if there is one. */
>  	    if (parsed_session_ticket->peer_cert.data != NULL) {
> +		CERTCertificate * cert;
> +		PORT_Assert(sid->peerCertChain == NULL);

We should move this assertion to ssl_SaveSinglePeerCert.

::: lib/ssl/sslcon.c
@@ +2166,5 @@
>  {
>      sslSessionID *sid = ss->sec.ci.sid;
>  
>      /* Record entry in nonce cache */
> +    PORT_Memcpy(sid->u.ssl2.sessionID, s, sizeof(sid->u.ssl2.sessionID));

Is it OK to do this unconditionally now?

::: lib/ssl/sslsnce.c
@@ +628,3 @@
>  		goto loser;
> +
> +	    rv = ssl_SaveSinglePeerCert(to, peerCert);

You need to destroy peerCert.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 812302 [details] [diff] [review]
Brian's patch, updated v2.1

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

::: lib/certdb/certdb.c
@@ +2518,5 @@
> +    /* Most cert chains are less than 4 certs long, not including the root, so
> +     * allocate much less memory than the default DER_DEFAULT_CHUNKSIZE we
> +     * usually pick.
> +     */
> +    arena = PORT_NewArena(sizeof(CERTCertList) + 4 * sizeof(CERTCertListNode));

OK.

::: lib/ssl/ssl3con.c
@@ -3095,5 @@
> -	}
> -	CERT_DestroyCertificate(ss->sec.peerCert);
> -	ss->sec.peerCert = NULL;
> -    }
> -    ssl3_CleanupPeerCerts(ss);

Before this patch: During a renegotiation, ss->sec.peerCert and ss->sec.peerKey would be the cert/key of the peer from the previous handshake, so they had to be cleared.

After this patch: That information is now stored only in the sid. We know we are doing a full handshake because ssl3_HandleNoCertificate is never called during a session resumption. Every full handshake creates a new session. (If we were resuming a session, then we'd already know the peer cert/key from the session cache entry.)

@@ +9400,5 @@
>  	goto alert_loser;
>      }
>  
> +    PORT_Assert(ss->sec.ci.sid->peerCertChain == NULL);
> +    PORT_Assert(ss->sec.peerKey == NULL);

We know we are starting a new session because if we were resuming a session we would not be processing a Certificate message. When we are starting a new session, the cert chain will be NULL until we process the cert chain, which is what this function (ssl3_HandleCertificate) does.

ss->sec.peerKey now is only used to store the server's public key from the ServerKeyExchange message when we are the client. ssl3_SendClientKeyExchange resets ss->sec.peerKey to NULL. So, ss->sec.peerKey is only non-null between the time we receive the ServerKeyExchange message and the time we send the ClientKeyExchange message.

This would be clearer if we renamed ss->sec.peerKey to ss->sec.serverEphemeralKey. I will make this change.

@@ +9456,5 @@
> +	    ss->sec.ci.sid->peerCertChain = CERT_NewCertList();
> +	    if (ss->sec.ci.sid->peerCertChain == NULL) {
> +		goto ambiguous_err;
> +	    }
> +	}

I will investigate this.

::: lib/ssl/sslcon.c
@@ +2166,5 @@
>  {
>      sslSessionID *sid = ss->sec.ci.sid;
>  
>      /* Record entry in nonce cache */
> +    PORT_Memcpy(sid->u.ssl2.sessionID, s, sizeof(sid->u.ssl2.sessionID));

It is hard for me to remember why I made this change like this. Are we going to remove the SSL 2.0 code? If so, then let's just do that before we land this.

Comment 18

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Patch rebased for NSS 3.16

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Patch rebased for NSS 3.16

This addresses the previous review comments.

Comment 21

[Wan-Teh Chang]

Comment on attachment 8385449 [details] [diff] [review]
Patch rebased for NSS 3.16

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

r=wtc. Please make the changes I suggested before checking this in.
I marked the more important comments with "IMPORTANT" or "BUG".

I suggest postponing this patch to NSS 3.16.1. I would not be
surprised to get a report of an assertion failure.

::: lib/ssl/ssl3con.c
@@ -3113,5 @@
> -    if (ss->sec.peerCert != NULL) {
> -	if (ss->sec.peerKey != NULL) {
> -	    SECKEY_DestroyPublicKey(ss->sec.peerKey);
> -	    ss->sec.peerKey = NULL;
> -	}

IMPORTANT: this also destroys ss->sec.peerKey. Are you sure
we don't need to do that?

@@ -9655,5 @@
> -static void
> -ssl3_CleanupPeerCerts(sslSocket *ss)
> -{
> -    PLArenaPool * arena = ss->ssl3.peerCertArena;
> -    ssl3CertNode *certs = (ssl3CertNode *)ss->ssl3.peerCertChain;

Please find out if we can remove the ssl3CertNode type definition.

@@ +9751,5 @@
>      }
>  
> +    ss->sec.ci.sid->peerCertChain = CERT_NewCertList();
> +    if (!ss->sec.ci.sid->peerCertChain) {
> +	goto loser;

Nit: I suggest preserving the "don't send alerts on memory errors"
comment on the "goto loser" line.

@@ +9780,1 @@
>  	    goto ambiguous_err;

Nit: I suggest preserving the original comment (it was before
the while loop) "We should report an alert if the cert was bad,
but not if the problem was just some local problem, like memory
error." before the "goto ambiguous_err" line.

@@ +9786,5 @@
> +	    goto loser;
> +	}
> +    }
> +
> +    if (remaining != 0 || !ss->sec.ci.sid->peerCertChain)

IMPORTANT: are you sure we need to test !ss->sec.ci.sid->peerCertChain?
ss->sec.ci.sid->peerCertChain cannot be NULL here.

@@ +9791,4 @@
>          goto decode_loser;
>  
> +    cert = SSL_PEER_CERT(ss->sec.ci.sid);
> +    PORT_Assert(cert); // peerCertChain is never empty

Use C comment delimiters /* */.

@@ +9827,5 @@
>  
>  loser:
> +    /* Although peerCertChain would eventually be destroyed when the sid is
> +     * destroyed, we destroy it now to enforce the invariant that it is never
> +     * empty. */

Nit: this comment seems to imply that peerCertChain is empty,
but peerCertChain may be non-empty (but incomplete). So the
comment is a little confusing.

I suggest either adding "peerCertChain may be non-empty." or just
removing the comment.

::: lib/ssl/ssl3ext.c
@@ +1099,1 @@
>  	rv = ssl3_AppendNumberToItem(&plaintext, CLIENT_AUTH_CERTIFICATE, 1);

Nit: it may be a good idea to cache SSL_PEER_CERT(ss->sec.ci.sid)
in a local variable. This block uses that expression three times.

@@ +1263,5 @@
> +/* Saves a copy of cert to the given session as the only cert in the
> + * chain. This is always the wrong thing to do. Also used by sslcon.c.
> + */
> +SECStatus
> +ssl_SaveSinglePeerCert(sslSessionID * sid, CERTCertificate * cert)

Nit: don't add a space between '*' and the variable/argument
name. Also fix this on line 1666.

@@ +1277,5 @@
> +    cert = CERT_DupCertificate(cert);
> +    rv = CERT_AddCertToListTail(sid->peerCertChain, cert);
> +    if (rv != SECSuccess) {
> +	CERT_DestroyCertList(sid->peerCertChain);
> +	CERT_DestroyCertificate(cert);

BUG: do not destroy |cert|. The copy of |cert| in the cert
list is destroyed by CERT_DestroyCertList.

@@ +1283,5 @@
> +    }
> +    return rv;
> +}
> +
> +

Nit: delete one blank line here?

::: lib/ssl/sslauth.c
@@ +20,5 @@
>  	SSL_DBG(("%d: SSL[%d]: bad socket in PeerCertificate",
>  		 SSL_GETPID(), fd));
>  	return 0;
>      }
> +    if (ss->opt.useSecurity && ss->sec.ci.sid &&

I assume ss->sec.ci.sid may be NULL?

@@ +153,5 @@
>  
>  	if (ip || sp) {
>  	    CERTCertificate *cert;
> +	    if (ss->sec.ci.sid && ss->sec.ci.sid->peerCertChain) {
> +		cert = SSL_PEER_CERT(ss->sec.ci.sid);

The declaration of |cert| can be moved into this block.

::: lib/ssl/sslcon.c
@@ +2167,5 @@
>  {
>      sslSessionID *sid = ss->sec.ci.sid;
>  
> +    PORT_Assert(sid->peerCertChain);
> +    PORT_Assert(CERT_LIST_HEAD(sid->peerCertChain));

Nit: the second assertion should be unnecessary. You don't assert
that elsewhere.

@@ +2266,5 @@
>      	goto done;
>      rv = SGN_Update(sgn, challenge, len);
>      if (rv != SECSuccess) 
>      	goto done;
> +

Nit: delete this blank line.

@@ -2467,5 @@
>      if (SECSuccess == rv) 
>  	goto done;
>  
>  loser:
> -    ss->sec.peerCert = NULL;

This isn't preserved in the new code. The equivalent new code
would be:

    CERT_DestroyCertList(ss->sec.ci.sid->peerCertChain);
    ss->sec.ci.sid->peerCertChain = NULL;

Are you sure we don't need to do that?

::: lib/ssl/sslimpl.h
@@ +1111,1 @@
>      SECKEYPublicKey *peerKey;					/* ssl3 only */

Nit: I wonder if it would be useful to add a comment that
tell people where to find the peer certificate (chain).

@@ +1902,5 @@
> +#define SSL_PEER_CERT(sid) \
> +    (CERT_LIST_HEAD((sid)->peerCertChain)->cert)
> +
> +SECStatus ssl_SaveSinglePeerCert(sslSessionID * sid,
> +CERTCertificate * cert);

Nit: align the second argument with the opening parenthesis
on the previous line.

::: lib/ssl/sslnonce.c
@@ +285,5 @@
>  		   /* server hostname matches. */
>  	           (sid->urlSvrName != NULL) &&
>  		   ((0 == PORT_Strcmp(urlSvrName, sid->urlSvrName)) ||
> +		    (sid->peerCertChain && (SECSuccess ==
> +		      CERT_VerifyCertName(SSL_PEER_CERT(sid), urlSvrName))) )

Nit: this line seems over-indented by one space.

::: lib/ssl/sslreveal.c
@@ -24,5 @@
> -  /* CERT_DupCertificate increases reference count and returns pointer to 
> -   * the same cert
> -   */
> -  if (sslsocket && sslsocket->sec.peerCert)
> -    cert = CERT_DupCertificate(sslsocket->sec.peerCert);

SSL_PeerCertificate also tests ss->opt.useSecurity. But I agree
that difference is not important.

::: lib/ssl/sslsnce.c
@@ +628,4 @@
>  		goto loser;
> +	    }
> +
> +	    rv = ssl_SaveSinglePeerCert(to, peerCert);

Should we also mark this ssl_SaveSinglePeerCert call with a bug
number?

@@ +909,5 @@
>              if (name->len && name->data) {
>                  now = CacheSrvName(cache, name, &sce);
>              }
> +            if (sid->peerCertChain) {
> +                /* XXX: Bug 657236 - only the 1st cert is cached */

Please open a new bug because this is the server session cache,
not the session ticket.

Nit: 1st cert => leaf cert

Comment 22

[Wan-Teh Chang]

Comment on attachment 8385449 [details] [diff] [review]
Patch rebased for NSS 3.16

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

Brian:

I just read your response to my review comments on a
previous version of this patch. I have two new comments.

::: lib/certdb/certdb.c
@@ +2518,5 @@
>      PLArenaPool *arena = NULL;
>      CERTCertList *ret = NULL;
>      
> +    /* Most cert chains are less than 7 certs long, not including the root, so
> +     * allocate much less memory than the default DER_DEFAULT_CHUNKSIZE we

OK, I looked at our past discussion on this comment. I think
the comment should still say "4 certs long". We can explain
why we use 7 instead of 4, something like "We can allocate 7
list nodes without exceeding the next power of 2."

::: lib/ssl/sslimpl.h
@@ +1111,1 @@
>      SECKEYPublicKey *peerKey;					/* ssl3 only */

In comment 17, you wrote:
> ss->sec.peerKey now is only used to store the server's public key from the
> ServerKeyExchange message when we are the client. ssl3_SendClientKeyExchange
> resets ss->sec.peerKey to NULL. So, ss->sec.peerKey is only non-null between
> the time we receive the ServerKeyExchange message and the time we send the
> ClientKeyExchange message.
>
> This would be clearer if we renamed ss->sec.peerKey to
> ss->sec.serverEphemeralKey. I will make this change.

You didn't make this change. If you are still interested,
I suggest making that change separately. In this patch,
you can just add a comment to explain how |peerKey| is used.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Hi Wan-Teh, I'm hoping to finish up this patch and get it checked in.

(In reply to Wan-Teh Chang from comment #21)
> Comment on attachment 8385449 [details] [diff] [review]

> ::: lib/ssl/ssl3con.c
> @@ -3113,5 @@
> > -    if (ss->sec.peerCert != NULL) {
> > -	if (ss->sec.peerKey != NULL) {
> > -	    SECKEY_DestroyPublicKey(ss->sec.peerKey);
> > -	    ss->sec.peerKey = NULL;
> > -	}
> 
> IMPORTANT: this also destroys ss->sec.peerKey. Are you sure
> we don't need to do that?

My assumption is no, based on comment 17 and comment 22.

> @@ -9655,5 @@
> > -static void
> > -ssl3_CleanupPeerCerts(sslSocket *ss)
> > -{
> > -    PLArenaPool * arena = ss->ssl3.peerCertArena;
> > -    ssl3CertNode *certs = (ssl3CertNode *)ss->ssl3.peerCertChain;
> 
> Please find out if we can remove the ssl3CertNode type definition.

Turns out, yes, we can (and so I did).

> @@ +9751,5 @@
> >      }
> >  
> > +    ss->sec.ci.sid->peerCertChain = CERT_NewCertList();
> > +    if (!ss->sec.ci.sid->peerCertChain) {
> > +	goto loser;
> 
> Nit: I suggest preserving the "don't send alerts on memory errors"
> comment on the "goto loser" line.

Ok - kept it.

> @@ +9780,1 @@
> >  	    goto ambiguous_err;
> 
> Nit: I suggest preserving the original comment (it was before
> the while loop) "We should report an alert if the cert was bad,
> but not if the problem was just some local problem, like memory
> error." before the "goto ambiguous_err" line.

Ok - kept it before "goto ambiguous_err".

> @@ +9786,5 @@
> > +	    goto loser;
> > +	}
> > +    }
> > +
> > +    if (remaining != 0 || !ss->sec.ci.sid->peerCertChain)
> 
> IMPORTANT: are you sure we need to test !ss->sec.ci.sid->peerCertChain?
> ss->sec.ci.sid->peerCertChain cannot be NULL here.

Removed the test.

> @@ +9791,4 @@
> >          goto decode_loser;
> >  
> > +    cert = SSL_PEER_CERT(ss->sec.ci.sid);
> > +    PORT_Assert(cert); // peerCertChain is never empty
> 
> Use C comment delimiters /* */.

Changed.

> @@ +9827,5 @@
> >  
> >  loser:
> > +    /* Although peerCertChain would eventually be destroyed when the sid is
> > +     * destroyed, we destroy it now to enforce the invariant that it is never
> > +     * empty. */
> 
> Nit: this comment seems to imply that peerCertChain is empty,
> but peerCertChain may be non-empty (but incomplete). So the
> comment is a little confusing.
> 
> I suggest either adding "peerCertChain may be non-empty." or just
> removing the comment.

I removed the comment.

> ::: lib/ssl/ssl3ext.c
> @@ +1099,1 @@
> >  	rv = ssl3_AppendNumberToItem(&plaintext, CLIENT_AUTH_CERTIFICATE, 1);
> 
> Nit: it may be a good idea to cache SSL_PEER_CERT(ss->sec.ci.sid)
> in a local variable. This block uses that expression three times.

I added a local variable for it.

> @@ +1263,5 @@
> > +/* Saves a copy of cert to the given session as the only cert in the
> > + * chain. This is always the wrong thing to do. Also used by sslcon.c.
> > + */
> > +SECStatus
> > +ssl_SaveSinglePeerCert(sslSessionID * sid, CERTCertificate * cert)
> 
> Nit: don't add a space between '*' and the variable/argument
> name. Also fix this on line 1666.

Fixed (if I understand correctly).

> @@ +1277,5 @@
> > +    cert = CERT_DupCertificate(cert);
> > +    rv = CERT_AddCertToListTail(sid->peerCertChain, cert);
> > +    if (rv != SECSuccess) {
> > +	CERT_DestroyCertList(sid->peerCertChain);
> > +	CERT_DestroyCertificate(cert);
> 
> BUG: do not destroy |cert|. The copy of |cert| in the cert
> list is destroyed by CERT_DestroyCertList.

I think CERT_AddCertToListTail will only fail if it doesn't succeed in taking ownership of cert, so I believe this is correct as written.

> @@ +1283,5 @@
> > +    }
> > +    return rv;
> > +}
> > +
> > +
> 
> Nit: delete one blank line here?

Done.

> ::: lib/ssl/sslauth.c
> @@ +20,5 @@
> >  	SSL_DBG(("%d: SSL[%d]: bad socket in PeerCertificate",
> >  		 SSL_GETPID(), fd));
> >  	return 0;
> >      }
> > +    if (ss->opt.useSecurity && ss->sec.ci.sid &&
> 
> I assume ss->sec.ci.sid may be NULL?

Let me know if you think I should remove the check.

> @@ +153,5 @@
> >  
> >  	if (ip || sp) {
> >  	    CERTCertificate *cert;
> > +	    if (ss->sec.ci.sid && ss->sec.ci.sid->peerCertChain) {
> > +		cert = SSL_PEER_CERT(ss->sec.ci.sid);
> 
> The declaration of |cert| can be moved into this block.

Moved.

> ::: lib/ssl/sslcon.c
> @@ +2167,5 @@
> >  {
> >      sslSessionID *sid = ss->sec.ci.sid;
> >  
> > +    PORT_Assert(sid->peerCertChain);
> > +    PORT_Assert(CERT_LIST_HEAD(sid->peerCertChain));
> 
> Nit: the second assertion should be unnecessary. You don't assert
> that elsewhere.

Removed.

> @@ +2266,5 @@
> >      	goto done;
> >      rv = SGN_Update(sgn, challenge, len);
> >      if (rv != SECSuccess) 
> >      	goto done;
> > +
> 
> Nit: delete this blank line.

Deleted.

> @@ -2467,5 @@
> >      if (SECSuccess == rv) 
> >  	goto done;
> >  
> >  loser:
> > -    ss->sec.peerCert = NULL;
> 
> This isn't preserved in the new code. The equivalent new code
> would be:
> 
>     CERT_DestroyCertList(ss->sec.ci.sid->peerCertChain);
>     ss->sec.ci.sid->peerCertChain = NULL;
> 
> Are you sure we don't need to do that?

I suppose so? I added it in any case.

> 
> ::: lib/ssl/sslimpl.h
> @@ +1111,1 @@
> >      SECKEYPublicKey *peerKey;					/* ssl3 only */
> 
> Nit: I wonder if it would be useful to add a comment that
> tell people where to find the peer certificate (chain).

I assume they can just look at the implementation of SSL_PeerCertificateChain or similar.

> @@ +1902,5 @@
> > +#define SSL_PEER_CERT(sid) \
> > +    (CERT_LIST_HEAD((sid)->peerCertChain)->cert)
> > +
> > +SECStatus ssl_SaveSinglePeerCert(sslSessionID * sid,
> > +CERTCertificate * cert);
> 
> Nit: align the second argument with the opening parenthesis
> on the previous line.

Aligned.

> ::: lib/ssl/sslnonce.c
> @@ +285,5 @@
> >  		   /* server hostname matches. */
> >  	           (sid->urlSvrName != NULL) &&
> >  		   ((0 == PORT_Strcmp(urlSvrName, sid->urlSvrName)) ||
> > +		    (sid->peerCertChain && (SECSuccess ==
> > +		      CERT_VerifyCertName(SSL_PEER_CERT(sid), urlSvrName))) )
> 
> Nit: this line seems over-indented by one space.

Fixed.

> ::: lib/ssl/sslreveal.c
> @@ -24,5 @@
> > -  /* CERT_DupCertificate increases reference count and returns pointer to 
> > -   * the same cert
> > -   */
> > -  if (sslsocket && sslsocket->sec.peerCert)
> > -    cert = CERT_DupCertificate(sslsocket->sec.peerCert);
> 
> SSL_PeerCertificate also tests ss->opt.useSecurity. But I agree
> that difference is not important.

Ok - I left it as is.

> ::: lib/ssl/sslsnce.c
> @@ +628,4 @@
> >  		goto loser;
> > +	    }
> > +
> > +	    rv = ssl_SaveSinglePeerCert(to, peerCert);
> 
> Should we also mark this ssl_SaveSinglePeerCert call with a bug
> number?

I added a comment.

> @@ +909,5 @@
> >              if (name->len && name->data) {
> >                  now = CacheSrvName(cache, name, &sce);
> >              }
> > +            if (sid->peerCertChain) {
> > +                /* XXX: Bug 657236 - only the 1st cert is cached */
> 
> Please open a new bug because this is the server session cache,
> not the session ticket.

Filed bug 1055859.

> Nit: 1st cert => leaf cert


Fixed.

Comment 24

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch rebased for NSS 3.17

I'd appreciate if you would have a final look at this, Wan-Teh. Thanks!

Comment 25

[J.C. Jones [:jcj] (he/they)]

Tim -

This looks like it will be useful to have once Bug 1406856 and Bug 1406854 land, making the cert chains widely available throughout Gecko... making it useful to consistently have them.

If it still looks like a reasonable approach, could you re-assign the rebase and review?

Comment 26

[Mark Goodwin [:mgoodwin]]

Can I take this, please?

Comment 27

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Please do :)

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

(In reply to J.C. Jones [:jcj] from comment #25)
> If it still looks like a reasonable approach, could you re-assign the rebase
> and review?

I'll make sure this gets reviewed once the dependencies are fixed and Mark has something to look at :)

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P1'.
:beurdouche, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 30

[Dennis Jackson]

@Dana - Do you know if this is still relevant? I'm hoping that since we ship intermediate certs now, there's no need to additionally store them here?

Comment 31

[Dana Keeler (she/her) (use needinfo) [:keeler]]

This isn't relevant to Firefox necessarily, but it's still a shortcoming in NSS.

Comment 32

[Dennis Jackson]

Thanks!