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: #57816
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

Author: tniessen

deps/ncrypto/engine.cc

@@ -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() {

deps/ncrypto/ncrypto.cc

@@ -1325,7 +1325,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;
@@ -1363,7 +1363,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);
 }
@@ -1419,9 +1419,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) {
@@ -1703,17 +1702,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 Digest& md, size_t length) {
@@ -2560,8 +2559,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());
 
@@ -2626,7 +2624,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;
@@ -2651,7 +2649,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;
@@ -2794,10 +2792,10 @@ bool SSLCtxPointer::setGroups(const char* groups) {
   return SSL_CTX_set1_groups_list(get(), groups) == 1;
 }
 
-bool SSLCtxPointer::setCipherSuites(std::string_view ciphers) {
+bool SSLCtxPointer::setCipherSuites(const char* ciphers) {
 #ifndef OPENSSL_IS_BORINGSSL
   if (!ctx_) return false;
-  return SSL_CTX_set_ciphersuites(ctx_.get(), ciphers.data());
+  return SSL_CTX_set_ciphersuites(ctx_.get(), ciphers);
 #else
   // BoringSSL does not allow API config of TLS 1.3 cipher suites.
   // We treat this as a non-op.
@@ -2807,8 +2805,8 @@ bool SSLCtxPointer::setCipherSuites(std::string_view ciphers) {
 
 // ============================================================================
 
-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) {
@@ -2922,7 +2920,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.
@@ -3839,7 +3837,7 @@ DataPointer Cipher::recover(const EVPKeyPointer& key,
 namespace {
 struct CipherCallbackContext {
   Cipher::CipherNameCallback cb;
-  void operator()(std::string_view name) { cb(name); }
+  void operator()(const char* name) { cb(name); }
 };
 
 #if OPENSSL_VERSION_MAJOR >= 3
@@ -3918,10 +3916,10 @@ int Ec::getCurve() const {
   return EC_GROUP_get_curve_name(getGroup());
 }
 
-int Ec::GetCurveIdFromName(std::string_view name) {
-  int nid = EC_curve_nist2nid(name.data());
+int Ec::GetCurveIdFromName(const char* name) {
+  int nid = EC_curve_nist2nid(name);
   if (nid == NID_undef) {
-    nid = OBJ_sn2nid(name.data());
+    nid = OBJ_sn2nid(name);
   }
   return nid;
 }
@@ -4294,7 +4292,7 @@ const Digest Digest::SHA256 = Digest(EVP_sha256());
 const Digest Digest::SHA384 = Digest(EVP_sha384());
 const Digest Digest::SHA512 = Digest(EVP_sha512());
 
-const Digest Digest::FromName(std::string_view name) {
+const Digest Digest::FromName(const char* name) {
   return ncrypto::getDigestByName(name);
 }
 

deps/ncrypto/ncrypto.h

@@ -272,7 +272,7 @@ class Digest final {
   static const Digest SHA384;
   static const Digest SHA512;
 
-  static const Digest FromName(std::string_view name);
+  static const Digest FromName(const char* name);
 
  private:
   const EVP_MD* md_ = nullptr;
@@ -314,7 +314,7 @@ class Cipher final {
   int getKeyLength() const;
   int getBlockSize() const;
   std::string_view getModeLabel() const;
-  std::string_view getName() const;
+  const char* getName() const;
 
   bool isGcmMode() const;
   bool isWrapMode() const;
@@ -331,11 +331,11 @@ class Cipher final {
                  unsigned char* key,
                  unsigned char* iv) 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);
 
-  using CipherNameCallback = std::function<void(std::string_view name)>;
+  using CipherNameCallback = std::function<void(const char* name)>;
 
   // Iterates the known ciphers if the underlying implementation
   // is able to do so.
@@ -477,9 +477,9 @@ class Ec final {
   inline operator bool() const { return ec_ != nullptr; }
   inline operator OSSL3_CONST EC_KEY*() const { return ec_; }
 
-  static int GetCurveIdFromName(std::string_view name);
+  static int GetCurveIdFromName(const char* name);
 
-  using GetCurveCallback = std::function<bool(std::string_view)>;
+  using GetCurveCallback = std::function<bool(const char*)>;
   static bool GetCurves(GetCurveCallback callback);
 
  private:
@@ -568,7 +568,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>
@@ -941,9 +941,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);
@@ -1042,7 +1041,7 @@ class SSLCtxPointer final {
     SSL_CTX_set_tlsext_status_arg(get(), nullptr);
   }
 
-  bool setCipherSuites(std::string_view ciphers);
+  bool setCipherSuites(const char* ciphers);
 
   static SSLCtxPointer NewServer();
   static SSLCtxPointer NewClient();
@@ -1071,8 +1070,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;
@@ -1088,7 +1087,7 @@ class SSLPointer final {
 
   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);
@@ -1184,13 +1183,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>
@@ -1227,8 +1226,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_;
@@ -1444,15 +1443,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.
@@ -1501,8 +1500,8 @@ Buffer<char> ExportChallenge(const char* input, size_t length);
 // ============================================================================
 // 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/crypto/crypto_cipher.cc

@@ -48,7 +48,7 @@ void GetCipherInfo(const FunctionCallbackInfo<Value>& args) {
   const auto cipher = ([&] {
     if (args[1]->IsString()) {
       Utf8Value name(env->isolate(), args[1]);
-      return Cipher::FromName(name.ToStringView());
+      return Cipher::FromName(*name);
     } else {
       int nid = args[1].As<Int32>()->Value();
       return Cipher::FromNid(nid);
@@ -117,7 +117,7 @@ void GetCipherInfo(const FunctionCallbackInfo<Value>& args) {
 
   if (info->Set(env->context(),
                 env->name_string(),
-                OneByteString(env->isolate(), name.data(), name.length()))
+                OneByteString(env->isolate(), name))
           .IsNothing()) {
     return;
   }
@@ -303,7 +303,7 @@ void CipherBase::New(const FunctionCallbackInfo<Value>& args) {
   new CipherBase(env, args.This(), args[0]->IsTrue() ? kCipher : kDecipher);
 }
 
-void CipherBase::CommonInit(std::string_view cipher_type,
+void CipherBase::CommonInit(const char* cipher_type,
                             const ncrypto::Cipher& cipher,
                             const unsigned char* key,
                             int key_len,
@@ -345,7 +345,7 @@ void CipherBase::CommonInit(std::string_view cipher_type,
   }
 }
 
-void CipherBase::InitIv(std::string_view cipher_type,
+void CipherBase::InitIv(const char* cipher_type,
                         const ByteSource& key_buf,
                         const ArrayBufferOrViewContents<unsigned char>& iv_buf,
                         unsigned int auth_tag_len) {
@@ -425,10 +425,10 @@ void CipherBase::InitIv(const FunctionCallbackInfo<Value>& args) {
     auth_tag_len = kNoAuthTagLength;
   }
 
-  cipher->InitIv(cipher_type.ToStringView(), key_buf, iv_buf, auth_tag_len);
+  cipher->InitIv(*cipher_type, key_buf, iv_buf, auth_tag_len);
 }
 
-bool CipherBase::InitAuthenticated(std::string_view cipher_type,
+bool CipherBase::InitAuthenticated(const char* cipher_type,
                                    int iv_len,
                                    unsigned int auth_tag_len) {
   CHECK(IsAuthenticatedMode());
@@ -933,7 +933,7 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo<Value>& args) {
   Digest digest;
   if (args[offset + 2]->IsString()) {
     Utf8Value oaep_str(env->isolate(), args[offset + 2]);
-    digest = Digest::FromName(oaep_str.ToStringView());
+    digest = Digest::FromName(*oaep_str);
     if (!digest) return THROW_ERR_OSSL_EVP_INVALID_DIGEST(env);
   }