crypto: revert dangerous uses of std::string_view

An `std::string_view v` is a `const char* v.data()` along with an
`std::size_t v.size()` that guarantees that `v.size()` contiguous
elements of type `char` can be accessed relative to the pointer
`v.data()`.

One of the main reasons behind the existence of  `std::string_view` is
the ability to operate on `char` sequences without requiring null
termination, which otherwise often requires expensive copies of strings
to be made. As a consequence, it is generally incorrect to assume that
`v.data()` points to a null-terminated sequence of `char`, and the only
way to obtain a null-terminated string from an `std::string_view` is to
make a copy. It is not even possible to check if the sequence pointed to
by `v.data()` is null-terminated because the null character would be at
position `v.data() + v.size()`, which is outside of the range that `v`
guarantees safe access to. (A default-constructed `std::string_view`
even sets its own data pointer to a `nullptr`, which is fine because it
only needs to guarantee safe access to zero elements, i.e., to no
elements).

In `deps/ncrypto` and `src/crypto`, there are various APIs that consume
`std::string_view v` arguments but then ignore `v.size()` and treat
`v.data()` as a C-style string of type `const char*`. However, that is
not what call sites would expect from functions that explicitly ask for
`std::string_view` arguments, since it makes assumptions beyond the
guarantees provided by `std::string_view` and leads to undefined
behavior unless the given view either contains an embedded null
character or the `char` at address `v.data() + v.size()` is a null
character. This is not a reasonable assumption for `std::string_view` in
general, and it also defeats the purpose of `std::string_view` for the
most part since, when `v.size()` is being ignored, it is essentially
just a `const char*`.

Constructing an `std::string_view` from a `const char*` is also not
"free" but requires computing the length of the C-style string (unless
the length can be computed at compile time, e.g., because the value is
just a string literal). Repeated conversion between `const char*` as
used by OpenSSL and `std::string_view` as used by ncrypto thus incurs
the additional overhead of computing the length of the string whenever
an `std::string_view` is constructed from a `const char*`. (This seems
negligible compared to the safety argument though.)

Similarly, returning a `const char*` pointer to a C-style string as an
`std::string_view` has two downsides: the function must compute the
length of the string in order to construct the view, and the caller
can no longer assume that the return value is null-terminated and thus
cannot pass the returned view to functions that require their arguments
to be null terminated. (And, for the reasons explained above, the caller
also cannot check if the value is null-terminated without potentially
invoking undefined behavior.)

C++20 unfortunately does not have a type similar to Rust's `CStr` or
GSL `czstring`. Therefore, this commit changes many occurrences of
`std::string_view` back to `const char*`, which is conventional for
null-terminated C-style strings and does not require computing the
length of strings.

There are _a lot_ of occurrences of `std::string_view` in ncrypto and
for each one, we need to evaluate if it is safe and a good abstraction.
I tried to do so, but I might have changed too few or too many, so
please feel free to give feedback on individual occurrences.

PR-URL: nodejs/node#57816
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

Author: tniessen

include/ncrypto.h

@@ -336,11 +336,11 @@ class Cipher final {
   int getKeyLength() const;
   int getBlockSize() const;
   std::string_view getModeLabel() const;
-  std::string_view getName() const;
+  const char* getName() const;
 
   bool isSupportedAuthenticatedMode() const;
 
-  static const Cipher FromName(std::string_view name);
+  static const Cipher FromName(const char* name);
   static const Cipher FromNid(int nid);
   static const Cipher FromCtx(const CipherCtxPointer& ctx);
 
@@ -564,6 +564,10 @@ class Ec final {
   inline operator bool() const { return ec_ != nullptr; }
   inline operator OSSL3_CONST EC_KEY*() const { return ec_; }
 
+  static int GetCurveIdFromName(const char* name);
+
+  using GetCurveCallback = std::function<bool(const char*)>;
+  static bool GetCurves(GetCurveCallback callback);
   inline const BignumPointer& getX() const { return x_; }
   inline const BignumPointer& getY() const { return y_; }
   inline const BignumPointer& getD() const { return d_; }
@@ -633,7 +637,7 @@ class BIOPointer final {
   static BIOPointer New(const BIO_METHOD* method);
   static BIOPointer New(const void* data, size_t len);
   static BIOPointer New(const BIGNUM* bn);
-  static BIOPointer NewFile(std::string_view filename, std::string_view mode);
+  static BIOPointer NewFile(const char* filename, const char* mode);
   static BIOPointer NewFp(FILE* fd, int flags);
 
   template <typename T>
@@ -945,9 +949,8 @@ class DHPointer final {
   static BignumPointer GetStandardGenerator();
 
   static BignumPointer FindGroup(
-      const std::string_view name,
-      FindGroupOption option = FindGroupOption::NONE);
-  static DHPointer FromGroup(const std::string_view name,
+      std::string_view name, FindGroupOption option = FindGroupOption::NONE);
+  static DHPointer FromGroup(std::string_view name,
                              FindGroupOption option = FindGroupOption::NONE);
 
   static DHPointer New(BignumPointer&& p, BignumPointer&& g);
@@ -1046,6 +1049,8 @@ class SSLCtxPointer final {
     SSL_CTX_set_tlsext_status_arg(get(), nullptr);
   }
 
+  bool setCipherSuites(const char* ciphers);
+
   static SSLCtxPointer NewServer();
   static SSLCtxPointer NewClient();
   static SSLCtxPointer New(const SSL_METHOD* method = TLS_method());
@@ -1073,8 +1078,8 @@ class SSLPointer final {
   bool setSession(const SSLSessionPointer& session);
   bool setSniContext(const SSLCtxPointer& ctx) const;
 
-  const std::string_view getClientHelloAlpn() const;
-  const std::string_view getClientHelloServerName() const;
+  const char* getClientHelloAlpn() const;
+  const char* getClientHelloServerName() const;
 
   std::optional<const std::string_view> getServerName() const;
   X509View getCertificate() const;
@@ -1087,8 +1092,9 @@ class SSLPointer final {
   std::optional<std::string_view> getCipherVersion() const;
 
   std::optional<uint32_t> verifyPeerCertificate() const;
+  static std::optional<int> getSecurityLevel();
 
-  void getCiphers(std::function<void(const std::string_view)> cb) const;
+  void getCiphers(std::function<void(const char*)> cb) const;
 
   static SSLPointer New(const SSLCtxPointer& ctx);
   static std::optional<const std::string_view> GetServerName(const SSL* ssl);
@@ -1186,13 +1192,13 @@ class X509View final {
     INVALID_NAME,
     OPERATION_FAILED,
   };
-  CheckMatch checkHost(const std::string_view host,
+  CheckMatch checkHost(std::string_view host,
                        int flags,
                        DataPointer* peerName = nullptr) const;
-  CheckMatch checkEmail(const std::string_view email, int flags) const;
-  CheckMatch checkIp(const std::string_view ip, int flags) const;
+  CheckMatch checkEmail(std::string_view email, int flags) const;
+  CheckMatch checkIp(std::string_view ip, int flags) const;
 
-  using UsageCallback = std::function<void(std::string_view)>;
+  using UsageCallback = std::function<void(const char*)>;
   bool enumUsages(UsageCallback callback) const;
 
   template <typename T>
@@ -1229,8 +1235,8 @@ class X509Pointer final {
   X509View view() const;
   operator X509View() const { return view(); }
 
-  static std::string_view ErrorCode(int32_t err);
-  static std::optional<std::string_view> ErrorReason(int32_t err);
+  static const char* ErrorCode(int32_t err);
+  static std::optional<const char*> ErrorReason(int32_t err);
 
  private:
   DeleteFnPtr<X509, X509_free> cert_;
@@ -1505,15 +1511,15 @@ class EnginePointer final {
 
   bool setAsDefault(uint32_t flags, CryptoErrorList* errors = nullptr);
   bool init(bool finish_on_exit = false);
-  EVPKeyPointer loadPrivateKey(const std::string_view key_name);
+  EVPKeyPointer loadPrivateKey(const char* key_name);
 
   // Release ownership of the ENGINE* pointer.
   ENGINE* release();
 
   // Retrieve an OpenSSL Engine instance by name. If the name does not
   // identify a valid named engine, the returned EnginePointer will be
   // empty.
-  static EnginePointer getEngineByName(const std::string_view name,
+  static EnginePointer getEngineByName(const char* name,
                                        CryptoErrorList* errors = nullptr);
 
   // Call once when initializing OpenSSL at startup for the process.
@@ -1570,8 +1576,8 @@ DataPointer ExportChallenge(const Buffer<const char>& buf);
 // ============================================================================
 // KDF
 
-const EVP_MD* getDigestByName(const std::string_view name);
-const EVP_CIPHER* getCipherByName(const std::string_view name);
+const EVP_MD* getDigestByName(const char* name);
+const EVP_CIPHER* getCipherByName(const char* name);
 
 // Verify that the specified HKDF output length is valid for the given digest.
 // The maximum length for HKDF output for a given digest is 255 times the

src/engine.cpp

@@ -44,15 +44,15 @@ ENGINE* EnginePointer::release() {
   return ret;
 }
 
-EnginePointer EnginePointer::getEngineByName(const std::string_view name,
+EnginePointer EnginePointer::getEngineByName(const char* name,
                                              CryptoErrorList* errors) {
   MarkPopErrorOnReturn mark_pop_error_on_return(errors);
-  EnginePointer engine(ENGINE_by_id(name.data()));
+  EnginePointer engine(ENGINE_by_id(name));
   if (!engine) {
     // Engine not found, try loading dynamically.
     engine = EnginePointer(ENGINE_by_id("dynamic"));
     if (engine) {
-      if (!ENGINE_ctrl_cmd_string(engine.get(), "SO_PATH", name.data(), 0) ||
+      if (!ENGINE_ctrl_cmd_string(engine.get(), "SO_PATH", name, 0) ||
           !ENGINE_ctrl_cmd_string(engine.get(), "LOAD", nullptr, 0)) {
         engine.reset();
       }
@@ -73,10 +73,10 @@ bool EnginePointer::init(bool finish_on_exit) {
   return ENGINE_init(engine) == 1;
 }
 
-EVPKeyPointer EnginePointer::loadPrivateKey(const std::string_view key_name) {
+EVPKeyPointer EnginePointer::loadPrivateKey(const char* key_name) {
   if (engine == nullptr) return EVPKeyPointer();
   return EVPKeyPointer(
-      ENGINE_load_private_key(engine, key_name.data(), nullptr, nullptr));
+      ENGINE_load_private_key(engine, key_name, nullptr, nullptr));
 }
 
 void EnginePointer::initEnginesOnce() {

src/ncrypto.cpp

@@ -1419,7 +1419,7 @@ X509Pointer X509Pointer::PeerFrom(const SSLPointer& ssl) {
 // When adding or removing errors below, please also update the list in the API
 // documentation. See the "OpenSSL Error Codes" section of doc/api/errors.md
 // Also *please* update the respective section in doc/api/tls.md as well
-std::string_view X509Pointer::ErrorCode(int32_t err) {  // NOLINT(runtime/int)
+const char* X509Pointer::ErrorCode(int32_t err) {  // NOLINT(runtime/int)
 #define CASE(CODE)                                                             \
   case X509_V_ERR_##CODE:                                                      \
     return #CODE;
@@ -1457,7 +1457,7 @@ std::string_view X509Pointer::ErrorCode(int32_t err) {  // NOLINT(runtime/int)
   return "UNSPECIFIED";
 }
 
-std::optional<std::string_view> X509Pointer::ErrorReason(int32_t err) {
+std::optional<const char*> X509Pointer::ErrorReason(int32_t err) {
   if (err == X509_V_OK) return std::nullopt;
   return X509_verify_cert_error_string(err);
 }
@@ -1513,9 +1513,8 @@ BIOPointer BIOPointer::New(const void* data, size_t len) {
   return BIOPointer(BIO_new_mem_buf(data, len));
 }
 
-BIOPointer BIOPointer::NewFile(std::string_view filename,
-                               std::string_view mode) {
-  return BIOPointer(BIO_new_file(filename.data(), mode.data()));
+BIOPointer BIOPointer::NewFile(const char* filename, const char* mode) {
+  return BIOPointer(BIO_new_file(filename, mode));
 }
 
 BIOPointer BIOPointer::NewFp(FILE* fd, int close_flag) {
@@ -1797,17 +1796,17 @@ DataPointer DHPointer::stateless(const EVPKeyPointer& ourKey,
 // ============================================================================
 // KDF
 
-const EVP_MD* getDigestByName(const std::string_view name) {
+const EVP_MD* getDigestByName(const char* name) {
   // Historically, "dss1" and "DSS1" were DSA aliases for SHA-1
   // exposed through the public API.
-  if (name == "dss1" || name == "DSS1") [[unlikely]] {
+  if (strcmp(name, "dss1") == 0 || strcmp(name, "DSS1") == 0) [[unlikely]] {
     return EVP_sha1();
   }
-  return EVP_get_digestbyname(name.data());
+  return EVP_get_digestbyname(name);
 }
 
-const EVP_CIPHER* getCipherByName(const std::string_view name) {
-  return EVP_get_cipherbyname(name.data());
+const EVP_CIPHER* getCipherByName(const char* name) {
+  return EVP_get_cipherbyname(name);
 }
 
 bool checkHkdfLength(const EVP_MD* md, size_t length) {
@@ -2920,8 +2919,7 @@ SSLPointer SSLPointer::New(const SSLCtxPointer& ctx) {
   return SSLPointer(SSL_new(ctx.get()));
 }
 
-void SSLPointer::getCiphers(
-    std::function<void(const std::string_view)> cb) const {
+void SSLPointer::getCiphers(std::function<void(const char*)> cb) const {
   if (!ssl_) return;
   STACK_OF(SSL_CIPHER)* ciphers = SSL_get_ciphers(get());
 
@@ -2986,7 +2984,7 @@ std::optional<uint32_t> SSLPointer::verifyPeerCertificate() const {
   return std::nullopt;
 }
 
-const std::string_view SSLPointer::getClientHelloAlpn() const {
+const char* SSLPointer::getClientHelloAlpn() const {
   if (ssl_ == nullptr) return {};
 #ifndef OPENSSL_IS_BORINGSSL
   const unsigned char* buf;
@@ -3011,7 +3009,7 @@ const std::string_view SSLPointer::getClientHelloAlpn() const {
 #endif
 }
 
-const std::string_view SSLPointer::getClientHelloServerName() const {
+const char* SSLPointer::getClientHelloServerName() const {
   if (ssl_ == nullptr) return {};
 #ifndef OPENSSL_IS_BORINGSSL
   const unsigned char* buf;
@@ -3154,6 +3152,12 @@ SSLCtxPointer SSLCtxPointer::New(const SSL_METHOD* method) {
 bool SSLCtxPointer::setGroups(const char* groups) {
 #ifndef NCRYPTO_NO_SSL_GROUP_LIST
   return SSL_CTX_set1_groups_list(get(), groups) == 1;
+}
+
+bool SSLCtxPointer::setCipherSuites(const char* ciphers) {
+#ifndef OPENSSL_IS_BORINGSSL
+  if (!ctx_) return false;
+  return SSL_CTX_set_ciphersuites(ctx_.get(), ciphers);
 #else
   // Older versions of openssl/boringssl do not implement the
   // SSL_CTX_set1_groups_list API
@@ -3163,8 +3167,8 @@ bool SSLCtxPointer::setGroups(const char* groups) {
 
 // ============================================================================
 
-const Cipher Cipher::FromName(std::string_view name) {
-  return Cipher(EVP_get_cipherbyname(name.data()));
+const Cipher Cipher::FromName(const char* name) {
+  return Cipher(EVP_get_cipherbyname(name));
 }
 
 const Cipher Cipher::FromNid(int nid) {
@@ -3282,7 +3286,7 @@ std::string_view Cipher::getModeLabel() const {
   return "{unknown}";
 }
 
-std::string_view Cipher::getName() const {
+const char* Cipher::getName() const {
   if (!cipher_) return {};
   // OBJ_nid2sn(EVP_CIPHER_nid(cipher)) is used here instead of
   // EVP_CIPHER_name(cipher) for compatibility with BoringSSL.
@@ -4193,6 +4197,74 @@ DataPointer Cipher::recover(const EVPKeyPointer& key,
       key, params, in);
 }
 
+namespace {
+struct CipherCallbackContext {
+  Cipher::CipherNameCallback cb;
+  void operator()(const char* name) { cb(name); }
+};
+
+#if OPENSSL_VERSION_MAJOR >= 3
+template <class TypeName,
+          TypeName* fetch_type(OSSL_LIB_CTX*, const char*, const char*),
+          void free_type(TypeName*),
+          const TypeName* getbyname(const char*),
+          const char* getname(const TypeName*)>
+void array_push_back(const TypeName* evp_ref,
+                     const char* from,
+                     const char* to,
+                     void* arg) {
+  if (from == nullptr) return;
+
+  const TypeName* real_instance = getbyname(from);
+  if (!real_instance) return;
+
+  const char* real_name = getname(real_instance);
+  if (!real_name) return;
+
+  // EVP_*_fetch() does not support alias names, so we need to pass it the
+  // real/original algorithm name.
+  // We use EVP_*_fetch() as a filter here because it will only return an
+  // instance if the algorithm is supported by the public OpenSSL APIs (some
+  // algorithms are used internally by OpenSSL and are also passed to this
+  // callback).
+  TypeName* fetched = fetch_type(nullptr, real_name, nullptr);
+  if (fetched == nullptr) return;
+
+  free_type(fetched);
+  auto& cb = *(static_cast<CipherCallbackContext*>(arg));
+  cb(from);
+}
+#else
+template <class TypeName>
+void array_push_back(const TypeName* evp_ref,
+                     const char* from,
+                     const char* to,
+                     void* arg) {
+  if (!from) return;
+  auto& cb = *(static_cast<CipherCallbackContext*>(arg));
+  cb(from);
+}
+#endif
+}  // namespace
+
+void Cipher::ForEach(Cipher::CipherNameCallback callback) {
+  ClearErrorOnReturn clearErrorOnReturn;
+  CipherCallbackContext context;
+  context.cb = std::move(callback);
+
+  EVP_CIPHER_do_all_sorted(
+#if OPENSSL_VERSION_MAJOR >= 3
+      array_push_back<EVP_CIPHER,
+                      EVP_CIPHER_fetch,
+                      EVP_CIPHER_free,
+                      EVP_get_cipherbyname,
+                      EVP_CIPHER_get0_name>,
+#else
+      array_push_back<EVP_CIPHER>,
+#endif
+      &context);
+}
+
 // ============================================================================
 
 Ec::Ec() : ec_(nullptr) {}
@@ -4214,6 +4286,24 @@ int Ec::getCurve() const {
   return EC_GROUP_get_curve_name(getGroup());
 }
 
+int Ec::GetCurveIdFromName(const char* name) {
+  int nid = EC_curve_nist2nid(name);
+  if (nid == NID_undef) {
+    nid = OBJ_sn2nid(name);
+  }
+  return nid;
+}
+
+bool Ec::GetCurves(Ec::GetCurveCallback callback) {
+  const size_t count = EC_get_builtin_curves(nullptr, 0);
+  std::vector<EC_builtin_curve> curves(count);
+  if (EC_get_builtin_curves(curves.data(), count) != count) {
+    return false;
+  }
+  for (auto curve : curves) {
+    if (!callback(OBJ_nid2sn(curve.nid))) return false;
+  }
+  return true;
 uint32_t Ec::getDegree() const {
   return EC_GROUP_get_degree(getGroup());
 }