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: #42002
Refs: #42001
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

Author: tniessen

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<X509_STORE_CTX, X509_STORE_CTX_free> 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;

test/parallel/test-x509-escaping.js

@@ -66,21 +66,21 @@ const { hasOpenSSL3 } = common;
     'email:[email protected]',
     // ... but should be escaped if they contain commas.
     'email:"[email protected]\\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',