LibCrypto: Make PKSystem methods return a ByteBuffer directly

It used to be that the caller would supply a buffer to write the output
to. This created an anti-pattern in multiple places where the caller
would allocate a `ByteBuffer` and then use `.bytes()` to provide it to
the `PKSystem` method. Then the callee would resize the output buffer
and reassign it, but because the resize was on `Bytes` and not on
`ByteBuffer`, the caller using the latter would cause a bug.

Additionally, in pretty much all cases the buffer was pre-allocated
shortly before.

Libraries/LibCrypto/PK/PK.h

@@ -232,11 +232,11 @@ public:
 
     PKSystem() = default;
 
-    virtual ErrorOr<void> encrypt(ReadonlyBytes in, Bytes& out) = 0;
-    virtual ErrorOr<void> decrypt(ReadonlyBytes in, Bytes& out) = 0;
+    virtual ErrorOr<ByteBuffer> encrypt(ReadonlyBytes in) = 0;
+    virtual ErrorOr<ByteBuffer> decrypt(ReadonlyBytes in) = 0;
 
-    virtual ErrorOr<void> verify(ReadonlyBytes in, Bytes& out) = 0;
-    virtual ErrorOr<void> sign(ReadonlyBytes in, Bytes& out) = 0;
+    virtual ErrorOr<ByteBuffer> verify(ReadonlyBytes in) = 0;
+    virtual ErrorOr<ByteBuffer> sign(ReadonlyBytes in) = 0;
 
     virtual ByteString class_name() const = 0;
 

Libraries/LibCrypto/PK/RSA.cpp

@@ -167,7 +167,7 @@ ErrorOr<RSA::KeyPairType> RSA::generate_key_pair(size_t bits, IntegerType e)
     return keys;
 }
 
-ErrorOr<void> RSA::encrypt(ReadonlyBytes in, Bytes& out)
+ErrorOr<ByteBuffer> RSA::encrypt(ReadonlyBytes in, Bytes& out)
 {
     dbgln_if(CRYPTO_DEBUG, "in size: {}", in.size());
     auto in_integer = UnsignedBigInteger::import_data(in.data(), in.size());
@@ -175,13 +175,15 @@ ErrorOr<void> RSA::encrypt(ReadonlyBytes in, Bytes& out)
         return Error::from_string_literal("Data too large for key");
 
     auto exp = NumberTheory::ModularPower(in_integer, m_public_key.public_exponent(), m_public_key.modulus());
+
+    auto out = TRY(ByteBuffer::create_uninitialized(exp.byte_length()));
     auto size = exp.export_data(out);
     auto outsize = out.size();
     VERIFY(size == outsize);
-    return {};
+    return out;
 }
 
-ErrorOr<void> RSA::decrypt(ReadonlyBytes in, Bytes& out)
+ErrorOr<ByteBuffer> RSA::decrypt(ReadonlyBytes in)
 {
     auto in_integer = UnsignedBigInteger::import_data(in.data(), in.size());
 
@@ -198,32 +200,34 @@ ErrorOr<void> RSA::decrypt(ReadonlyBytes in, Bytes& out)
         m = m2.plus(h.multiplied_by(m_private_key.prime2()));
     }
 
+    auto out = TRY(ByteBuffer::create_uninitialized(m.byte_length()));
     auto size = m.export_data(out);
     auto align = m_private_key.length();
     auto aligned_size = (size + align - 1) / align * align;
 
     for (auto i = size; i < aligned_size; ++i)
         out[out.size() - i - 1] = 0; // zero the non-aligned values
-    out = out.slice(out.size() - aligned_size, aligned_size);
-    return {};
+    return out.slice(out.size() - aligned_size, aligned_size);
 }
 
-ErrorOr<void> RSA::sign(ReadonlyBytes in, Bytes& out)
+ErrorOr<ByteBuffer> RSA::sign(ReadonlyBytes in)
 {
     auto in_integer = UnsignedBigInteger::import_data(in.data(), in.size());
     auto exp = NumberTheory::ModularPower(in_integer, m_private_key.private_exponent(), m_private_key.modulus());
+
+    auto out = TRY(ByteBuffer::create_uninitialized(exp.byte_length()));
     auto size = exp.export_data(out);
-    out = out.slice(out.size() - size, size);
-    return {};
+    return out.slice(out.size() - size, size);
 }
 
-ErrorOr<void> RSA::verify(ReadonlyBytes in, Bytes& out)
+ErrorOr<ByteBuffer> RSA::verify(ReadonlyBytes in)
 {
     auto in_integer = UnsignedBigInteger::import_data(in.data(), in.size());
     auto exp = NumberTheory::ModularPower(in_integer, m_public_key.public_exponent(), m_public_key.modulus());
+
+    auto out = TRY(ByteBuffer::create_uninitialized(exp.byte_length()));
     auto size = exp.export_data(out);
-    out = out.slice(out.size() - size, size);
-    return {};
+    return out.slice(out.size() - size, size);
 }
 
 void RSA::import_private_key(ReadonlyBytes bytes, bool pem)
@@ -288,15 +292,14 @@ void RSA::import_public_key(ReadonlyBytes bytes, bool pem)
     m_public_key = maybe_key.release_value().public_key;
 }
 
-ErrorOr<void> RSA_PKCS1_EME::encrypt(ReadonlyBytes in, Bytes& out)
+ErrorOr<ByteBuffer> RSA_PKCS1_EME::encrypt(ReadonlyBytes in)
 {
     auto mod_len = (m_public_key.modulus().trimmed_length() * sizeof(u32) * 8 + 7) / 8;
     dbgln_if(CRYPTO_DEBUG, "key size: {}", mod_len);
     if (in.size() > mod_len - 11)
         return Error::from_string_literal("Message too long");
 
-    if (out.size() < mod_len)
-        return Error::from_string_literal("Output buffer too small");
+    auto out = TRY(ByteBuffer::create_uninitialized(mod_len));
 
     auto ps_length = mod_len - in.size() - 3;
     Vector<u8, 8096> ps;
@@ -316,21 +319,20 @@ ErrorOr<void> RSA_PKCS1_EME::encrypt(ReadonlyBytes in, Bytes& out)
     out.overwrite(2, ps.data(), ps_length);
     out.overwrite(2 + ps_length, paddings, 1);
     out.overwrite(3 + ps_length, in.data(), in.size());
-    out = out.trim(3 + ps_length + in.size()); // should be a single block
+    out.trim(3 + ps_length + in.size(), true); // should be a single block
 
     dbgln_if(CRYPTO_DEBUG, "padded output size: {} buffer size: {}", 3 + ps_length + in.size(), out.size());
 
-    TRY(RSA::encrypt(out, out));
-    return {};
+    return TRY(RSA::encrypt(out));
 }
 
-ErrorOr<void> RSA_PKCS1_EME::decrypt(ReadonlyBytes in, Bytes& out)
+ErrorOr<ByteBuffer> RSA_PKCS1_EME::decrypt(ReadonlyBytes in)
 {
     auto mod_len = (m_public_key.modulus().trimmed_length() * sizeof(u32) * 8 + 7) / 8;
     if (in.size() != mod_len)
         return Error::from_string_literal("Invalid input size");
 
-    TRY(RSA::decrypt(in, out));
+    auto out = TRY(RSA::decrypt(in));
 
     if (out.size() < RSA::output_size())
         return Error::from_string_literal("Not enough data after decryption");
@@ -350,16 +352,15 @@ ErrorOr<void> RSA_PKCS1_EME::decrypt(ReadonlyBytes in, Bytes& out)
     if (offset - 3 < 8)
         return Error::from_string_literal("PS too small");
 
-    out = out.slice(offset, out.size() - offset);
-    return {};
+    return out.slice(offset, out.size() - offset);;
 }
 
-ErrorOr<void> RSA_PKCS1_EME::sign(ReadonlyBytes, Bytes&)
+ErrorOr<ByteBuffer> RSA_PKCS1_EME::sign(ReadonlyBytes)
 {
     return Error::from_string_literal("FIXME: RSA_PKCS_EME::sign");
 }
 
-ErrorOr<void> RSA_PKCS1_EME::verify(ReadonlyBytes, Bytes&)
+ErrorOr<ByteBuffer> RSA_PKCS1_EME::verify(ReadonlyBytes)
 {
     return Error::from_string_literal("FIXME: RSA_PKCS_EME::verify");
 }

Libraries/LibCrypto/PK/RSA.h

@@ -195,11 +195,11 @@ public:
         m_public_key.set(m_private_key.modulus(), m_private_key.public_exponent());
     }
 
-    virtual ErrorOr<void> encrypt(ReadonlyBytes in, Bytes& out) override;
-    virtual ErrorOr<void> decrypt(ReadonlyBytes in, Bytes& out) override;
+    virtual ErrorOr<ByteBuffer> encrypt(ReadonlyBytes in) override;
+    virtual ErrorOr<ByteBuffer> decrypt(ReadonlyBytes in) override;
 
-    virtual ErrorOr<void> verify(ReadonlyBytes in, Bytes& out) override;
-    virtual ErrorOr<void> sign(ReadonlyBytes in, Bytes& out) override;
+    virtual ErrorOr<ByteBuffer> verify(ReadonlyBytes in) override;
+    virtual ErrorOr<ByteBuffer> sign(ReadonlyBytes in) override;
 
     virtual ByteString class_name() const override
     {
@@ -232,11 +232,11 @@ public:
 
     ~RSA_PKCS1_EME() = default;
 
-    virtual ErrorOr<void> encrypt(ReadonlyBytes in, Bytes& out) override;
-    virtual ErrorOr<void> decrypt(ReadonlyBytes in, Bytes& out) override;
+    virtual ErrorOr<ByteBuffer> encrypt(ReadonlyBytes in) override;
+    virtual ErrorOr<ByteBuffer> decrypt(ReadonlyBytes in) override;
 
-    virtual ErrorOr<void> verify(ReadonlyBytes in, Bytes& out) override;
-    virtual ErrorOr<void> sign(ReadonlyBytes in, Bytes& out) override;
+    virtual ErrorOr<ByteBuffer> verify(ReadonlyBytes in) override;
+    virtual ErrorOr<ByteBuffer> sign(ReadonlyBytes in) override;
 
     virtual ByteString class_name() const override
     {

Libraries/LibTLS/HandshakeClient.cpp

@@ -191,11 +191,7 @@ void TLSv12::build_rsa_pre_master_secret(PacketBuilder& builder)
     }
 
     Crypto::PK::RSA_PKCS1_EME rsa(certificate.public_key.rsa);
-
-    Vector<u8, 32> out;
-    out.resize(rsa.output_size());
-    auto outbuf = out.span();
-    MUST(rsa.encrypt(m_context.premaster_key, outbuf));
+    auto outbuf = MUST(rsa.encrypt(m_context.premaster_key));
 
     if constexpr (TLS_DEBUG) {
         dbgln("Encrypted: ");

Libraries/LibTLS/HandshakeServer.cpp

@@ -381,14 +381,7 @@ ssize_t TLSv12::verify_rsa_server_key_exchange(ReadonlyBytes server_key_info_buf
     Crypto::PK::RSAPrivateKey dummy_private_key;
     auto rsa = Crypto::PK::RSA(certificate_public_key.rsa, dummy_private_key);
 
-    auto signature_verify_buffer_result = ByteBuffer::create_uninitialized(signature_length);
-    if (signature_verify_buffer_result.is_error()) {
-        dbgln("verify_rsa_server_key_exchange failed: Not enough memory");
-        return (i8)Error::OutOfMemory;
-    }
-    auto signature_verify_buffer = signature_verify_buffer_result.release_value();
-    auto signature_verify_bytes = signature_verify_buffer.bytes();
-    MUST(rsa.verify(signature, signature_verify_bytes));
+    auto signature_verify = MUST(rsa.verify(signature));
 
     auto message_result = ByteBuffer::create_uninitialized(64 + server_key_info_buffer.size());
     if (message_result.is_error()) {
@@ -420,7 +413,7 @@ ssize_t TLSv12::verify_rsa_server_key_exchange(ReadonlyBytes server_key_info_buf
     }
 
     auto pkcs1 = Crypto::PK::EMSA_PKCS1_V1_5<Crypto::Hash::Manager>(hash_kind);
-    auto verification = pkcs1.verify(message, signature_verify_bytes, signature_length * 8);
+    auto verification = pkcs1.verify(message, signature_verify, signature_length * 8);
 
     if (verification == Crypto::VerificationConsistency::Inconsistent) {
         dbgln("verify_rsa_server_key_exchange failed: Verification of signature inconsistent");

Libraries/LibTLS/TLSv12.cpp

@@ -345,18 +345,11 @@ bool Context::verify_certificate_pair(Certificate const& subject, Certificate co
         Crypto::PK::RSAPrivateKey dummy_private_key;
         Crypto::PK::RSAPublicKey public_key_copy { issuer.public_key.rsa };
         auto rsa = Crypto::PK::RSA(public_key_copy, dummy_private_key);
-        auto verification_buffer_result = ByteBuffer::create_uninitialized(subject.signature_value.size());
-        if (verification_buffer_result.is_error()) {
-            dbgln("verify_certificate_pair: Unable to allocate buffer for verification");
-            return false;
-        }
-        auto verification_buffer = verification_buffer_result.release_value();
-        auto verification_buffer_bytes = verification_buffer.bytes();
-        MUST(rsa.verify(subject.signature_value, verification_buffer_bytes));
+        auto verification_bytes = MUST(rsa.verify(subject.signature_value));
 
         ReadonlyBytes message = subject.tbs_asn1.bytes();
         auto pkcs1 = Crypto::PK::EMSA_PKCS1_V1_5<Crypto::Hash::Manager>(kind);
-        auto verification = pkcs1.verify(message, verification_buffer_bytes, subject.signature_value.size() * 8);
+        auto verification = pkcs1.verify(message, verification_bytes, subject.signature_value.size() * 8);
         return verification == Crypto::VerificationConsistency::Consistent;
     }
 

Libraries/LibWeb/Crypto/CryptoAlgorithms.cpp

@@ -687,16 +687,13 @@ WebIDL::ExceptionOr<GC::Ref<JS::ArrayBuffer>> RSAOAEP::encrypt(AlgorithmParams c
     auto padding = maybe_padding.release_value();
 
     // 5. Let ciphertext be the value C that results from performing the operation.
-    auto ciphertext = TRY_OR_THROW_OOM(vm, ByteBuffer::create_uninitialized(public_key.length()));
-    auto ciphertext_bytes = ciphertext.bytes();
-
     auto rsa = ::Crypto::PK::RSA { public_key };
-    auto maybe_encrypt_error = rsa.encrypt(padding, ciphertext_bytes);
-    if (maybe_encrypt_error.is_error())
+    auto maybe_ciphertext = rsa.encrypt(padding);
+    if (maybe_ciphertext.is_error())
         return WebIDL::OperationError::create(realm, "Failed to encrypt"_string);
 
     // 6. Return the result of creating an ArrayBuffer containing ciphertext.
-    return JS::ArrayBuffer::create(realm, move(ciphertext));
+    return JS::ArrayBuffer::create(realm, maybe_ciphertext.release_value());
 }
 
 // https://w3c.github.io/webcrypto/#rsa-oaep-operations
@@ -723,22 +720,20 @@ WebIDL::ExceptionOr<GC::Ref<JS::ArrayBuffer>> RSAOAEP::decrypt(AlgorithmParams c
     auto rsa = ::Crypto::PK::RSA { private_key };
     u32 private_key_length = private_key.length();
 
-    auto padding = TRY_OR_THROW_OOM(vm, ByteBuffer::create_uninitialized(private_key_length));
-    auto padding_bytes = padding.bytes();
-    auto maybe_encrypt_error = rsa.decrypt(ciphertext, padding_bytes);
-    if (maybe_encrypt_error.is_error())
+    auto maybe_padding = rsa.decrypt(ciphertext);
+    if (maybe_padding.is_error())
         return WebIDL::OperationError::create(realm, "Failed to encrypt"_string);
 
     auto error_message = MUST(String::formatted("Invalid hash function '{}'", hash));
     ErrorOr<ByteBuffer> maybe_plaintext = Error::from_string_view(error_message.bytes_as_string_view());
     if (hash == "SHA-1") {
-        maybe_plaintext = ::Crypto::Padding::OAEP::eme_decode<::Crypto::Hash::SHA1, ::Crypto::Hash::MGF>(padding, label, private_key_length);
+        maybe_plaintext = ::Crypto::Padding::OAEP::eme_decode<::Crypto::Hash::SHA1, ::Crypto::Hash::MGF>(maybe_padding.release_value(), label, private_key_length);
     } else if (hash == "SHA-256") {
-        maybe_plaintext = ::Crypto::Padding::OAEP::eme_decode<::Crypto::Hash::SHA256, ::Crypto::Hash::MGF>(padding, label, private_key_length);
+        maybe_plaintext = ::Crypto::Padding::OAEP::eme_decode<::Crypto::Hash::SHA256, ::Crypto::Hash::MGF>(maybe_padding.release_value(), label, private_key_length);
     } else if (hash == "SHA-384") {
-        maybe_plaintext = ::Crypto::Padding::OAEP::eme_decode<::Crypto::Hash::SHA384, ::Crypto::Hash::MGF>(padding, label, private_key_length);
+        maybe_plaintext = ::Crypto::Padding::OAEP::eme_decode<::Crypto::Hash::SHA384, ::Crypto::Hash::MGF>(maybe_padding.release_value(), label, private_key_length);
     } else if (hash == "SHA-512") {
-        maybe_plaintext = ::Crypto::Padding::OAEP::eme_decode<::Crypto::Hash::SHA512, ::Crypto::Hash::MGF>(padding, label, private_key_length);
+        maybe_plaintext = ::Crypto::Padding::OAEP::eme_decode<::Crypto::Hash::SHA512, ::Crypto::Hash::MGF>(maybe_padding.release_value(), label, private_key_length);
     }
 
     // 4. If performing the operation results in an error, then throw an OperationError.

Tests/LibCrypto/TestOAEP.cpp

@@ -133,11 +133,6 @@ TEST_CASE(test_oaep)
     auto public_key = Crypto::PK::RSAPublicKey(n, e);
     auto rsa = Crypto::PK::RSA(public_key, private_key);
 
-    auto maybe_output_buffer = ByteBuffer::create_uninitialized(128);
-    auto output_buffer = maybe_output_buffer.release_value();
-    auto output_span = output_buffer.bytes();
-
-    TRY_OR_FAIL(rsa.encrypt(result, output_span));
-
-    EXPECT_EQ(expected_rsa_value, output_span);
+    auto enc = TRY_OR_FAIL(rsa.encrypt(result));
+    EXPECT_EQ(expected_rsa_value, enc);
 }

Tests/LibCrypto/TestRSA.cpp

@@ -25,10 +25,7 @@ TEST_CASE(test_RSA_raw_encrypt)
         "8126832723025844890518845777858816391166654950553329127845898924164623511718747856014227624997335860970996746552094406240834082304784428582653994490504519"_bigint,
         "65537"_bigint,
     });
-    ByteBuffer buffer = {};
-    buffer.resize(rsa.output_size());
-    auto buf = buffer.bytes();
-    TRY_OR_FAIL(rsa.encrypt(data, buf));
+    auto buf = TRY_OR_FAIL(rsa.encrypt(data));
     EXPECT(memcmp(result, buf.data(), buf.size()) == 0);
 }
 
@@ -39,13 +36,10 @@ TEST_CASE(test_RSA_PKCS_1_encrypt)
 
     auto keypair = TRY_OR_FAIL(Crypto::PK::RSA::generate_key_pair(1024));
     Crypto::PK::RSA_PKCS1_EME rsa(keypair);
-    ByteBuffer buffer = {};
-    buffer.resize(rsa.output_size());
-    auto buf = buffer.bytes();
-    TRY_OR_FAIL(rsa.encrypt(data, buf));
-    TRY_OR_FAIL(rsa.decrypt(buf, buf));
+    auto enc = TRY_OR_FAIL(rsa.encrypt(data));
+    auto dec = TRY_OR_FAIL(rsa.decrypt(enc));
 
-    EXPECT(memcmp(buf.data(), "hellohellohellohellohellohellohellohellohello123-", 49) == 0);
+    EXPECT(memcmp(dec.data(), "hellohellohellohellohellohellohellohellohello123-", 49) == 0);
 }
 
 // RSA | ASN1 PKCS1 DER / PEM encoded Key import
@@ -138,19 +132,14 @@ c8yGzl89pYST
 
     EXPECT_EQ(keypem, StringView(priv_pem));
 
-    ByteBuffer enc_buffer = {};
-    enc_buffer.resize(rsa_from_pair.output_size());
+    ByteBuffer msg_buffer = {};
+    msg_buffer.resize(rsa_from_pair.output_size());
 
-    ByteBuffer dec_buffer = {};
-    dec_buffer.resize(rsa_from_pair.output_size());
+    auto msg = msg_buffer.bytes();
+    msg.overwrite(0, "WellHelloFriends", 16);
 
-    auto enc = enc_buffer.bytes();
-    auto dec = dec_buffer.bytes();
-
-    dec.overwrite(0, "WellHelloFriends", 16);
-
-    TRY_OR_FAIL(rsa_from_pair.encrypt(dec, enc));
-    TRY_OR_FAIL(rsa_from_pem.decrypt(enc, dec));
+    auto enc = TRY_OR_FAIL(rsa_from_pair.encrypt(msg));
+    auto dec = TRY_OR_FAIL(rsa_from_pem.decrypt(enc));
 
     EXPECT_EQ(memcmp(dec.data(), "WellHelloFriends", 16), 0);
 }
@@ -160,19 +149,14 @@ TEST_CASE(test_RSA_encrypt_decrypt)
     auto keypair = TRY_OR_FAIL(Crypto::PK::RSA::generate_key_pair(1024));
     Crypto::PK::RSA rsa(keypair);
 
-    ByteBuffer enc_buffer = {};
-    enc_buffer.resize(rsa.output_size());
+    ByteBuffer msg_buffer = {};
+    msg_buffer.resize(rsa.output_size());
 
-    ByteBuffer dec_buffer = {};
-    dec_buffer.resize(rsa.output_size());
+    auto msg = msg_buffer.bytes();
+    msg.overwrite(0, "WellHelloFriendsWellHelloFriendsWellHelloFriendsWellHelloFriends", 64);
 
-    auto enc = enc_buffer.bytes();
-    auto dec = dec_buffer.bytes();
+    auto enc = TRY_OR_FAIL(rsa.encrypt(msg));
+    auto dec = TRY_OR_FAIL(rsa.decrypt(enc));
 
-    enc.overwrite(0, "WellHelloFriendsWellHelloFriendsWellHelloFriendsWellHelloFriends", 64);
-
-    TRY_OR_FAIL(rsa.encrypt(enc, dec));
-    TRY_OR_FAIL(rsa.decrypt(dec, enc));
-
-    EXPECT(memcmp(enc.data(), "WellHelloFriendsWellHelloFriendsWellHelloFriendsWellHelloFriends", 64) == 0);
+    EXPECT(memcmp(dec.data(), "WellHelloFriendsWellHelloFriendsWellHelloFriendsWellHelloFriends", 64) == 0);
 }