Attention is currently required from: Arne Schwabe.
Hello Arne Schwabe,
I'd like you to do a code review.
Please visit
https://gerrit.openvpn.net/c/openvpn/+/2145?usp=email
to review the following change.
Change subject: ssl_verify: share fingerprint and x509 env helpers between
backends
......................................................................
ssl_verify: share fingerprint and x509 env helpers between backends
Replace x509_get_sha1_fingerprint() and x509_get_sha256_fingerprint()
with x509_get_fingerprint(cert, algo, gc) in both backends. This also
removes the switch in verify_cert(), whose default case could not be
reached, as verify_hash_algo is always MD_SHA1 or MD_SHA256. The
backends now switch over the enum without a default, so a new
algorithm gets a -Wswitch warning, and ASSERT on unknown values.
Move the identical do_setenv_x509() copies from the backends to
ssl_verify.c, with a stub for test_ssl as for verify_cert().
Change-Id: I76cd424b34e91f07a6d187b23a258d9dcbf7ed0f
Signed-off-by: Razvan Cojocaru <[email protected]>
---
M src/openvpn/ssl_verify.c
M src/openvpn/ssl_verify_backend.h
M src/openvpn/ssl_verify_mbedtls.c
M src/openvpn/ssl_verify_openssl.c
M tests/unit_tests/openvpn/mock_ssl_dependencies.c
5 files changed, 85 insertions(+), 131 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/45/2145/1
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 0cdec1e..e516fb0 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -53,6 +53,22 @@
string_mod(str, CC_PRINT, CC_CRLF, '_');
}
+void
+do_setenv_x509(struct env_set *es, const char *name, char *value, int depth)
+{
+ char *name_expand;
+ size_t name_expand_size;
+
+ string_mod(value, CC_ANY, CC_CRLF, '?');
+ msg(D_X509_ATTR, "X509 ATTRIBUTE name='%s' value='%s' depth=%d", name,
value, depth);
+ name_expand_size = 64 + strlen(name);
+ name_expand = (char *)malloc(name_expand_size);
+ check_malloc_return(name_expand);
+ snprintf(name_expand, name_expand_size, "X509_%d_%s", depth, name);
+ setenv_str(es, name_expand, value);
+ free(name_expand);
+}
+
/*
* Export the untrusted IP address and port to the environment
*/
@@ -428,8 +444,8 @@
/* export X509 cert fingerprints */
{
- struct buffer sha1 = x509_get_sha1_fingerprint(peer_cert, &gc);
- struct buffer sha256 = x509_get_sha256_fingerprint(peer_cert, &gc);
+ struct buffer sha1 = x509_get_fingerprint(peer_cert, MD_SHA1, &gc);
+ struct buffer sha256 = x509_get_fingerprint(peer_cert, MD_SHA256, &gc);
snprintf(envname, sizeof(envname), "tls_digest_%d", cert_depth);
setenv_str(es, envname, format_hex_ex(BPTR(&sha1), BLEN(&sha1), 0, 1,
":", &gc));
@@ -671,32 +687,7 @@
if (cert_depth == opt->verify_hash_depth && opt->verify_hash)
{
- struct buffer cert_fp = { 0 };
-
- switch (opt->verify_hash_algo)
- {
- case MD_SHA1:
- cert_fp = x509_get_sha1_fingerprint(cert, &gc);
- break;
-
- case MD_SHA256:
- cert_fp = x509_get_sha256_fingerprint(cert, &gc);
- break;
-
- default:
- /* This should normally not happen at all; the algorithm used
- * is parsed by add_option() [options.c] and set to a
predefined
- * value in an enumerated type. So if this unlikely scenario
- * happens, consider this a failure
- */
- msg(M_WARN,
- "Unexpected invalid algorithm used with "
- "--verify-hash (%i)",
- opt->verify_hash_algo);
- ret = FAILURE;
- goto cleanup;
- }
-
+ struct buffer cert_fp = x509_get_fingerprint(cert,
opt->verify_hash_algo, &gc);
const struct verify_hash_list *current_hash = opt->verify_hash;
while (current_hash)
diff --git a/src/openvpn/ssl_verify_backend.h b/src/openvpn/ssl_verify_backend.h
index 46720d6..93916b7 100644
--- a/src/openvpn/ssl_verify_backend.h
+++ b/src/openvpn/ssl_verify_backend.h
@@ -75,6 +75,17 @@
const struct buffer *cert_hash);
/*
+ * Export an X509 attribute as the X509_<depth>_<name> environment variable,
+ * replacing CR and LF in value with '?'.
+ *
+ * @param es Environment set to add the variable to
+ * @param name Attribute name
+ * @param value Attribute value, modified in place
+ * @param depth Depth of the certificate the attribute belongs to
+ */
+void do_setenv_x509(struct env_set *es, const char *name, char *value, int
depth);
+
+/*
* Library-specific functions.
*
* The following functions must be implemented on a library-specific basis.
@@ -91,24 +102,16 @@
char *x509_get_subject(openvpn_x509_cert_t *cert, struct gc_arena *gc);
/**
- * Retrieve the certificate's SHA1 fingerprint.
+ * Retrieve the certificate's fingerprint.
*
* @param cert Certificate to retrieve the fingerprint from.
+ * @param algo Hash algorithm to use.
* @param gc Garbage collection arena to use when allocating string.
*
- * @return a string containing the certificate fingerprint
+ * @return a buffer containing the certificate fingerprint
*/
-struct buffer x509_get_sha1_fingerprint(openvpn_x509_cert_t *cert, struct
gc_arena *gc);
-
-/**
- * Retrieve the certificate's SHA256 fingerprint.
- *
- * @param cert Certificate to retrieve the fingerprint from.
- * @param gc Garbage collection arena to use when allocating string.
- *
- * @return a string containing the certificate fingerprint
- */
-struct buffer x509_get_sha256_fingerprint(openvpn_x509_cert_t *cert, struct
gc_arena *gc);
+struct buffer x509_get_fingerprint(openvpn_x509_cert_t *cert, hash_algo_type
algo,
+ struct gc_arena *gc);
/*
* Retrieve the certificate's username from the specified field.
diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
index a132f12..cc4afb3 100644
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -55,7 +55,7 @@
session->verified = false;
/* Remember certificate hash */
- struct buffer cert_fingerprint = x509_get_sha256_fingerprint(cert, &gc);
+ struct buffer cert_fingerprint = x509_get_fingerprint(cert, MD_SHA256,
&gc);
cert_hash_remember(session, cert_depth, &cert_fingerprint);
if (session->opt->verify_hash_no_ca)
@@ -502,9 +502,23 @@
return FAILURE;
}
-static struct buffer
-x509_get_fingerprint(const mbedtls_md_info_t *md_info, mbedtls_x509_crt *cert,
struct gc_arena *gc)
+struct buffer
+x509_get_fingerprint(mbedtls_x509_crt *cert, hash_algo_type algo, struct
gc_arena *gc)
{
+ mbedtls_md_type_t md_type = MBEDTLS_MD_NONE;
+ switch (algo)
+ {
+ case MD_SHA1:
+ md_type = MBEDTLS_MD_SHA1;
+ break;
+
+ case MD_SHA256:
+ md_type = MBEDTLS_MD_SHA256;
+ break;
+ }
+ ASSERT(md_type != MBEDTLS_MD_NONE);
+
+ const mbedtls_md_info_t *md_info = mbedtls_md_info_from_type(md_type);
const size_t md_size = mbedtls_md_get_size(md_info);
struct buffer fingerprint = alloc_buf_gc(md_size, gc);
mbedtls_md(md_info, cert->raw.p, cert->raw.len, BPTR(&fingerprint));
@@ -512,18 +526,6 @@
return fingerprint;
}
-struct buffer
-x509_get_sha1_fingerprint(mbedtls_x509_crt *cert, struct gc_arena *gc)
-{
- return x509_get_fingerprint(mbedtls_md_info_from_type(MBEDTLS_MD_SHA1),
cert, gc);
-}
-
-struct buffer
-x509_get_sha256_fingerprint(mbedtls_x509_crt *cert, struct gc_arena *gc)
-{
- return x509_get_fingerprint(mbedtls_md_info_from_type(MBEDTLS_MD_SHA256),
cert, gc);
-}
-
char *
x509_get_subject(mbedtls_x509_crt *cert, struct gc_arena *gc)
{
@@ -542,22 +544,6 @@
return subject;
}
-static void
-do_setenv_x509(struct env_set *es, const char *name, char *value, int depth)
-{
- char *name_expand;
- size_t name_expand_size;
-
- string_mod(value, CC_ANY, CC_CRLF, '?');
- msg(D_X509_ATTR, "X509 ATTRIBUTE name='%s' value='%s' depth=%d", name,
value, depth);
- name_expand_size = 64 + strlen(name);
- name_expand = (char *)malloc(name_expand_size);
- check_malloc_return(name_expand);
- snprintf(name_expand, name_expand_size, "X509_%d_%s", depth, name);
- setenv_str(es, name_expand, value);
- free(name_expand);
-}
-
static char *
asn1_buf_to_c_string(const mbedtls_asn1_buf *orig, struct gc_arena *gc)
{
@@ -701,22 +687,13 @@
{
if (depth == 0 || (xt->flags & XT_FULL_CHAIN))
{
- if (0 == strcmp(xt->name, "SHA1") || 0 == strcmp(xt->name,
"SHA256"))
+ bool sha1 = strcmp(xt->name, "SHA1") == 0;
+ if (sha1 || strcmp(xt->name, "SHA256") == 0)
{
/* Fingerprint is not part of X509 structure */
- struct buffer cert_hash;
- char *fingerprint;
-
- if (0 == strcmp(xt->name, "SHA1"))
- {
- cert_hash = x509_get_sha1_fingerprint(cert, &gc);
- }
- else
- {
- cert_hash = x509_get_sha256_fingerprint(cert, &gc);
- }
-
- fingerprint =
+ hash_algo_type algo = sha1 ? MD_SHA1 : MD_SHA256;
+ struct buffer cert_hash = x509_get_fingerprint(cert, algo,
&gc);
+ char *fingerprint =
format_hex_ex(BPTR(&cert_hash), BLEN(&cert_hash), 0, 1 |
FHE_CAPS, ":", &gc);
do_setenv_x509(es, xt->name, fingerprint, depth);
}
diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 0c36cf2..ad02bb0 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -61,7 +61,7 @@
ASSERT(session);
X509 *current_cert = X509_STORE_CTX_get_current_cert(ctx);
- struct buffer cert_hash = x509_get_sha256_fingerprint(current_cert, &gc);
+ struct buffer cert_hash = x509_get_fingerprint(current_cert, MD_SHA256,
&gc);
cert_hash_remember(session, X509_STORE_CTX_get_error_depth(ctx),
&cert_hash);
/* did peer present cert which was signed by our root cert? */
@@ -356,22 +356,25 @@
}
struct buffer
-x509_get_sha1_fingerprint(X509 *cert, struct gc_arena *gc)
+x509_get_fingerprint(X509 *cert, hash_algo_type algo, struct gc_arena *gc)
{
- const EVP_MD *sha1 = EVP_sha1();
- struct buffer hash = alloc_buf_gc((size_t)EVP_MD_size(sha1), gc);
- X509_digest(cert, EVP_sha1(), BPTR(&hash), NULL);
- ASSERT(buf_inc_len(&hash, (int)EVP_MD_size(sha1)));
- return hash;
-}
+ const EVP_MD *md = NULL;
+ switch (algo)
+ {
+ case MD_SHA1:
+ md = EVP_sha1();
+ break;
-struct buffer
-x509_get_sha256_fingerprint(X509 *cert, struct gc_arena *gc)
-{
- const EVP_MD *sha256 = EVP_sha256();
- struct buffer hash = alloc_buf_gc((size_t)EVP_MD_size(sha256), gc);
- X509_digest(cert, EVP_sha256(), BPTR(&hash), NULL);
- ASSERT(buf_inc_len(&hash, (int)EVP_MD_size(sha256)));
+ case MD_SHA256:
+ md = EVP_sha256();
+ break;
+ }
+ ASSERT(md);
+
+ const int md_size = EVP_MD_size(md);
+ struct buffer hash = alloc_buf_gc((size_t)md_size, gc);
+ X509_digest(cert, md, BPTR(&hash), NULL);
+ ASSERT(buf_inc_len(&hash, md_size));
return hash;
}
@@ -464,23 +467,6 @@
}
}
-/* worker method for setenv_x509_track */
-static void
-do_setenv_x509(struct env_set *es, const char *name, char *value, int depth)
-{
- char *name_expand;
- size_t name_expand_size;
-
- string_mod(value, CC_ANY, CC_CRLF, '?');
- msg(D_X509_ATTR, "X509 ATTRIBUTE name='%s' value='%s' depth=%d", name,
value, depth);
- name_expand_size = 64 + strlen(name);
- name_expand = (char *)malloc(name_expand_size);
- check_malloc_return(name_expand);
- snprintf(name_expand, name_expand_size, "X509_%d_%s", depth, name);
- setenv_str(es, name_expand, value);
- free(name_expand);
-}
-
void
x509_setenv_track(const struct x509_track *xt, struct env_set *es, const int
depth, X509 *x509)
{
@@ -502,19 +488,10 @@
case NID_sha1:
case NID_sha256:
{
- struct buffer fp_buf;
- char *fp_str = NULL;
-
- if (xt->nid == NID_sha1)
- {
- fp_buf = x509_get_sha1_fingerprint(x509, &gc);
- }
- else
- {
- fp_buf = x509_get_sha256_fingerprint(x509, &gc);
- }
-
- fp_str = format_hex_ex(BPTR(&fp_buf), BLEN(&fp_buf), 0, 1
| FHE_CAPS, ":", &gc);
+ hash_algo_type algo = xt->nid == NID_sha1 ? MD_SHA1 :
MD_SHA256;
+ struct buffer fp_buf = x509_get_fingerprint(x509, algo,
&gc);
+ char *fp_str =
+ format_hex_ex(BPTR(&fp_buf), BLEN(&fp_buf), 0, 1 |
FHE_CAPS, ":", &gc);
do_setenv_x509(es, xt->name, fp_str, depth);
}
break;
diff --git a/tests/unit_tests/openvpn/mock_ssl_dependencies.c
b/tests/unit_tests/openvpn/mock_ssl_dependencies.c
index b22dd89..65e4635 100644
--- a/tests/unit_tests/openvpn/mock_ssl_dependencies.c
+++ b/tests/unit_tests/openvpn/mock_ssl_dependencies.c
@@ -64,3 +64,9 @@
{
return FAILURE;
}
+
+void
+do_setenv_x509(struct env_set *es, const char *name, char *value, int depth)
+{
+ assert_false(true);
+}
--
To view, visit https://gerrit.openvpn.net/c/openvpn/+/2145?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76cd424b34e91f07a6d187b23a258d9dcbf7ed0f
Gerrit-Change-Number: 2145
Gerrit-PatchSet: 1
Gerrit-Owner: Răzvan Cojocaru <[email protected]>
Gerrit-Reviewer: Arne Schwabe <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: Arne Schwabe <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
