Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 7702 signature bound checks #7641

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/acceptance-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ concurrency:
cancel-in-progress: true

env:
GRADLE_OPTS: "-Xmx6g"
GRADLE_OPTS: "-Xmx7g"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to reduce the failure rate of ATs, then increase the number of total runners, maybe you can move 2 from reference-tests, that are faster, here

total-runners: 12

jobs:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,12 @@ public SECPSignature createSignature(final BigInteger r, final BigInteger s, fin
return SECPSignature.create(r, s, recId, curveOrder);
}

@Override
public CodeDelegationSignature createCodeDelegationSignature(
final BigInteger r, final BigInteger s, final long yParity) {
return CodeDelegationSignature.create(r, s, yParity);
}

@Override
public SECPSignature decodeSignature(final Bytes bytes) {
return SECPSignature.decode(bytes, curveOrder);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.crypto;

import static com.google.common.base.Preconditions.checkNotNull;

import java.math.BigInteger;

/** Secp signature with code delegation. */
public class CodeDelegationSignature extends SECPSignature {
private static final BigInteger TWO_POW_256 = BigInteger.TWO.pow(256);

/**
* Instantiates a new SECPSignature.
*
* @param r the r part of the signature
* @param s the s part of the signature
* @param yParity the parity of the y coordinate of the public key
*/
public CodeDelegationSignature(final BigInteger r, final BigInteger s, final byte yParity) {
super(r, s, yParity);
}

/**
* Create a new CodeDelegationSignature.
*
* @param r the r part of the signature
* @param s the s part of the signature
* @param yParity the parity of the y coordinate of the public key
* @return the new CodeDelegationSignature
*/
public static CodeDelegationSignature create(
final BigInteger r, final BigInteger s, final long yParity) {
checkNotNull(r);
checkNotNull(s);

if (r.compareTo(TWO_POW_256) >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could be a bit more precise here and check whether r and s are smaller than the prime of the curve, which is p = 2^256-2^32-977. But that still does not guarantee that (r, s) is a point on the curve.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code this could be any curve that is being used here, so 2^256 might be better for an initial check?

throw new IllegalArgumentException("Invalid 'r' value, should be < 2^256 but got " + r);
}

if (s.compareTo(TWO_POW_256) >= 0) {
throw new IllegalArgumentException("Invalid 's' value, should be < 2^256 but got " + s);
}

return new CodeDelegationSignature(r, s, (byte) yParity);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,17 @@ Optional<SECPPublicKey> recoverPublicKeyFromSignature(
*/
SECPSignature createSignature(final BigInteger r, final BigInteger s, final byte recId);

/**
* Create code delegation signature which have different bounds than transaction signatures.
*
* @param r the r part of the signature
* @param s the s part of the signature
* @param yParity the parity of the y coordinate of the public key
* @return the code delegation signature
*/
CodeDelegationSignature createCodeDelegationSignature(
final BigInteger r, final BigInteger s, final long yParity);

/**
* Decode secp signature.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.crypto;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;

import java.math.BigInteger;

import org.junit.jupiter.api.Test;

class CodeDelegationSignatureTest {

private static final BigInteger TWO_POW_256 = BigInteger.valueOf(2).pow(256);

@Test
void testValidInputs() {
BigInteger r = BigInteger.ONE;
BigInteger s = BigInteger.TEN;
long yParity = 1L;

CodeDelegationSignature result = CodeDelegationSignature.create(r, s, yParity);

assertThat(r).isEqualTo(result.getR());
assertThat(s).isEqualTo(result.getS());
assertThat((byte) yParity).isEqualTo(result.getRecId());
}

@Test
void testNullRValue() {
BigInteger s = BigInteger.TEN;
long yParity = 0L;

assertThatExceptionOfType(NullPointerException.class)
.isThrownBy(() -> CodeDelegationSignature.create(null, s, yParity));
}

@Test
void testNullSValue() {
BigInteger r = BigInteger.ONE;
long yParity = 0L;

assertThatExceptionOfType(NullPointerException.class)
.isThrownBy(() -> CodeDelegationSignature.create(r, null, yParity));
}

@Test
void testRValueExceedsTwoPow256() {
BigInteger r = TWO_POW_256;
BigInteger s = BigInteger.TEN;
long yParity = 0L;

assertThatExceptionOfType(IllegalArgumentException.class)
.isThrownBy(() -> CodeDelegationSignature.create(r, s, yParity))
.withMessageContainingAll("Invalid 'r' value, should be < 2^256");
}

@Test
void testSValueExceedsTwoPow256() {
BigInteger r = BigInteger.ONE;
BigInteger s = TWO_POW_256;
long yParity = 0L;

assertThatExceptionOfType(IllegalArgumentException.class)
.isThrownBy(() -> CodeDelegationSignature.create(r, s, yParity))
.withMessageContainingAll("Invalid 's' value, should be < 2^256");
}

@Test
void testValidYParityZero() {
BigInteger r = BigInteger.ONE;
BigInteger s = BigInteger.TEN;
long yParity = 0L;

CodeDelegationSignature result = CodeDelegationSignature.create(r, s, yParity);

assertThat(r).isEqualTo(result.getR());
assertThat(s).isEqualTo(result.getS());
assertThat((byte) yParity).isEqualTo(result.getRecId());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,14 @@ public static CodeDelegation decodeInnerPayload(final RLPInput input) {
final Address address = Address.wrap(input.readBytes());
final long nonce = input.readLongScalar();

final byte yParity = (byte) input.readUnsignedByteScalar();
final long yParity = input.readUnsignedIntScalar();
final BigInteger r = input.readUInt256Scalar().toUnsignedBigInteger();
final BigInteger s = input.readUInt256Scalar().toUnsignedBigInteger();

input.leaveList();

final SECPSignature signature = SIGNATURE_ALGORITHM.get().createSignature(r, s, yParity);
final SECPSignature signature =
SIGNATURE_ALGORITHM.get().createCodeDelegationSignature(r, s, yParity);

return new org.hyperledger.besu.ethereum.core.CodeDelegation(
chainId, address, nonce, signature);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@
*/
public class MainnetTransactionValidator implements TransactionValidator {

public static final BigInteger TWO_POW_256 = BigInteger.TWO.pow(256);

private final GasCalculator gasCalculator;
private final GasLimitCalculator gasLimitCalculator;
private final FeeMarket feeMarket;
Expand Down Expand Up @@ -163,6 +165,12 @@ private static ValidationResult<TransactionInvalidReason> validateCodeDelegation
.map(
codeDelegations -> {
for (CodeDelegation codeDelegation : codeDelegations) {
if (codeDelegation.chainId().compareTo(TWO_POW_256) >= 0) {
throw new IllegalArgumentException(
"Invalid 'chainId' value, should be < 2^256 but got "
+ codeDelegation.chainId());
}

if (codeDelegation.signature().getS().compareTo(halfCurveOrder) > 0) {
return ValidationResult.invalid(
TransactionInvalidReason.INVALID_SIGNATURE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,16 @@ void shouldDecodeInnerPayloadWithChainIdZero() {
assertThat(signature.getS().toString(16))
.isEqualTo("3c8a25b2becd6e666f69803d1ae3322f2e137b7745c2c7f19da80f993ffde4df");
}

@Test
void shouldDecodeInnerPayloadWhenSignatureIsZero() {
final BytesValueRLPInput input =
new BytesValueRLPInput(
Bytes.fromHexString(
"0xdf8501a1f0ff5a947a40026a3b9a41754a95eec8c92c6b99886f440c5b808080"),
true);
final CodeDelegation authorization = CodeDelegationTransactionDecoder.decodeInnerPayload(input);

assertThat(authorization.chainId()).isEqualTo(new BigInteger("01a1f0ff5a", 16));
}
}
Loading