From 563b2ed00053d917fbeea0beddd57a1024f433ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Tue, 22 Feb 2022 16:27:01 +0100 Subject: [PATCH] crypto: use RFC2253 format in PrintGeneralName For backward compatibility, node uses X509_NAME_oneline to format X509_NAME entries in PrintGeneralName. However, the format produced by this function is non-standard and its use is discouraged. It also does not handle Unicode names correctly. This change switches to X509_NAME_print_ex with flags that produce an RFC2253-compatible format. Non-ASCII strings are converted to UTF-8 and preserved in the output. Control characters are not escaped by OpenSSL when producing the RFC2253 format because they will be escaped by node in a JSON-compatible manner afterwards. PR-URL: https://github.com/nodejs/node/pull/42002 Refs: https://github.com/nodejs/node/pull/42001 Reviewed-By: Rich Trott Reviewed-By: Matteo Collina --- src/crypto/crypto_common.cc | 38 ++++++++++++++++++++--------- test/parallel/test-x509-escaping.js | 30 +++++++++++------------ 2 files changed, 42 insertions(+), 26 deletions(-) diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc index 221d70d08cc..dce0774e8fa 100644 --- a/src/crypto/crypto_common.cc +++ b/src/crypto/crypto_common.cc @@ -48,6 +48,11 @@ static constexpr int kX509NameFlagsMultiline = XN_FLAG_SEP_MULTILINE | XN_FLAG_FN_SN; +static constexpr int kX509NameFlagsRFC2253WithinUtf8JSON = + XN_FLAG_RFC2253 & + ~ASN1_STRFLGS_ESC_MSB & + ~ASN1_STRFLGS_ESC_CTRL; + int SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert, X509** issuer) { X509_STORE* store = SSL_CTX_get_cert_store(ctx); DeleteFnPtr store_ctx( @@ -740,20 +745,31 @@ static bool PrintGeneralName(const BIOPointer& out, const GENERAL_NAME* gen) { // escaping. PrintLatin1AltName(out, name); } else if (gen->type == GEN_DIRNAME) { - // For backward compatibility, use X509_NAME_oneline to print the - // X509_NAME object. The format is non standard and should be avoided - // elsewhere, but conveniently, the function produces ASCII and the output - // is unlikely to contains commas or other characters that would require - // escaping. With that in mind, note that it SHOULD NOT produce ASCII - // output since an RFC5280 AttributeValue may be a UTF8String. - // TODO(tniessen): switch to RFC2253 rules in a major release + // Earlier versions of Node.js used X509_NAME_oneline to print the X509_NAME + // object. The format was non standard and should be avoided. The use of + // X509_NAME_oneline is discouraged by OpenSSL but was required for backward + // compatibility. Conveniently, X509_NAME_oneline produced ASCII and the + // output was unlikely to contains commas or other characters that would + // require escaping. However, it SHOULD NOT produce ASCII output since an + // RFC5280 AttributeValue may be a UTF8String. + // Newer versions of Node.js have since switched to X509_NAME_print_ex to + // produce a better format at the cost of backward compatibility. The new + // format may contain Unicode characters and it is likely to contain commas, + // which require escaping. Fortunately, the recently safeguarded function + // PrintAltName handles all of that safely. BIO_printf(out.get(), "DirName:"); - char oline[256]; - if (X509_NAME_oneline(gen->d.dirn, oline, sizeof(oline)) != nullptr) { - PrintAltName(out, oline, strlen(oline), false, nullptr); - } else { + BIOPointer tmp(BIO_new(BIO_s_mem())); + CHECK(tmp); + if (X509_NAME_print_ex(tmp.get(), + gen->d.dirn, + 0, + kX509NameFlagsRFC2253WithinUtf8JSON) < 0) { return false; } + char* oline = nullptr; + size_t n_bytes = BIO_get_mem_data(tmp.get(), &oline); + CHECK_IMPLIES(n_bytes != 0, oline != nullptr); + PrintAltName(out, oline, n_bytes, true, nullptr); } else if (gen->type == GEN_IPADD) { BIO_printf(out.get(), "IP Address:"); const ASN1_OCTET_STRING* ip = gen->d.ip; diff --git a/test/parallel/test-x509-escaping.js b/test/parallel/test-x509-escaping.js index 3f534cfa168..c3b88646899 100644 --- a/test/parallel/test-x509-escaping.js +++ b/test/parallel/test-x509-escaping.js @@ -66,21 +66,21 @@ const { hasOpenSSL3 } = common; 'email:foo@example.com', // ... but should be escaped if they contain commas. 'email:"foo@example.com\\u002c DNS:good.example.com"', - 'DirName:/C=DE/L=Hannover', - // TODO(tniessen): support UTF8 in DirName - 'DirName:"/C=DE/L=M\\\\xC3\\\\xBCnchen"', - 'DirName:"/C=DE/L=Berlin\\u002c DNS:good.example.com"', - 'DirName:"/C=DE/L=Berlin\\u002c DNS:good.example.com\\\\x00' + - 'evil.example.com"', - 'DirName:"/C=DE/L=Berlin\\u002c DNS:good.example.com\\\\\\\\x00' + - 'evil.example.com"', - // These next two tests might be surprising. OpenSSL applies its own rules - // first, which introduce backslashes, which activate node's escaping. - // Unfortunately, there are also differences between OpenSSL 1.1.1 and 3.0. - 'DirName:"/C=DE/L=Berlin\\\\x0D\\\\x0A"', - hasOpenSSL3 ? - 'DirName:"/C=DE/L=Berlin\\\\/CN=good.example.com"' : - 'DirName:/C=DE/L=Berlin/CN=good.example.com', + // New versions of Node.js use RFC2253 to print DirName entries, which + // almost always results in commas, which should be escaped properly. + 'DirName:"L=Hannover\\u002cC=DE"', + // Node.js unsets ASN1_STRFLGS_ESC_MSB to prevent unnecessarily escaping + // Unicode characters, so Unicode characters should be preserved. + 'DirName:"L=München\\u002cC=DE"', + 'DirName:"L=Berlin\\\\\\u002c DNS:good.example.com\\u002cC=DE"', + // Node.js also unsets ASN1_STRFLGS_ESC_CTRL and relies on JSON-compatible + // escaping rules to safely escape control characters. + 'DirName:"L=Berlin\\\\\\u002c DNS:good.example.com\\u0000' + + 'evil.example.com\\u002cC=DE"', + 'DirName:"L=Berlin\\\\\\u002c DNS:good.example.com\\\\\\\\\\u0000' + + 'evil.example.com\\u002cC=DE"', + 'DirName:"L=Berlin\\u000d\\u000a\\u002cC=DE"', + 'DirName:"L=Berlin/CN=good.example.com\\u002cC=DE"', // Even OIDs that are well-known (such as the following, which is // sha256WithRSAEncryption) should be represented numerically only. 'Registered ID:1.2.840.113549.1.1.11',