Skip to content

Commit 38e8725

Browse files
garyschultegaryschulte
authored
Add explict check for points-on-curve in p256_verify bouncycastle implementation (#9146)
* add explict check and test coverage for points-on-curve in the bouncy-castle implementation of p256Verify * update tests for r' exceeding N and exercising native SignatureAlgorithm cases for public key not on curve. Signed-off-by: garyschulte <garyschulte@gmail.com>
1 parent a322c1c commit 38e8725

2 files changed

Lines changed: 124 additions & 6 deletions

File tree

‎evm/src/main/java/org/hyperledger/besu/evm/precompile/P256VerifyPrecompiledContract.java‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,9 @@ public class P256VerifyPrecompiledContract extends AbstractPrecompiledContract {
4242
private static final Bytes INVALID = Bytes.EMPTY;
4343
private static final int SECP256R1_INPUT_LENGTH = 160;
4444

45-
private static final X9ECParameters R1_PARAMS = SECNamedCurves.getByName("secp256r1");
46-
private static final BigInteger N = R1_PARAMS.getN();
47-
private static final BigInteger P = R1_PARAMS.getCurve().getField().getCharacteristic();
45+
static final X9ECParameters R1_PARAMS = SECNamedCurves.getByName("secp256r1");
46+
static final BigInteger N = R1_PARAMS.getN();
47+
static final BigInteger P = R1_PARAMS.getCurve().getField().getCharacteristic();
4848

4949
/** The constant useNative. */
5050
// use the BoringSSL native library implementation, if it is available
@@ -190,7 +190,7 @@ private static PrecompileInputResultTuple computeNative(final Bytes input) {
190190

191191
// This may still use native SECP256R1 depending on how SignatureAlgorithm is configured
192192
@NotNull
193-
private PrecompileInputResultTuple computeDefault(final Bytes input) {
193+
PrecompileInputResultTuple computeDefault(final Bytes input) {
194194
final Bytes messageHash = input.slice(0, 32);
195195
final Bytes rBytes = input.slice(32, 32);
196196
final Bytes sBytes = input.slice(64, 32);
@@ -222,7 +222,14 @@ private PrecompileInputResultTuple computeDefault(final Bytes input) {
222222
return new PrecompileInputResultTuple(
223223
enableResultCaching ? input.copy() : input, PrecompileContractResult.success(INVALID));
224224
}
225-
225+
try {
226+
final org.bouncycastle.math.ec.ECPoint ecPoint = R1_PARAMS.getCurve().createPoint(qx, qy);
227+
signatureAlgorithm.getCurve().validatePublicPoint(ecPoint);
228+
} catch (IllegalArgumentException e) {
229+
LOG.trace("Public key not on curve: {}", e.getMessage());
230+
return new PrecompileInputResultTuple(
231+
enableResultCaching ? input.copy() : input, PrecompileContractResult.success(INVALID));
232+
}
226233
// Create the signature; recID is not used in verification - use 0
227234
final SECPSignature signature = signatureAlgorithm.createSignature(r, s, (byte) 0);
228235
final SECPPublicKey publicKey = signatureAlgorithm.createPublicKey(pubKeyBytes);

‎evm/src/test/java/org/hyperledger/besu/evm/precompile/P256VerifyPrecompiledContractTest.java‎

Lines changed: 112 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,11 @@
1515
package org.hyperledger.besu.evm.precompile;
1616

1717
import static org.junit.jupiter.api.Assertions.assertEquals;
18+
import static org.junit.jupiter.api.Assertions.assertTrue;
1819
import static org.mockito.Mockito.mock;
1920

21+
import org.hyperledger.besu.crypto.SECP256R1;
22+
import org.hyperledger.besu.crypto.SignatureAlgorithm;
2023
import org.hyperledger.besu.evm.frame.MessageFrame;
2124
import org.hyperledger.besu.evm.gascalculator.SpuriousDragonGasCalculator;
2225

@@ -27,15 +30,17 @@
2730

2831
import com.fasterxml.jackson.databind.ObjectMapper;
2932
import org.apache.tuweni.bytes.Bytes;
33+
import org.apache.tuweni.bytes.Bytes32;
3034
import org.junit.jupiter.api.BeforeAll;
3135
import org.junit.jupiter.api.Test;
3236
import org.junit.jupiter.params.ParameterizedTest;
3337
import org.junit.jupiter.params.provider.MethodSource;
3438

3539
class P256VerifyPrecompiledContractTest {
3640

41+
private static final SignatureAlgorithm secp256R1 = new SECP256R1();
3742
private static final P256VerifyPrecompiledContract contract =
38-
new P256VerifyPrecompiledContract(new SpuriousDragonGasCalculator());
43+
new P256VerifyPrecompiledContract(new SpuriousDragonGasCalculator(), secp256R1);
3944

4045
private static final MessageFrame messageFrame = mock(MessageFrame.class);
4146

@@ -87,4 +92,110 @@ void testP256Verify(final TestCase testCase) {
8792
void sanityCheck() {
8893
assertEquals(6_900L, contract.gasRequirement(Bytes.wrap(new byte[128])));
8994
}
95+
96+
@Test
97+
void testInvalidCurvePointReturnsInvalid() {
98+
// Test using a realistic attack scenario:
99+
// Create r,s values that would verify against an invalid public key point
100+
// The point is in the field but NOT on the secp256r1 curve
101+
102+
String messageHash = "44acf6b7e36c1342c2c5897204fe09504e1e2efb1a900377dbc4e7a6a133ec56";
103+
104+
// Point in field but NOT on curve: these coordinates are valid field elements
105+
// but do not satisfy the secp256r1 curve equation y² = x³ - 3x + b
106+
String invalidPubKeyX = "79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798";
107+
String invalidPubKeyY = "b7c52588d95c3b9aa25b0403f1eef75702e84bb7597aabe663b82f6f04ef2777";
108+
109+
// Generate r,s values that could theoretically verify with this invalid point
110+
// r component from a valid signature
111+
String r = "c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5";
112+
// s component that would correspond to this scenario
113+
String s = "30dae23890abb63e378e003d7f1d5006ab23cc7b3b65b3d0c7b45c7e1e2e08b9";
114+
115+
String input = messageHash + r + s + invalidPubKeyX + invalidPubKeyY;
116+
Bytes inputBytes = Bytes.fromHexString(input);
117+
118+
// call default implementation:
119+
secp256R1.disableNative();
120+
var defaultResult = contract.computeDefault(inputBytes);
121+
// call default native implementation:
122+
secp256R1.maybeEnableNative();
123+
var defaultMaybeNativeResult = contract.computeDefault(inputBytes);
124+
125+
// verify boringssl precompile-specific implementation
126+
// this should be configured/run statically, adding the call here just for clarity
127+
P256VerifyPrecompiledContract.maybeEnableNativeBoringSSL();
128+
var maybeNativeResult = contract.computePrecompile(inputBytes, messageFrame);
129+
130+
// Should return empty bytes (INVALID) because curve validation catches
131+
// the invalid point before signature verification attempts
132+
assertTrue(
133+
defaultResult.cachedResult().isSuccessful(),
134+
"Invalid curve point should succeed with empty result");
135+
assertEquals(
136+
Bytes.EMPTY,
137+
defaultResult.cachedResult().output(),
138+
"Invalid curve point should return empty result");
139+
assertTrue(
140+
defaultMaybeNativeResult.cachedResult().isSuccessful(),
141+
"Invalid curve point should succeed with empty result");
142+
assertEquals(
143+
Bytes.EMPTY,
144+
defaultMaybeNativeResult.cachedResult().output(),
145+
"Invalid curve point should return empty result");
146+
assertTrue(
147+
maybeNativeResult.isSuccessful(),
148+
"Invalid curve point should succeed with empty result for native implementation");
149+
assertEquals(
150+
Bytes.EMPTY,
151+
maybeNativeResult.output(),
152+
"Invalid curve point should return empty result for native implementation");
153+
}
154+
155+
@Test
156+
void testModularComparisonWhenRPrimeExceedsN() {
157+
158+
// parameters borrowed from execution-spec-tests for R' exceeding N:
159+
// https://github.com/ethereum/execution-spec-tests/blob/61f8ac90841770d9fc407f1498ec0e5229b27508/tests/osaka/eip7951_p256verify_precompiles/test_p256verify.py#L303-L315
160+
String messageHash = "BB5A52F42F9C9261ED4361F59422A1E30036E7C32B270C8807A419FECA605023";
161+
String r = "000000000000000000000000000000004319055358E8617B0C46353D039CDAAB";
162+
String s = "FFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC63254E";
163+
String pubKeyX = "0AD99500288D466940031D72A9F5445A4D43784640855BF0A69874D2DE5FE103";
164+
String pubKeyY = "C5011E6EF2C42DCD50D5D3D29F99AE6EBA2C80C9244F4C5422F0979FF0C3BA5E";
165+
String input = messageHash + r + s + pubKeyX + pubKeyY;
166+
Bytes inputBytes = Bytes.fromHexString(input);
167+
168+
// call the signaturebased impl explicitly, with native disabled:
169+
secp256R1.disableNative();
170+
var defaultResult = contract.computeDefault(inputBytes);
171+
// call the signaturebased impl explicitly, with openssl native (maybe)enabled:
172+
secp256R1.maybeEnableNative();
173+
var defaultMaybeNativeResult = contract.computeDefault(inputBytes);
174+
175+
// maybeNative implementation:
176+
secp256R1.maybeEnableNative();
177+
var maybeNativeResult = contract.computePrecompile(inputBytes, messageFrame);
178+
179+
// This signature should be VALID if modular comparison is implemented correctly
180+
// The verification should compute R' with x-coordinate that may exceed n,
181+
// but R'.x ≡ r (mod n) should still hold true
182+
assertTrue(
183+
defaultResult.cachedResult().isSuccessful(),
184+
"Valid signature with R'.x > n should verify correctly via modular comparison");
185+
assertEquals(
186+
Bytes32.leftPad(Bytes.of(1)),
187+
defaultResult.cachedResult().output(),
188+
"Valid signature with R'.x > n should verify correctly via modular comparison");
189+
assertTrue(
190+
defaultMaybeNativeResult.cachedResult().isSuccessful(),
191+
"Valid signature with R'.x > n should verify correctly via modular comparison for native SignatureAlgorithm");
192+
assertEquals(
193+
Bytes32.leftPad(Bytes.of(1)),
194+
defaultMaybeNativeResult.cachedResult().output(),
195+
"Valid signature with R'.x > n should verify correctly via modular comparison for native SignatureAlgorithm");
196+
assertEquals(
197+
Bytes32.leftPad(Bytes.of(1)),
198+
maybeNativeResult.output(),
199+
"Valid signature with R'.x > n should verify correctly via modular comparison with Native implementation");
200+
}
90201
}

0 commit comments

Comments
 (0)