Remove publicKey argument from Ed25519ExpandedPrivateKey.sign

Author: str4d

CHANGELOG.md

@@ -5,6 +5,17 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Security
+- `Ed25519ExpandedPrivateKey.sign` no longer takes a `publicKey` argument. The
+  previous API allowed the caller to control how the public key was cached in
+  memory, but it created an opportunity for misuse: if two signatures were
+  created using different public keys, the private scalar could be recovered
+  from the signatures (see [here][pubkey-2014] and [here][pubkey-2022] for
+  details). We now always cache the public key ourselves to provide a safer
+  signing API.
+
+[pubkey-2014]: https://github.com/jedisct1/libsodium/issues/170
+[pubkey-2022]: https://github.com/MystenLabs/ed25519-unsafe-libs
 
 ## [0.1.0] - 2020-04-13
 Initial release!

src/jmh/java/cafe/cryptography/ed25519/Ed25519Bench.java

@@ -33,22 +33,17 @@ public void prepare() {
         this.vk = this.sk.derivePublic();
         this.message = new byte[64];
         r.nextBytes(this.message);
-        this.signature = this.sk.expand().sign(this.message, this.vk);
+        this.signature = this.sk.expand().sign(this.message);
     }
 
     @Benchmark
     public Ed25519PublicKey keygen() {
         return Ed25519PrivateKey.generate(this.r).derivePublic();
     }
 
-    @Benchmark
-    public Ed25519ExpandedPrivateKey expand() {
-        return this.sk.expand();
-    }
-
     @Benchmark
     public Ed25519Signature sign() {
-        return this.expsk.sign(this.message, this.vk);
+        return this.expsk.sign(this.message);
     }
 
     @Benchmark

src/main/java/cafe/cryptography/ed25519/Ed25519ExpandedPrivateKey.java

@@ -24,31 +24,52 @@ public class Ed25519ExpandedPrivateKey {
     /**
      * The prefix component of the expanded Ed25519 private key.
      *
-     * Note that because the `final` keyword only makes the reference a constant, the
-     * contents of this byte[] could in theory be mutated (via reflection, as this field
-     * is private). This misunderstanding was a contributor to the "final" security bug in
-     * Google's Java implementation of Ed25519 [0]. However, the primary cause of that bug
-     * was their reuse of the prefix buffer to hold the result of calculating S; we are
-     * protected from that failure mode by the type-safe curve25519-elisabeth API.
+     * Note that because the `final` keyword only makes the reference a constant,
+     * the contents of this byte[] could in theory be mutated (via reflection, as
+     * this field is private). This misunderstanding was a contributor to the
+     * "final" security bug in Google's Java implementation of Ed25519 [0]. However,
+     * the primary cause of that bug was their reuse of the prefix buffer to hold
+     * the result of calculating S; we are protected from that failure mode by the
+     * type-safe curve25519-elisabeth API.
      *
      * [0] https://github.com/cryptosubtlety/final-security-bug
      */
     private final byte[] prefix;
 
+    /**
+     * The public key corresponding to this private key.
+     *
+     * We store the public key inside the expanded private key so that we always use
+     * the correct public key when creating signatures, while caching its
+     * computation along with the other expanded components.
+     *
+     * Version 0.1.0 of ed25519-elisabeth required the caller to provide the public
+     * key. This allowed the caller to control how the public key was cached in
+     * memory, but it created an opportunity for misuse: if two signatures were
+     * created using different public keys, the private scalar could be recovered
+     * from the signatures [0] [1]. We now always cache the public key ourselves to
+     * provide a safer signing API.
+     *
+     * [0] https://github.com/jedisct1/libsodium/issues/170
+     * [1] https://github.com/MystenLabs/ed25519-unsafe-libs
+     */
+    private final Ed25519PublicKey publicKey;
+
     Ed25519ExpandedPrivateKey(Scalar s, byte[] prefix) {
         this.s = s;
         this.prefix = prefix;
+        EdwardsPoint A = Constants.ED25519_BASEPOINT_TABLE.multiply(this.s);
+        this.publicKey = new Ed25519PublicKey(A);
     }
 
     /**
-     * Derive the Ed25519 public key corresponding to this expanded private key.
+     * Returns the Ed25519 public key corresponding to this expanded private key.
      *
      * @return the public key.
      */
     @NotNull
     public Ed25519PublicKey derivePublic() {
-        EdwardsPoint A = Constants.ED25519_BASEPOINT_TABLE.multiply(this.s);
-        return new Ed25519PublicKey(A);
+        return this.publicKey;
     }
 
     /**
@@ -57,8 +78,8 @@ public Ed25519PublicKey derivePublic() {
      * @return the signature.
      */
     @NotNull
-    public Ed25519Signature sign(@NotNull byte[] message, @NotNull Ed25519PublicKey publicKey) {
-        return this.sign(message, 0, message.length, publicKey);
+    public Ed25519Signature sign(@NotNull byte[] message) {
+        return this.sign(message, 0, message.length);
     }
 
     /**
@@ -67,7 +88,7 @@ public Ed25519Signature sign(@NotNull byte[] message, @NotNull Ed25519PublicKey
      * @return the signature.
      */
     @NotNull
-    public Ed25519Signature sign(@NotNull byte[] message, int offset, int length, @NotNull Ed25519PublicKey publicKey) {
+    public Ed25519Signature sign(@NotNull byte[] message, int offset, int length) {
         // @formatter:off
         // RFC 8032, section 5.1:
         //   PH(x)   | x (i.e., the identity function)
@@ -101,7 +122,7 @@ public Ed25519Signature sign(@NotNull byte[] message, int offset, int length, @N
         // @formatter:on
         h.reset();
         h.update(R.toByteArray());
-        h.update(publicKey.toByteArray());
+        h.update(this.publicKey.toByteArray());
         h.update(message, offset, length);
         Scalar k = Scalar.fromBytesModOrderWide(h.digest());
 

src/test/java/cafe/cryptography/ed25519/Ed25519Rfc8032TestVectors.java

@@ -161,11 +161,11 @@ public void derivePublic() {
 
     @Test
     public void testSign() {
-        assertThat(TEST_1_SK.expand().sign(TEST_1_MSG, TEST_1_VK), is(TEST_1_SIG));
-        assertThat(TEST_2_SK.expand().sign(TEST_2_MSG, TEST_2_VK), is(TEST_2_SIG));
-        assertThat(TEST_3_SK.expand().sign(TEST_3_MSG, TEST_3_VK), is(TEST_3_SIG));
-        assertThat(TEST_1024_SK.expand().sign(TEST_1024_MSG, TEST_1024_VK), is(TEST_1024_SIG));
-        assertThat(TEST_SHA_SK.expand().sign(TEST_SHA_MSG, TEST_SHA_VK), is(TEST_SHA_SIG));
+        assertThat(TEST_1_SK.expand().sign(TEST_1_MSG), is(TEST_1_SIG));
+        assertThat(TEST_2_SK.expand().sign(TEST_2_MSG), is(TEST_2_SIG));
+        assertThat(TEST_3_SK.expand().sign(TEST_3_MSG), is(TEST_3_SIG));
+        assertThat(TEST_1024_SK.expand().sign(TEST_1024_MSG), is(TEST_1024_SIG));
+        assertThat(TEST_SHA_SK.expand().sign(TEST_SHA_MSG), is(TEST_SHA_SIG));
     }
 
     @Test

src/test/java/cafe/cryptography/ed25519/Ed25519TestVectors.java

@@ -88,7 +88,7 @@ public void testSign() {
         for (TestTuple testCase : testCases) {
             Ed25519PrivateKey sk = Ed25519PrivateKey.fromByteArray(testCase.sk);
             assertThat("Test case " + testCase.caseNum + " failed",
-                    sk.expand().sign(testCase.message, sk.derivePublic()).toByteArray(), is(testCase.signature));
+                    sk.expand().sign(testCase.message).toByteArray(), is(testCase.signature));
         }
     }