Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(652)

Unified Diff: net/socket/ssl_client_socket_openssl.cc

Issue 203323004: Do not assume OSCertHandle is OpenSSL's X509 structure. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Addressed wtc's comments Created 6 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index d04670fc14fcab9b1e7465a3525886b6b798b03a..f97c4697cfe6fb30e8801a9193afc51d5e6fca30 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -332,16 +332,17 @@ class SSLClientSocketOpenSSL::SSLContext {
// and the other elements are in the order given by the server.
class SSLClientSocketOpenSSL::PeerCertificateChain {
public:
- explicit PeerCertificateChain(SSL* ssl) { Reset(ssl); }
+ explicit PeerCertificateChain(STACK_OF(X509)* chain) { Reset(chain); }
PeerCertificateChain(const PeerCertificateChain& other) { *this = other; }
~PeerCertificateChain() {}
PeerCertificateChain& operator=(const PeerCertificateChain& other);
- // Resets the PeerCertificateChain to the set of certificates supplied by the
- // peer of |ssl|, which may be NULL, indicating to empty the store
- // certificates. Note: If an error occurs, such as being unable to parse the
- // certificates, this will behave as if Reset(NULL) was called.
- void Reset(SSL* ssl);
+ // Resets the PeerCertificateChain to the set of certificates in|chain|,
+ // which may be NULL, indicating to empty the store certificates.
+ // Note: If an error occurs, such as being unable to parse the certificates,
+ // this will behave as if Reset(NULL) was called.
+ void Reset(STACK_OF(X509)* chain);
+
// Note that when USE_OPENSSL is defined, OSCertHandle is X509*
const scoped_refptr<X509Certificate>& AsOSChain() const { return os_chain_; }
@@ -356,6 +357,8 @@ class SSLClientSocketOpenSSL::PeerCertificateChain {
return sk_X509_value(openssl_chain_.get(), index);
}
+ bool IsValid() { return os_chain_.get() && openssl_chain_.get(); }
+
private:
static void FreeX509Stack(STACK_OF(X509)* cert_chain) {
sk_X509_pop_free(cert_chain, X509_free);
@@ -389,14 +392,11 @@ SSLClientSocketOpenSSL::PeerCertificateChain::operator=(
#if defined(USE_OPENSSL)
// When OSCertHandle is typedef'ed to X509, this implementation does a short cut
// to avoid converting back and forth between der and X509 struct.
-void SSLClientSocketOpenSSL::PeerCertificateChain::Reset(SSL* ssl) {
+void SSLClientSocketOpenSSL::PeerCertificateChain::Reset(
+ STACK_OF(X509)* chain) {
openssl_chain_.reset(NULL);
os_chain_ = NULL;
- if (ssl == NULL)
- return;
-
- STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl);
if (!chain)
return;
@@ -418,14 +418,11 @@ void SSLClientSocketOpenSSL::PeerCertificateChain::Reset(SSL* ssl) {
}
}
#else // !defined(USE_OPENSSL)
-void SSLClientSocketOpenSSL::PeerCertificateChain::Reset(SSL* ssl) {
+void SSLClientSocketOpenSSL::PeerCertificateChain::Reset(
+ STACK_OF(X509)* chain) {
openssl_chain_.reset(NULL);
os_chain_ = NULL;
- if (ssl == NULL)
- return;
-
- STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl);
if (!chain)
return;
@@ -1058,8 +1055,12 @@ void SSLClientSocketOpenSSL::DoConnectCallback(int rv) {
}
X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
- server_cert_chain_->Reset(ssl_);
+ server_cert_chain_->Reset(SSL_get_peer_cert_chain(ssl_));
server_cert_ = server_cert_chain_->AsOSChain();
+
+ if (!server_cert_chain_->IsValid())
+ DVLOG(1) << "UpdateServerCert received invalid certificate chain from peer";
+
return server_cert_.get();
}
@@ -1502,12 +1503,16 @@ int SSLClientSocketOpenSSL::CertVerifyCallback(X509_STORE_CTX* store_ctx) {
return 1;
}
- if (X509Certificate::IsSameOSCert(server_cert_->os_cert_handle(),
- sk_X509_value(store_ctx->untrusted, 0))) {
+ CHECK(server_cert_.get());
+
+ PeerCertificateChain chain(store_ctx->chain);
+ if (chain.IsValid() && server_cert_->Equals(chain.AsOSChain()))
return 1;
- }
- LOG(ERROR) << "Server certificate changed between handshakes";
+ if (!chain.IsValid())
+ LOG(ERROR) << "Received invalid certificate chain between handshakes";
+ else
+ LOG(ERROR) << "Server certificate changed between handshakes";
return 0;
}
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698