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

ASSIGNED
Assigned to

Status

NSS
Libraries
P1
normal
ASSIGNED
6 years ago
3 months ago

People

(Reporter: Wan-Teh Chang, Assigned: mgoodwin)

Tracking

(Blocks: 4 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 601479 [details] [diff] [review]
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?
Attachment #601479 - Flags: superreview?(rrelyea)
Attachment #601479 - Flags: review?(bsmith)

Comment 1

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 731485

Comment 2

6 years ago
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

6 years ago
"We should have memory issues" is missing the "not": "We should not have memory issues"
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).
(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

6 years ago
>  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
(Reporter)

Comment 7

6 years ago
(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.
(Reporter)

Comment 8

6 years ago
(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 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.
Attachment #601479 - Flags: review?(bsmith)
(Reporter)

Updated

6 years ago
Target Milestone: 3.13.4 → 3.13.5

Comment 10

6 years ago
Comment on attachment 601479 [details] [diff] [review]
Proposed patch

clearing review flag on this patch
Attachment #601479 - Flags: superreview?(rrelyea)
Created attachment 619897 [details] [diff] [review]
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.
Attachment #619897 - Flags: feedback?(wtc)

Updated

6 years ago
Target Milestone: 3.13.5 → 3.14
(Reporter)

Updated

6 years ago
Target Milestone: 3.14 → 3.14.1
(Reporter)

Updated

5 years ago
Target Milestone: 3.14.1 → 3.14.2

Updated

5 years ago
Target Milestone: 3.14.2 → 3.14.3
(Reporter)

Updated

5 years ago
Target Milestone: 3.14.3 → 3.15.1
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.
Flags: needinfo?(wtc)
(Reporter)

Updated

5 years ago
Flags: needinfo?(wtc)
Target Milestone: 3.15.1 → 3.15.2
Flags: needinfo?(wtc)
Flags: needinfo?(wtc)
Created attachment 803254 [details] [diff] [review]
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.
Attachment #619897 - Attachment is obsolete: true
Attachment #619897 - Flags: feedback?(wtc)
Attachment #803254 - Flags: review?(wtc)
(Reporter)

Updated

5 years ago
Priority: P2 → P1
Target Milestone: 3.15.2 → 3.15.3
(Reporter)

Comment 14

5 years ago
Created attachment 812261 [details] [diff] [review]
Brian's patch, updated v2

I updated Brian's patch to the current NSS tip. I also included the newly
added SSL_PeerCertificateChain function.
Attachment #803254 - Attachment is obsolete: true
Attachment #803254 - Flags: review?(wtc)
Attachment #812261 - Flags: review?(wtc)
(Reporter)

Updated

5 years ago
Attachment #812261 - Attachment description: Brian's patch, updated → Brian's patch, updated v2
(Reporter)

Comment 15

5 years ago
Created attachment 812302 [details] [diff] [review]
Brian's patch, updated v2.1

The only difference from v2 should be tabs vs. spaces.
Attachment #812261 - Attachment is obsolete: true
Attachment #812261 - Flags: review?(wtc)
Attachment #812302 - Flags: review?(wtc)
(Reporter)

Comment 16

5 years ago
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 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

5 years ago
changing target milestone to 3.15.4
Target Milestone: 3.15.3 → 3.15.4
(Reporter)

Updated

4 years ago
Target Milestone: 3.15.4 → 3.16
Created attachment 8385377 [details] [diff] [review]
Patch rebased for NSS 3.16
Attachment #601479 - Attachment is obsolete: true
Attachment #812302 - Attachment is obsolete: true
Attachment #812302 - Flags: review?(wtc)
Attachment #8385377 - Flags: review?(wtc)
Assignee: wtc → brian
Created attachment 8385449 [details] [diff] [review]
Patch rebased for NSS 3.16

This addresses the previous review comments.
Attachment #8385377 - Attachment is obsolete: true
Attachment #8385449 - Flags: review?(wtc)
(Reporter)

Comment 21

4 years ago
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
Attachment #8385449 - Flags: review?(wtc) → review+
(Reporter)

Comment 22

4 years ago
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.
Target Milestone: 3.16 → 3.16.1
Assignee: brian → nobody
Target Milestone: 3.16.1 → ---
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.
Created attachment 8475560 [details] [diff] [review]
patch rebased for NSS 3.17

I'd appreciate if you would have a final look at this, Wan-Teh. Thanks!
Assignee: nobody → dkeeler
Attachment #8385449 - Attachment is obsolete: true
Attachment #8475560 - Flags: review?(wtc)

Updated

2 years ago
Blocks: 1049110

Comment 25

6 months ago
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?
Flags: needinfo?(ttaubert)
(Assignee)

Comment 26

6 months ago
Can I take this, please?
Flags: needinfo?(dkeeler)
Please do :)
Assignee: dkeeler → mgoodwin
Flags: needinfo?(dkeeler)
(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 :)
Flags: needinfo?(ttaubert)

Updated

6 months ago
Blocks: 1414358

Updated

3 months ago
Blocks: 1434936
You need to log in before you can comment on or make changes to this bug.