From 49545ce47d550fed807522dff86546c812ccbbac Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 19 Jun 2022 14:38:27 +0000 Subject: [PATCH] Remove `publicKey` argument from `Ed25519ExpandedPrivateKey.sign` --- CHANGELOG.md | 11 +++++ .../cryptography/ed25519/Ed25519Bench.java | 9 +--- .../ed25519/Ed25519ExpandedPrivateKey.java | 47 ++++++++++++++----- .../ed25519/Ed25519Rfc8032TestVectors.java | 10 ++-- .../ed25519/Ed25519TestVectors.java | 2 +- 5 files changed, 53 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 448cb5f..915456f 100644 --- a/CHANGELOG.md +++ b/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! diff --git a/src/jmh/java/cafe/cryptography/ed25519/Ed25519Bench.java b/src/jmh/java/cafe/cryptography/ed25519/Ed25519Bench.java index b90ebce..cac8b88 100644 --- a/src/jmh/java/cafe/cryptography/ed25519/Ed25519Bench.java +++ b/src/jmh/java/cafe/cryptography/ed25519/Ed25519Bench.java @@ -33,7 +33,7 @@ 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 @@ -41,14 +41,9 @@ 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 diff --git a/src/main/java/cafe/cryptography/ed25519/Ed25519ExpandedPrivateKey.java b/src/main/java/cafe/cryptography/ed25519/Ed25519ExpandedPrivateKey.java index a8ad2d9..10e38e1 100644 --- a/src/main/java/cafe/cryptography/ed25519/Ed25519ExpandedPrivateKey.java +++ b/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()); diff --git a/src/test/java/cafe/cryptography/ed25519/Ed25519Rfc8032TestVectors.java b/src/test/java/cafe/cryptography/ed25519/Ed25519Rfc8032TestVectors.java index 31d19ee..f3fb922 100644 --- a/src/test/java/cafe/cryptography/ed25519/Ed25519Rfc8032TestVectors.java +++ b/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 diff --git a/src/test/java/cafe/cryptography/ed25519/Ed25519TestVectors.java b/src/test/java/cafe/cryptography/ed25519/Ed25519TestVectors.java index 1886c9c..4e32f13 100644 --- a/src/test/java/cafe/cryptography/ed25519/Ed25519TestVectors.java +++ b/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)); } }