From 0184c4f9fb6c01465aec4088e2b3466afc92333b Mon Sep 17 00:00:00 2001 From: Dan Breslau Date: Fri, 30 Sep 2016 16:55:18 -0400 Subject: [PATCH] User should validate the server certificate, not the CA certificate --- libeap/src/crypto/tls.h | 8 +-- libeap/src/crypto/tls_openssl.c | 109 ++++++++++++++++++----------------- libeap/src/eap_peer/eap_config.h | 4 +- libeap/src/eap_peer/eap_tls_common.c | 6 +- mech_eap/init_sec_context.c | 8 ++- 5 files changed, 71 insertions(+), 64 deletions(-) diff --git a/libeap/src/crypto/tls.h b/libeap/src/crypto/tls.h index 7d80643..d02158e 100644 --- a/libeap/src/crypto/tls.h +++ b/libeap/src/crypto/tls.h @@ -143,9 +143,9 @@ struct X509; /* from OpenSSL */ * @flags: Parameter options (TLS_CONN_*) * @ocsp_stapling_response: DER encoded file with cached OCSP stapling response * or %NULL if OCSP is not enabled - * @validate_ca_cb: Optional callback to be used to validate server certificate + * @server_cert_cb: Optional callback to be used to validate server certificate * when no CA or path was specified. - * @validate_ca_ctx: Optional context arg for validate_ca_cb. + * @server_cert_ctx: Optional context arg for server_cert_cb. * * TLS connection parameters to be configured with tls_connection_set_params() * and tls_global_set_params(). @@ -191,8 +191,8 @@ struct tls_connection_params { * If non-null, specifies a callback method that can be used to * confirm the validity of a peer certificate. */ - int (*validate_ca_cb)(int ok_so_far, X509* cert, void *ca_ctx); - void *validate_ca_ctx; + int (*server_cert_cb)(int ok_so_far, X509* cert, void *ca_ctx); + void *server_cert_ctx; }; diff --git a/libeap/src/crypto/tls_openssl.c b/libeap/src/crypto/tls_openssl.c index e82e949..491182b 100644 --- a/libeap/src/crypto/tls_openssl.c +++ b/libeap/src/crypto/tls_openssl.c @@ -130,8 +130,8 @@ struct tls_connection { unsigned char server_random[SSL3_RANDOM_SIZE]; #endif - int (*validate_ca_cb)(int ok_so_far, X509* cert, void *ca_ctx); - void *validate_ca_ctx; + int (*server_cert_cb)(int ok_so_far, X509* cert, void *ca_ctx); + void *server_cert_ctx; }; @@ -1561,6 +1561,8 @@ static void openssl_tls_cert_event(struct tls_connection *conn, } +static void debug_print_cert(X509 *cert, const char *title); + static int tls_verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) { char buf[256]; @@ -1576,6 +1578,8 @@ static int tls_verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) if (!err_cert) return 0; + // debug_print_cert(err_cert, "\n\n***** tls_verify_cb:\n"); + err = X509_STORE_CTX_get_error(x509_ctx); depth = X509_STORE_CTX_get_error_depth(x509_ctx); ssl = X509_STORE_CTX_get_ex_data(x509_ctx, @@ -1594,9 +1598,9 @@ static int tls_verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) conn->peer_issuer_issuer = err_cert; wpa_printf(MSG_DEBUG, "TLS: tls_verify_cb(enter) - preverify_ok=%d " - "err=%d (%s) ca_cert_verify=%d depth=%d buf='%s'", + "err=%d (%s) ca_cert_verify=%d depth=%d buf='%s' server_cert_cb=%p server_cert_only=%d", preverify_ok, err, X509_verify_cert_error_string(err), - conn->ca_cert_verify, depth, buf); + conn->ca_cert_verify, depth, buf, conn->server_cert_cb, conn->server_cert_only); context = conn->context; @@ -1605,16 +1609,9 @@ static int tls_verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) suffix_match = conn->suffix_match; domain_match = conn->domain_match; - if (!preverify_ok && !conn->ca_cert_verify) { - if (conn->validate_ca_cb) { - preverify_ok = conn->validate_ca_cb(preverify_ok, err_cert, conn->validate_ca_ctx); - wpa_printf(MSG_DEBUG, "TLS: tls_verify_cb: validate_ca_cb returned %d", preverify_ok); - } - else { - preverify_ok = 1; - wpa_printf(MSG_DEBUG, "TLS: tls_verify_cb: allowing cert because !conn->ca_cert_verify\n"); - } - } + if (!preverify_ok && !conn->ca_cert_verify) + preverify_ok = 1; + if (!preverify_ok && depth > 0 && conn->server_cert_only) { wpa_printf(MSG_DEBUG, "TLS: tls_verify_cb: allowing cert because depth > 0 && conn->server_cert_only\n"); preverify_ok = 1; @@ -1630,40 +1627,46 @@ static int tls_verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) err_str = X509_verify_cert_error_string(err); #ifdef CONFIG_SHA256 - /* - * Do not require preverify_ok so we can explicity allow otherwise - * invalid pinned server certificates. - */ if (depth == 0 && conn->server_cert_only) { - struct wpabuf *cert; - cert = get_x509_cert(err_cert); - if (!cert) { - wpa_printf(MSG_DEBUG, "tls_verify_cb: OpenSSL: Could not fetch " - "server certificate data"); - preverify_ok = 0; - } else { - u8 hash[32]; - const u8 *addr[1]; - size_t len[1]; - addr[0] = wpabuf_head(cert); - len[0] = wpabuf_len(cert); - if (sha256_vector(1, addr, len, hash) < 0 || - os_memcmp(conn->srv_cert_hash, hash, 32) != 0) { - err_str = "Server certificate mismatch"; - err = X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN; - preverify_ok = 0; - } else if (!preverify_ok) { - /* - * Certificate matches pinned certificate, allow - * regardless of other problems. - */ - wpa_printf(MSG_DEBUG, - "tls_verify_cb: OpenSSL: Ignore validation issues for a pinned server certificate"); - preverify_ok = 1; - } - wpabuf_free(cert); - } - } + if (depth == 0 && conn->server_cert_cb) { + preverify_ok = conn->server_cert_cb(preverify_ok, err_cert, conn->server_cert_ctx); + wpa_printf(MSG_DEBUG, "TLS: tls_verify_cb: server_cert_cb returned %d", preverify_ok); + } + else { + /* + * Do not require preverify_ok so we can explicity allow otherwise + * invalid pinned server certificates. + */ + struct wpabuf *cert; + cert = get_x509_cert(err_cert); + if (!cert) { + wpa_printf(MSG_DEBUG, "tls_verify_cb: OpenSSL: Could not fetch " + "server certificate data"); + preverify_ok = 0; + } else { + u8 hash[32]; + const u8 *addr[1]; + size_t len[1]; + addr[0] = wpabuf_head(cert); + len[0] = wpabuf_len(cert); + if (sha256_vector(1, addr, len, hash) < 0 || + os_memcmp(conn->srv_cert_hash, hash, 32) != 0) { + err_str = "Server certificate mismatch"; + err = X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN; + preverify_ok = 0; + } else if (!preverify_ok) { + /* + * Certificate matches pinned certificate, allow + * regardless of other problems. + */ + wpa_printf(MSG_DEBUG, + "tls_verify_cb: OpenSSL: Ignore validation issues for a pinned server certificate"); + preverify_ok = 1; + } + wpabuf_free(cert); + } + } + } #endif /* CONFIG_SHA256 */ if (!preverify_ok) { @@ -1767,8 +1770,8 @@ static int tls_connection_ca_cert(struct tls_data *data, struct tls_connection *conn, const char *ca_cert, const u8 *ca_cert_blob, size_t ca_cert_blob_len, const char *ca_path, - int (*validate_ca_cb)(int ok_so_far, X509* cert, void *ca_ctx), - void *validate_ca_ctx) + int (*server_cert_cb)(int ok_so_far, X509* cert, void *ca_ctx), + void *server_cert_ctx) { SSL_CTX *ssl_ctx = data->ssl; X509_STORE *store; @@ -1787,6 +1790,8 @@ static int tls_connection_ca_cert(struct tls_data *data, SSL_set_verify(conn->ssl, SSL_VERIFY_PEER, tls_verify_cb); conn->ca_cert_verify = 1; + conn->server_cert_cb = server_cert_cb; + conn->server_cert_ctx = server_cert_ctx; if (ca_cert && os_strncmp(ca_cert, "probe://", 8) == 0) { wpa_printf(MSG_DEBUG, "OpenSSL: Probe for server certificate " @@ -1926,8 +1931,6 @@ static int tls_connection_ca_cert(struct tls_data *data, * certificate */ wpa_printf(MSG_DEBUG, "OpenSSL: tls_connection_ca_cert: No ca_cert; setting conn->ca_cert_verify=0"); conn->ca_cert_verify = 0; - conn->validate_ca_cb = validate_ca_cb; - conn->validate_ca_ctx = validate_ca_ctx; } return 0; @@ -3866,8 +3869,8 @@ int tls_connection_set_params(void *tls_ctx, struct tls_connection *conn, if (tls_connection_ca_cert(data, conn, params->ca_cert, params->ca_cert_blob, params->ca_cert_blob_len, - params->ca_path, params->validate_ca_cb, - params->validate_ca_ctx)) + params->ca_path, params->server_cert_cb, + params->server_cert_ctx)) return -1; } diff --git a/libeap/src/eap_peer/eap_config.h b/libeap/src/eap_peer/eap_config.h index bd446c7..0dc8ea8 100644 --- a/libeap/src/eap_peer/eap_config.h +++ b/libeap/src/eap_peer/eap_config.h @@ -785,8 +785,8 @@ struct eap_peer_config { * If non-null, specifies a callback method that can be used to * override the validity of a peer certificate. */ - int (*validate_ca_cb)(int ok_so_far, X509* cert, void *ca_ctx); - void *validate_ca_ctx; + int (*server_cert_cb)(int ok_so_far, X509* cert, void *ca_ctx); + void *server_cert_ctx; }; diff --git a/libeap/src/eap_peer/eap_tls_common.c b/libeap/src/eap_peer/eap_tls_common.c index e7cbe62..bc4482a 100644 --- a/libeap/src/eap_peer/eap_tls_common.c +++ b/libeap/src/eap_peer/eap_tls_common.c @@ -103,8 +103,8 @@ static void eap_tls_params_from_conf1(struct tls_connection_params *params, params->cert_id = config->cert_id; params->ca_cert_id = config->ca_cert_id; eap_tls_params_flags(params, config->phase1); - params->validate_ca_cb = config->validate_ca_cb; - params->validate_ca_ctx = config->validate_ca_ctx; + params->server_cert_cb = config->server_cert_cb; + params->server_cert_ctx = config->server_cert_ctx; } @@ -128,6 +128,8 @@ static void eap_tls_params_from_conf2(struct tls_connection_params *params, params->cert_id = config->cert2_id; params->ca_cert_id = config->ca_cert2_id; eap_tls_params_flags(params, config->phase2); + params->server_cert_cb = config->server_cert_cb; + params->server_cert_ctx = config->server_cert_ctx; } diff --git a/mech_eap/init_sec_context.c b/mech_eap/init_sec_context.c index de308b6..c4769d7 100644 --- a/mech_eap/init_sec_context.c +++ b/mech_eap/init_sec_context.c @@ -429,8 +429,9 @@ static int peerValidateCA(int ok_so_far, X509* cert, void *ca_ctx) MoonshotError *error = NULL; struct eap_peer_config *eap_config = (struct eap_peer_config *) ca_ctx; char *identity = strdup((const char *) eap_config->identity); - char* at = strchr(identity, '@'); + // Truncate the identity to just the username + char* at = strchr(identity, '@'); if (at != NULL) { *at = '\0'; } @@ -450,6 +451,7 @@ static int peerValidateCA(int ok_so_far, X509* cert, void *ca_ctx) realm = ((char *) eap_config->anonymous_identity) + 1; ok_so_far = moonshot_confirm_ca_certificate(identity, realm, hash, 32, &error); + free(identity); printf("peerValidateCA: Returning %d\n", ok_so_far); return ok_so_far; @@ -564,8 +566,8 @@ peerConfigInit(OM_uint32 *minor, gss_ctx_id_t ctx) eapPeerConfig->private_key_passwd = (char *)cred->password.value; } - eapPeerConfig->validate_ca_cb = peerValidateCA; - eapPeerConfig->validate_ca_ctx = eapPeerConfig; + eapPeerConfig->server_cert_cb = peerValidateCA; + eapPeerConfig->server_cert_ctx = eapPeerConfig; *minor = 0; return GSS_S_COMPLETE; -- 2.1.4