Preserve session->sess_cert on ticket renewal.
Turns out the safer/simpler method still wasn't quite right. :-)
session->sess_cert isn't serialized and deserialized, which is poor. Duplicate
it manually for now. Leave a TODO to get rid of that field altogether as it's
not especially helpful. The certificate-related fields should be in the
session. The others probably have no reason to be preserved on resumptions at
all.
Test by making bssl_shim.cc assert the peer cert chain is there or not as
expected.
BUG=501220
Change-Id: I44034167629720d6e2b7b0b938d58bcab3ab0abe
Reviewed-on: https://boringssl-review.googlesource.com/5170
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index caac446..7e278ae 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -650,7 +650,12 @@
* disable session caching and tickets. */
int not_resumable;
- /* The cert is the certificate used to establish this connection */
+ /* The cert is the certificate used to establish this connection
+ *
+ * TODO(davidben): Remove this field. It is not serialized as part of the
+ * session, but some APIs access it. Certificate-related fields, where not
+ * redundant with |peer|, should be added to the session. Others should
+ * probably not be retained across resumptions. */
struct sess_cert_st /* SESS_CERT */ *sess_cert;
/* This is the cert for the other end. On clients, it will be the same as
diff --git a/ssl/internal.h b/ssl/internal.h
index 074db0d..00eccfa 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -797,7 +797,8 @@
void ssl_cert_clear_certs(CERT *c);
void ssl_cert_free(CERT *c);
SESS_CERT *ssl_sess_cert_new(void);
-void ssl_sess_cert_free(SESS_CERT *sc);
+SESS_CERT *ssl_sess_cert_dup(const SESS_CERT *sess_cert);
+void ssl_sess_cert_free(SESS_CERT *sess_cert);
int ssl_get_new_session(SSL *s, int session);
int ssl_get_prev_session(SSL *s, const struct ssl_early_callback_ctx *ctx);
STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, const CBS *cbs);
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 90b59c5..8d192b6 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -1517,6 +1517,15 @@
OPENSSL_PUT_ERROR(SSL, ssl3_get_new_session_ticket, ERR_R_INTERNAL_ERROR);
goto err;
}
+ if (s->session->sess_cert != NULL) {
+ /* |sess_cert| is not serialized and must be duplicated explicitly. */
+ assert(new_session->sess_cert == NULL);
+ new_session->sess_cert = ssl_sess_cert_dup(s->session->sess_cert);
+ if (new_session->sess_cert == NULL) {
+ SSL_SESSION_free(new_session);
+ goto err;
+ }
+ }
SSL_SESSION_free(s->session);
s->session = new_session;
diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index f27685d..85aa079 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -119,11 +119,13 @@
#include <openssl/bio.h>
#include <openssl/bn.h>
#include <openssl/buf.h>
+#include <openssl/ec_key.h>
#include <openssl/dh.h>
#include <openssl/err.h>
#include <openssl/mem.h>
#include <openssl/obj.h>
#include <openssl/pem.h>
+#include <openssl/x509.h>
#include <openssl/x509v3.h>
#include "../crypto/dh/internal.h"
@@ -413,17 +415,44 @@
return ret;
}
-void ssl_sess_cert_free(SESS_CERT *sc) {
- if (sc == NULL) {
+SESS_CERT *ssl_sess_cert_dup(const SESS_CERT *sess_cert) {
+ SESS_CERT *ret = ssl_sess_cert_new();
+ if (ret == NULL) {
+ return NULL;
+ }
+
+ if (sess_cert->cert_chain != NULL) {
+ ret->cert_chain = X509_chain_up_ref(sess_cert->cert_chain);
+ if (ret->cert_chain == NULL) {
+ ssl_sess_cert_free(ret);
+ return NULL;
+ }
+ }
+ if (sess_cert->peer_cert != NULL) {
+ ret->peer_cert = X509_up_ref(sess_cert->peer_cert);
+ }
+ if (sess_cert->peer_dh_tmp != NULL) {
+ ret->peer_dh_tmp = sess_cert->peer_dh_tmp;
+ DH_up_ref(ret->peer_dh_tmp);
+ }
+ if (sess_cert->peer_ecdh_tmp != NULL) {
+ ret->peer_ecdh_tmp = sess_cert->peer_ecdh_tmp;
+ EC_KEY_up_ref(ret->peer_ecdh_tmp);
+ }
+ return ret;
+}
+
+void ssl_sess_cert_free(SESS_CERT *sess_cert) {
+ if (sess_cert == NULL) {
return;
}
- sk_X509_pop_free(sc->cert_chain, X509_free);
- X509_free(sc->peer_cert);
- DH_free(sc->peer_dh_tmp);
- EC_KEY_free(sc->peer_ecdh_tmp);
+ sk_X509_pop_free(sess_cert->cert_chain, X509_free);
+ X509_free(sess_cert->peer_cert);
+ DH_free(sess_cert->peer_dh_tmp);
+ EC_KEY_free(sess_cert->peer_ecdh_tmp);
- OPENSSL_free(sc);
+ OPENSSL_free(sess_cert);
}
int ssl_verify_cert_chain(SSL *s, STACK_OF(X509) *sk) {
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 40cb149..3b95d7e 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -838,6 +838,20 @@
return false;
}
}
+
+ if (!config->is_server) {
+ /* Clients should expect a peer certificate chain iff this was not a PSK
+ * cipher suite. */
+ if (config->psk.empty()) {
+ if (SSL_get_peer_cert_chain(ssl.get()) == nullptr) {
+ fprintf(stderr, "Missing peer certificate chain!\n");
+ return false;
+ }
+ } else if (SSL_get_peer_cert_chain(ssl.get()) != nullptr) {
+ fprintf(stderr, "Unexpected peer certificate chain!\n");
+ return false;
+ }
+ }
}
if (config->export_keying_material > 0) {
