LibCrypto+LibTLS+LibWeb: Store EC key size + refactor serialization

In order for public/private key serialization to work correctly we must store the size of the key because P-521 cannot be stored as full words inside `UnsignedBigInteger` and therefore is exported as the wrong length (68 instead of 66). This makes it also possible to refactor some methods and cleanup constants scattered around. Gets almost all import/export tests, expect the JWK ones that calculate the public key on export. The `SECPxxxr1` implementation currently fails to do calculations for P-521.
devgianlu · Dec 13, 2024 · f796167 · f796167
1 parent 32bbbff
commit f796167
Show file tree

Hide file tree

Showing 9 changed files with 140 additions and 110 deletions.
diff --git a/Libraries/LibCrypto/Certificate/Certificate.cpp b/Libraries/LibCrypto/Certificate/Certificate.cpp
@@ -14,11 +14,6 @@
 #include <LibCrypto/ASN1/PEM.h>
 #include <LibCrypto/PK/EC.h>
 
-namespace {
-// Used by ASN1 macros
-static String s_error_string;
-}
-
 namespace Crypto::Certificate {
 
 static ErrorOr<Crypto::UnsignedBigInteger> parse_certificate_version(Crypto::ASN1::Decoder& decoder, Vector<StringView> current_scope)

diff --git a/Libraries/LibCrypto/Curves/SECPxxxr1.h b/Libraries/LibCrypto/Curves/SECPxxxr1.h
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2023, Michiel Visser <[email protected]>
+ * Copyright (c) 2024, Altomani Gianluca <[email protected]>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -36,6 +37,16 @@ struct SECPxxxr1CurveParameters {
 struct SECPxxxr1Point {
     UnsignedBigInteger x;
     UnsignedBigInteger y;
+    size_t size;
+
+    static ErrorOr<ByteBuffer> scalar_to_bytes(UnsignedBigInteger const& a, size_t size)
+    {
+        auto a_bytes = TRY(ByteBuffer::create_uninitialized(a.byte_length()));
+        auto a_size = a.export_data(a_bytes.span());
+        VERIFY(a_size >= size);
+
+        return a_bytes.slice(a_size - size, size);
+    }
 
     static ErrorOr<SECPxxxr1Point> from_uncompressed(ReadonlyBytes data)
     {
@@ -46,16 +57,30 @@ struct SECPxxxr1Point {
         return SECPxxxr1Point {
             UnsignedBigInteger::import_data(data.slice(1, half_size)),
             UnsignedBigInteger::import_data(data.slice(1 + half_size, half_size)),
+            half_size,
         };
     }
 
+    ErrorOr<ByteBuffer> x_bytes() const
+    {
+        return scalar_to_bytes(x, size);
+    }
+
+    ErrorOr<ByteBuffer> y_bytes() const
+    {
+        return scalar_to_bytes(y, size);
+    }
+
     ErrorOr<ByteBuffer> to_uncompressed() const
     {
-        auto bytes = TRY(ByteBuffer::create_uninitialized(1 + x.byte_length() + y.byte_length()));
+        auto x = TRY(x_bytes());
+        auto y = TRY(y_bytes());
+
+        auto bytes = TRY(ByteBuffer::create_uninitialized(1 + (size * 2)));
         bytes[0] = 0x04; // uncompressed
-        auto x_size = x.export_data(bytes.span().slice(1));
-        auto y_size = y.export_data(bytes.span().slice(1 + x_size));
-        return bytes.slice(0, 1 + x_size + y_size);
+        memcpy(bytes.data() + 1, x.data(), size);
+        memcpy(bytes.data() + 1 + size, y.data(), size);
+        return bytes;
     }
 };
 
@@ -218,7 +243,11 @@ class SECPxxxr1 : public EllipticCurve {
     {
         VERIFY(scalar.byte_length() >= KEY_BYTE_SIZE);
 
-        return compute_coordinate_point(scalar, SECPxxxr1Point { UnsignedBigInteger::import_data(GENERATOR_POINT.data() + 1, KEY_BYTE_SIZE), UnsignedBigInteger::import_data(GENERATOR_POINT.data() + 1 + KEY_BYTE_SIZE, KEY_BYTE_SIZE) });
+        return compute_coordinate_point(scalar, SECPxxxr1Point {
+                                                    UnsignedBigInteger::import_data(GENERATOR_POINT.data() + 1, KEY_BYTE_SIZE),
+                                                    UnsignedBigInteger::import_data(GENERATOR_POINT.data() + 1 + KEY_BYTE_SIZE, KEY_BYTE_SIZE),
+                                                    KEY_BYTE_SIZE,
+                                                });
     }
 
     ErrorOr<ByteBuffer> compute_coordinate(ReadonlyBytes scalar_bytes, ReadonlyBytes point_bytes) override
@@ -248,8 +277,9 @@ class SECPxxxr1 : public EllipticCurve {
         auto result_point = TRY(compute_coordinate_internal(scalar_int, JacobianPoint { point_x_int, point_y_int, 1u }));
 
         return SECPxxxr1Point {
-            .x = storage_type_to_unsigned_big_integer(result_point.x),
-            .y = storage_type_to_unsigned_big_integer(result_point.y),
+            storage_type_to_unsigned_big_integer(result_point.x),
+            storage_type_to_unsigned_big_integer(result_point.y),
+            KEY_BYTE_SIZE,
         };
     }
 

diff --git a/Libraries/LibCrypto/PK/EC.cpp b/Libraries/LibCrypto/PK/EC.cpp
@@ -9,11 +9,6 @@
 #include <LibCrypto/ASN1/DER.h>
 #include <LibCrypto/Certificate/Certificate.h>
 
-namespace {
-// Used by ASN1 macros
-static String s_error_string;
-}
-
 namespace Crypto::PK {
 
 template<>
@@ -23,9 +18,8 @@ ErrorOr<ByteBuffer> ECPrivateKey<IntegerType>::export_as_der() const
     TRY(encoder.write_constructed(ASN1::Class::Universal, ASN1::Kind::Sequence, [&]() -> ErrorOr<void> {
         TRY(encoder.write(1u)); // version
 
-        auto d_bytes = TRY(ByteBuffer::create_uninitialized(m_d.byte_length()));
-        auto d_size = m_d.export_data(d_bytes.span());
-        TRY(encoder.write<ReadonlyBytes>(d_bytes.span().slice(0, d_size)));
+        auto d = TRY(d_bytes());
+        TRY(encoder.write<ReadonlyBytes>(d));
 
         if (m_parameters.has_value()) {
             TRY(encoder.write_constructed(ASN1::Class::Context, static_cast<ASN1::Kind>(0), [&]() -> ErrorOr<void> {
@@ -65,6 +59,7 @@ static ErrorOr<ECPublicKey<>> read_ec_public_key(ReadonlyBytes bytes, Vector<Str
         return ::Crypto::PK::ECPublicKey<> {
             UnsignedBigInteger::import_data(bytes.slice(1, half_size)),
             UnsignedBigInteger::import_data(bytes.slice(1 + half_size, half_size)),
+            half_size,
         };
     } else {
         ERROR_WITH_SCOPE("Unsupported public key format");
@@ -125,15 +120,15 @@ ErrorOr<EC::KeyPairType> EC::parse_ec_key(ReadonlyBytes der, bool is_private, Ve
 
                 keypair.public_key = maybe_public_key.release_value();
                 public_key = keypair.public_key;
-                if (keypair.public_key.x().byte_length() != private_key.byte_length() || keypair.public_key.y().byte_length() != private_key.byte_length()) {
+                if (keypair.public_key.scalar_size() != private_key_bytes.length()) {
                     ERROR_WITH_SCOPE("Invalid public key length");
                 }
 
                 EXIT_SCOPE();
             }
         }
 
-        keypair.private_key = ECPrivateKey { private_key, parameters, public_key };
+        keypair.private_key = ECPrivateKey { private_key, private_key_bytes.length(), parameters, public_key };
 
         EXIT_SCOPE();
         return keypair;

diff --git a/Libraries/LibCrypto/PK/EC.h b/Libraries/LibCrypto/PK/EC.h
@@ -9,48 +9,70 @@
 #include <AK/ByteBuffer.h>
 #include <AK/StringView.h>
 #include <LibCrypto/ASN1/DER.h>
+#include <LibCrypto/Curves/SECPxxxr1.h>
 #include <LibCrypto/PK/PK.h>
 
 namespace Crypto::PK {
 
 template<typename Integer = UnsignedBigInteger>
 class ECPublicKey {
 public:
-    ECPublicKey(Integer x, Integer y)
+    ECPublicKey(Integer x, Integer y, size_t scalar_size)
         : m_x(move(x))
         , m_y(move(y))
+        , m_scalar_size(scalar_size)
+    {
+    }
+
+    ECPublicKey(Curves::SECPxxxr1Point point)
+        : m_x(move(point.x))
+        , m_y(move(point.y))
+        , m_scalar_size(point.size)
     {
     }
 
     ECPublicKey()
         : m_x(0)
         , m_y(0)
+        , m_scalar_size(0)
+    {
+    }
+
+    size_t scalar_size() const { return m_scalar_size; }
+
+    ErrorOr<ByteBuffer> x_bytes() const
+    {
+        return Curves::SECPxxxr1Point::scalar_to_bytes(m_x, m_scalar_size);
+    }
+
+    ErrorOr<ByteBuffer> y_bytes() const
     {
+        return Curves::SECPxxxr1Point::scalar_to_bytes(m_y, m_scalar_size);
     }
 
-    Integer const& x() const { return m_x; }
-    Integer const& y() const { return m_y; }
+    Curves::SECPxxxr1Point to_secpxxxr1_point() const
+    {
+        return Curves::SECPxxxr1Point { m_x, m_y, m_scalar_size };
+    }
 
     ErrorOr<ByteBuffer> to_uncompressed() const
     {
-        auto bytes = TRY(ByteBuffer::create_uninitialized(1 + m_x.byte_length() + m_y.byte_length()));
-        bytes[0] = 0x04; // uncompressed
-        auto x_size = m_x.export_data(bytes.span().slice(1));
-        auto y_size = m_y.export_data(bytes.span().slice(1 + x_size));
-        return bytes.slice(0, 1 + x_size + y_size);
+        return to_secpxxxr1_point().to_uncompressed();
     }
 
 private:
     Integer m_x;
     Integer m_y;
+    size_t m_scalar_size;
 };
 
 // https://www.rfc-editor.org/rfc/rfc5915#section-3
 template<typename Integer = UnsignedBigInteger>
 class ECPrivateKey {
 public:
-    ECPrivateKey(Integer d, Optional<Vector<int>> parameters, Optional<ECPublicKey<Integer>> public_key)
+    ECPrivateKey(Integer d, size_t scalar_size, Optional<Vector<int>> parameters, Optional<ECPublicKey<Integer>> public_key)
         : m_d(move(d))
+        , m_scalar_size(scalar_size)
         , m_parameters(parameters)
         , m_public_key(public_key)
     {
@@ -59,13 +81,20 @@ class ECPrivateKey {
     ECPrivateKey() = default;
 
     Integer const& d() const { return m_d; }
+    ErrorOr<ByteBuffer> d_bytes() const
+    {
+        return Curves::SECPxxxr1Point::scalar_to_bytes(m_d, m_scalar_size);
+    }
+
     Optional<Vector<int> const&> parameters() const { return m_parameters; }
     Optional<ECPublicKey<Integer> const&> public_key() const { return m_public_key; }
 
     ErrorOr<ByteBuffer> export_as_der() const;
 
 private:
     Integer m_d;
+    size_t m_scalar_size;
+
     Optional<Vector<int>> m_parameters;
     Optional<ECPublicKey<Integer>> m_public_key;
 };

diff --git a/Libraries/LibCrypto/PK/RSA.cpp b/Libraries/LibCrypto/PK/RSA.cpp
@@ -13,11 +13,6 @@
 #include <LibCrypto/Certificate/Certificate.h>
 #include <LibCrypto/PK/RSA.h>
 
-namespace {
-// Used by ASN1 macros
-static String s_error_string;
-}
-
 namespace Crypto::PK {
 
 ErrorOr<RSA::KeyPairType> RSA::parse_rsa_key(ReadonlyBytes der, bool is_private, Vector<StringView> current_scope)

diff --git a/Libraries/LibTLS/HandshakeServer.cpp b/Libraries/LibTLS/HandshakeServer.cpp
@@ -458,8 +458,7 @@ ssize_t TLSv12::verify_ecdsa_server_key_exchange(ReadonlyBytes server_key_info_b
         dbgln("verify_ecdsa_server_key_exchange failed: Attempting to verify signature without certificates");
         return (i8)Error::NotSafe;
     }
-    auto server_public_key = m_context.certificates.first().public_key.ec;
-    auto server_point = Crypto::Curves::SECPxxxr1Point { server_public_key.x(), server_public_key.y() };
+    auto server_point = m_context.certificates.first().public_key.ec.to_secpxxxr1_point();
 
     auto message_result = ByteBuffer::create_uninitialized(64 + server_key_info_buffer.size());
     if (message_result.is_error()) {

diff --git a/Libraries/LibTLS/TLSv12.cpp b/Libraries/LibTLS/TLSv12.cpp
@@ -371,8 +371,7 @@ bool Context::verify_certificate_pair(Certificate const& subject, Certificate co
         return false;
     }
 
-    auto public_key = issuer.public_key.ec;
-    auto public_point = Crypto::Curves::SECPxxxr1Point { public_key.x(), public_key.y() };
+    auto public_point = issuer.public_key.ec.to_secpxxxr1_point();
 
     auto maybe_signature = Crypto::Curves::SECPxxxr1Signature::from_asn(subject.signature_value, {});
     if (maybe_signature.is_error()) {