You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@openoffice.apache.org by tr...@apache.org on 2020/09/30 08:44:09 UTC
[openoffice] 02/02: Fix handling of NUL characters in certificate
fields
This is an automated email from the ASF dual-hosted git repository.
truckman pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/openoffice.git
commit 202391d17634db7776868942325c45a7836d68df
Author: Don Lewis <tr...@apache.org>
AuthorDate: Tue Sep 29 22:02:50 2020 -0700
Fix handling of NUL characters in certificate fields
A flaw was found in the way Serf handled NUL characters in the CommonName
and SubjectAltNames fields of X.509 certificates. An attacker able to
get a carefully-crafted certificate signed by a trusted Certificate
Authority could trick applications using Serf (such as Subversion on
Fedora 20 and later, refer also to bug 1127063) into accepting it by
mistake, allowing the attacker to perform a man-in-the-middle attack.
Patch by: Ben Reser of WANdisco via Serf Project and Apache Serf
---
ext_libraries/serf/NULbytes.patch | 313 ++++++++++++++++++++++++++++++++++++++
ext_libraries/serf/makefile.mk | 2 +-
2 files changed, 314 insertions(+), 1 deletion(-)
diff --git a/ext_libraries/serf/NULbytes.patch b/ext_libraries/serf/NULbytes.patch
new file mode 100644
index 0000000..12593d9
--- /dev/null
+++ b/ext_libraries/serf/NULbytes.patch
@@ -0,0 +1,313 @@
+------------------------------------------------------------------------
+r1699925 | breser | 2014-08-04 11:04:00 -0700 (Mon, 04 Aug 2014) | 5 lines
+
+On the 1.3.x branch:
+
+Merge changes from trunk:
+- r2392: Handle NUL bytes in fields of X.509 certs.
+
+------------------------------------------------------------------------
+Index: misc/build/serf-1.2.1/buckets/ssl_buckets.c
+===================================================================
+--- misc/serf-1.2.1/buckets/ssl_buckets.c (revision 1699924)
++++ misc/build/serf-1.2.1/buckets/ssl_buckets.c (revision 1699925)
+@@ -202,6 +202,8 @@
+ };
+
+ static void disable_compression(serf_ssl_context_t *ssl_ctx);
++static char *
++ pstrdup_escape_nul_bytes(const char *buf, int len, apr_pool_t *pool);
+
+ #if SSL_VERBOSE
+ /* Log all ssl alerts that we receive from the server. */
+@@ -427,6 +429,81 @@
+ #endif
+ };
+
++typedef enum san_copy_t {
++ EscapeNulAndCopy = 0,
++ ErrorOnNul = 1,
++} san_copy_t;
++
++
++static apr_status_t
++get_subject_alt_names(apr_array_header_t **san_arr, X509 *ssl_cert,
++ san_copy_t copy_action, apr_pool_t *pool)
++{
++ STACK_OF(GENERAL_NAME) *names;
++
++ /* assert: copy_action == ErrorOnNul || (san_arr && pool) */
++
++ /* Get subjectAltNames */
++ names = X509_get_ext_d2i(ssl_cert, NID_subject_alt_name, NULL, NULL);
++ if (names) {
++ int names_count = sk_GENERAL_NAME_num(names);
++ int name_idx;
++
++ if (san_arr)
++ *san_arr = apr_array_make(pool, names_count, sizeof(char*));
++ for (name_idx = 0; name_idx < names_count; name_idx++) {
++ char *p = NULL;
++ GENERAL_NAME *nm = sk_GENERAL_NAME_value(names, name_idx);
++
++ switch (nm->type) {
++ case GEN_DNS:
++ if (copy_action == ErrorOnNul &&
++ strlen(nm->d.ia5->data) != nm->d.ia5->length)
++ return SERF_ERROR_SSL_CERT_FAILED;
++ if (san_arr && *san_arr)
++ p = pstrdup_escape_nul_bytes((const char *)nm->d.ia5->data,
++ nm->d.ia5->length,
++ pool);
++ break;
++ default:
++ /* Don't know what to do - skip. */
++ break;
++ }
++
++ if (p) {
++ APR_ARRAY_PUSH(*san_arr, char*) = p;
++ }
++ }
++ sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free);
++ }
++
++ return APR_SUCCESS;
++}
++
++static apr_status_t validate_cert_hostname(X509 *server_cert, apr_pool_t *pool)
++{
++ char buf[1024];
++ int length;
++ apr_status_t ret;
++
++ ret = get_subject_alt_names(NULL, server_cert, ErrorOnNul, NULL);
++ if (ret) {
++ return ret;
++ } else {
++ /* Fail if the subject's CN field contains \0 characters. */
++ X509_NAME *subject = X509_get_subject_name(server_cert);
++ if (!subject)
++ return SERF_ERROR_SSL_CERT_FAILED;
++
++ length = X509_NAME_get_text_by_NID(subject, NID_commonName, buf, 1024);
++ if (length != -1)
++ if (strlen(buf) != length)
++ return SERF_ERROR_SSL_CERT_FAILED;
++ }
++
++ return APR_SUCCESS;
++}
++
+ static int
+ validate_server_certificate(int cert_valid, X509_STORE_CTX *store_ctx)
+ {
+@@ -435,6 +512,7 @@
+ X509 *server_cert;
+ int err, depth;
+ int failures = 0;
++ apr_status_t status;
+
+ ssl = X509_STORE_CTX_get_ex_data(store_ctx,
+ SSL_get_ex_data_X509_STORE_CTX_idx());
+@@ -475,6 +553,11 @@
+ }
+ }
+
++ /* Validate hostname */
++ status = validate_cert_hostname(server_cert, ctx->pool);
++ if (status)
++ failures |= SERF_SSL_CERT_UNKNOWN_FAILURE;
++
+ /* Check certificate expiry dates. */
+ if (X509_cmp_current_time(X509_get_notBefore(server_cert)) >= 0) {
+ failures |= SERF_SSL_CERT_NOTYETVALID;
+@@ -485,7 +568,6 @@
+
+ if (ctx->server_cert_callback &&
+ (depth == 0 || failures)) {
+- apr_status_t status;
+ serf_ssl_certificate_t *cert;
+ apr_pool_t *subpool;
+
+@@ -512,7 +594,6 @@
+
+ if (ctx->server_cert_chain_callback
+ && (depth == 0 || failures)) {
+- apr_status_t status;
+ STACK_OF(X509) *chain;
+ const serf_ssl_certificate_t **certs;
+ int certs_len;
+@@ -1461,7 +1542,50 @@
+
+ /* Functions to read a serf_ssl_certificate structure. */
+
+-/* Creates a hash_table with keys (E, CN, OU, O, L, ST and C). */
++/* Takes a counted length string and escapes any NUL bytes so that
++ * it can be used as a C string. NUL bytes are escaped as 3 characters
++ * "\00" (that's a literal backslash).
++ * The returned string is allocated in POOL.
++ */
++static char *
++pstrdup_escape_nul_bytes(const char *buf, int len, apr_pool_t *pool)
++{
++ int i, nul_count = 0;
++ char *ret;
++
++ /* First determine if there are any nul bytes in the string. */
++ for (i = 0; i < len; i++) {
++ if (buf[i] == '\0')
++ nul_count++;
++ }
++
++ if (nul_count == 0) {
++ /* There aren't so easy case to just copy the string */
++ ret = apr_pstrdup(pool, buf);
++ } else {
++ /* There are so we have to replace nul bytes with escape codes
++ * Proper length is the length of the original string, plus
++ * 2 times the number of nulls (for two digit hex code for
++ * the value) + the trailing null. */
++ char *pos;
++ ret = pos = apr_palloc(pool, len + 2 * nul_count + 1);
++ for (i = 0; i < len; i++) {
++ if (buf[i] != '\0') {
++ *(pos++) = buf[i];
++ } else {
++ *(pos++) = '\\';
++ *(pos++) = '0';
++ *(pos++) = '0';
++ }
++ }
++ *pos = '\0';
++ }
++
++ return ret;
++}
++
++/* Creates a hash_table with keys (E, CN, OU, O, L, ST and C). Any NUL bytes in
++ these fields in the certificate will be escaped as \00. */
+ static apr_hash_t *
+ convert_X509_NAME_to_table(X509_NAME *org, apr_pool_t *pool)
+ {
+@@ -1474,37 +1598,44 @@
+ NID_commonName,
+ buf, 1024);
+ if (ret != -1)
+- apr_hash_set(tgt, "CN", APR_HASH_KEY_STRING, apr_pstrdup(pool, buf));
++ apr_hash_set(tgt, "CN", APR_HASH_KEY_STRING,
++ pstrdup_escape_nul_bytes(buf, ret, pool));
+ ret = X509_NAME_get_text_by_NID(org,
+ NID_pkcs9_emailAddress,
+ buf, 1024);
+ if (ret != -1)
+- apr_hash_set(tgt, "E", APR_HASH_KEY_STRING, apr_pstrdup(pool, buf));
++ apr_hash_set(tgt, "E", APR_HASH_KEY_STRING,
++ pstrdup_escape_nul_bytes(buf, ret, pool));
+ ret = X509_NAME_get_text_by_NID(org,
+ NID_organizationalUnitName,
+ buf, 1024);
+ if (ret != -1)
+- apr_hash_set(tgt, "OU", APR_HASH_KEY_STRING, apr_pstrdup(pool, buf));
++ apr_hash_set(tgt, "OU", APR_HASH_KEY_STRING,
++ pstrdup_escape_nul_bytes(buf, ret, pool));
+ ret = X509_NAME_get_text_by_NID(org,
+ NID_organizationName,
+ buf, 1024);
+ if (ret != -1)
+- apr_hash_set(tgt, "O", APR_HASH_KEY_STRING, apr_pstrdup(pool, buf));
++ apr_hash_set(tgt, "O", APR_HASH_KEY_STRING,
++ pstrdup_escape_nul_bytes(buf, ret, pool));
+ ret = X509_NAME_get_text_by_NID(org,
+ NID_localityName,
+ buf, 1024);
+ if (ret != -1)
+- apr_hash_set(tgt, "L", APR_HASH_KEY_STRING, apr_pstrdup(pool, buf));
++ apr_hash_set(tgt, "L", APR_HASH_KEY_STRING,
++ pstrdup_escape_nul_bytes(buf, ret, pool));
+ ret = X509_NAME_get_text_by_NID(org,
+ NID_stateOrProvinceName,
+ buf, 1024);
+ if (ret != -1)
+- apr_hash_set(tgt, "ST", APR_HASH_KEY_STRING, apr_pstrdup(pool, buf));
++ apr_hash_set(tgt, "ST", APR_HASH_KEY_STRING,
++ pstrdup_escape_nul_bytes(buf, ret, pool));
+ ret = X509_NAME_get_text_by_NID(org,
+ NID_countryName,
+ buf, 1024);
+ if (ret != -1)
+- apr_hash_set(tgt, "C", APR_HASH_KEY_STRING, apr_pstrdup(pool, buf));
++ apr_hash_set(tgt, "C", APR_HASH_KEY_STRING,
++ pstrdup_escape_nul_bytes(buf, ret, pool));
+
+ return tgt;
+ }
+@@ -1550,7 +1681,7 @@
+ unsigned int md_size, i;
+ unsigned char md[EVP_MAX_MD_SIZE];
+ BIO *bio;
+- STACK_OF(GENERAL_NAME) *names;
++ apr_array_header_t *san_arr;
+
+ /* sha1 fingerprint */
+ if (X509_digest(cert->ssl_cert, EVP_sha1(), md, &md_size)) {
+@@ -1595,33 +1726,9 @@
+ BIO_free(bio);
+
+ /* Get subjectAltNames */
+- names = X509_get_ext_d2i(cert->ssl_cert, NID_subject_alt_name, NULL, NULL);
+- if (names) {
+- int names_count = sk_GENERAL_NAME_num(names);
+-
+- apr_array_header_t *san_arr = apr_array_make(pool, names_count,
+- sizeof(char*));
++ if (!get_subject_alt_names(&san_arr, cert->ssl_cert, EscapeNulAndCopy, pool))
+ apr_hash_set(tgt, "subjectAltName", APR_HASH_KEY_STRING, san_arr);
+- for (i = 0; i < names_count; i++) {
+- char *p = NULL;
+- GENERAL_NAME *nm = sk_GENERAL_NAME_value(names, i);
+
+- switch (nm->type) {
+- case GEN_DNS:
+- p = apr_pstrmemdup(pool, (const char *)nm->d.ia5->data,
+- nm->d.ia5->length);
+- break;
+- default:
+- /* Don't know what to do - skip. */
+- break;
+- }
+- if (p) {
+- APR_ARRAY_PUSH(san_arr, char*) = p;
+- }
+- }
+- sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free);
+- }
+-
+ return tgt;
+ }
+
+------------------------------------------------------------------------
+r1699931 | breser | 2014-08-05 19:24:00 -0700 (Tue, 05 Aug 2014) | 6 lines
+
+On the 1.3.x branch:
+
+Merge changes from trunk:
+- r2398: Initialize san_arr when we're expected to fill it.
+
+
+------------------------------------------------------------------------
+Index: misc/build/serf-1.2.1/buckets/ssl_buckets.c
+===================================================================
+--- misc/serf-1.2.1/buckets/ssl_buckets.c (revision 1699930)
++++ misc/build/serf-1.2.1/buckets/ssl_buckets.c (revision 1699931)
+@@ -443,6 +443,10 @@
+
+ /* assert: copy_action == ErrorOnNul || (san_arr && pool) */
+
++ if (san_arr) {
++ *san_arr = NULL;
++ }
++
+ /* Get subjectAltNames */
+ names = X509_get_ext_d2i(ssl_cert, NID_subject_alt_name, NULL, NULL);
+ if (names) {
diff --git a/ext_libraries/serf/makefile.mk b/ext_libraries/serf/makefile.mk
index c75a9cf..d3f7c55 100644
--- a/ext_libraries/serf/makefile.mk
+++ b/ext_libraries/serf/makefile.mk
@@ -46,7 +46,7 @@ TARFILE_NAME=$(PRJNAME)-$(LIBSERFVERSION)
# thankfully, does not care.
TARFILE_MD5=f65fbbd72926c8e7cf0dbd4ada03b0d226f461fd
-PATCH_FILES=
+PATCH_FILES=NULbytes.patch
.IF "$(OS)"=="WNT"