Skip to content

Commit 23625fc

Browse files
Federico2014Federico2014
committed
fix(crypto): address review findings
1 parent 4f3c68b commit 23625fc

6 files changed

Lines changed: 59 additions & 60 deletions

File tree

‎crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java‎

Lines changed: 8 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
import java.util.Objects;
1818
import javax.annotation.Nullable;
1919
import lombok.extern.slf4j.Slf4j;
20-
import org.bouncycastle.asn1.ASN1InputStream;
2120
import org.bouncycastle.asn1.ASN1Integer;
2221
import org.bouncycastle.asn1.ASN1Primitive;
2322
import org.bouncycastle.asn1.ASN1Sequence;
@@ -161,11 +160,10 @@ public SM2(@Nullable PrivateKey privKey, ECPoint pub) {
161160
+ privKey.getAlgorithm());
162161
}
163162

164-
if (pub == null) {
165-
throw new IllegalArgumentException("Public key should not be null");
166-
} else {
167-
this.pub = pub;
163+
if (pub == null || pub.isInfinity() || !pub.isValid()) {
164+
throw new IllegalArgumentException("Public key is not a valid point on SM2 curve.");
168165
}
166+
this.pub = pub;
169167
}
170168

171169
/**
@@ -346,9 +344,9 @@ public static byte[] signatureToKeyBytes(byte[] messageHash, String signatureBas
346344
throw new SignatureException("Could not decode base64", e);
347345
}
348346
// Parse the signature bytes into r/s and the selector value.
349-
if (signatureEncoded.length < 65) {
350-
throw new SignatureException("Signature truncated, expected 65 " +
351-
"bytes and got " + signatureEncoded.length);
347+
if (signatureEncoded.length != 65) {
348+
throw new SignatureException("Invalid signature length, expected 65 bytes and got "
349+
+ signatureEncoded.length);
352350
}
353351

354352
return signatureToKeyBytes(
@@ -721,42 +719,7 @@ public static boolean verify(byte[] data, byte[] signature, byte[] pub) {
721719
}
722720
}
723721

724-
/**
725-
* Verifies a DER-encoded SM2 signature against the message bytes using the
726-
* public key bytes.
727-
*
728-
* @param message message bytes to verify
729-
* @param signature DER-encoded signature
730-
* @param pub public key bytes to use
731-
* @return false when the signature is malformed or invalid
732-
*/
733-
public static boolean verifyMessage(byte[] message, byte[] signature, byte[] pub) {
734-
return verifyMessage(message, signature, pub, null);
735-
}
736722

737-
/**
738-
* Verifies a DER-encoded SM2 signature against the message bytes using the
739-
* public key bytes.
740-
*
741-
* @param message message bytes to verify
742-
* @param signature DER-encoded signature
743-
* @param pub public key bytes to use
744-
* @param userID optional user identifier
745-
* @return false when the signature is malformed or invalid
746-
*/
747-
public static boolean verifyMessage(byte[] message, byte[] signature, byte[] pub,
748-
@Nullable String userID) {
749-
if (message == null || signature == null || pub == null) {
750-
return false;
751-
}
752-
try {
753-
SM2 key = SM2.fromPublicOnly(pub);
754-
byte[] messageHash = key.getSM2SignerForHash().generateSM3Hash(message);
755-
return verify(messageHash, SM2Signature.decodeFromDER(signature), pub);
756-
} catch (IllegalArgumentException | SignatureException e) {
757-
return false;
758-
}
759-
}
760723

761724
/**
762725
* Returns true if the given pubkey is canonical, i.e. the correct length taking
@@ -988,8 +951,8 @@ public static SM2Signature decodeFromDER(byte[] signature) throws SignatureExcep
988951
throw new SignatureException("Invalid DER signature length");
989952
}
990953

991-
try (ASN1InputStream inputStream = new ASN1InputStream(signature)) {
992-
ASN1Primitive primitive = inputStream.readObject();
954+
try {
955+
ASN1Primitive primitive = ASN1Primitive.fromByteArray(signature);
993956
if (!(primitive instanceof ASN1Sequence)) {
994957
throw new SignatureException("Invalid DER signature format");
995958
}

‎crypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package org.tron.common.crypto.sm2;
22

33
import java.math.BigInteger;
4-
import java.security.SecureRandom;
54
import javax.annotation.Nullable;
65
import org.bouncycastle.crypto.CipherParameters;
76
import org.bouncycastle.crypto.Digest;
@@ -186,8 +185,9 @@ public boolean verifySignature(byte[] message, BigInteger r, BigInteger s,
186185
* verify the hash signature
187186
*/
188187
public boolean verifyHashSignature(byte[] hash, BigInteger r, BigInteger s) {
189-
if (ByteArray.isEmpty(hash)) {
190-
throw new IllegalArgumentException("Hash cannot be empty");
188+
if (ByteArray.isEmpty(hash) || hash.length != 32) {
189+
throw new IllegalArgumentException("Expected 32 byte input to "
190+
+ "SM2 signature, not " + (hash == null ? "null" : hash.length));
191191
}
192192
if (r == null || s == null) {
193193
throw new IllegalArgumentException("R or S cannot be null");

‎framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,21 +80,25 @@ public static LocalWitnesses initFromKeystore(
8080
}
8181

8282
List<String> privateKeys = new ArrayList<>();
83+
byte[] privKeyBytes = null;
8384
try {
8485
Credentials credentials = WalletUtils.loadCredentials(pwd, new File(fileName));
8586
SignInterface sign = credentials.getSignInterface();
86-
byte[] privKeyBytes = sign.getPrivateKey();
87-
if (!SignUtils.isValidPrivateKey(privKeyBytes, Args.getInstance().isECKeyCryptoEngine())) {
88-
Arrays.fill(privKeyBytes, (byte) 0);
87+
privKeyBytes = sign.getPrivateKey();
88+
if (privKeyBytes == null
89+
|| !SignUtils.isValidPrivateKey(privKeyBytes, Args.getInstance().isECKeyCryptoEngine())) {
8990
throw new TronError(
9091
"Keystore contains an invalid private key",
9192
TronError.ErrCode.WITNESS_KEYSTORE_LOAD);
9293
}
9394
privateKeys.add(ByteArray.toHexString(privKeyBytes));
94-
Arrays.fill(privKeyBytes, (byte) 0);
9595
} catch (IOException | CipherException e) {
9696
logger.error("Witness node start failed!");
9797
throw new TronError(e, TronError.ErrCode.WITNESS_KEYSTORE_LOAD);
98+
} finally {
99+
if (privKeyBytes != null) {
100+
Arrays.fill(privKeyBytes, (byte) 0);
101+
}
98102
}
99103

100104
LocalWitnesses witnesses = new LocalWitnesses();

‎framework/src/main/java/org/tron/core/services/RpcApiService.java‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -722,8 +722,9 @@ public void isSpend(NoteParameters request, StreamObserver<SpendResult> response
722722
public void scanShieldedTRC20NotesByIvk(IvkDecryptTRC20Parameters request,
723723
StreamObserver<DecryptNotesTRC20> responseObserver) {
724724
if (!request.getEventsList().isEmpty()) {
725-
responseObserver.onError(getRunTimeException(
726-
new IllegalArgumentException("'events' field is deprecated and no longer supported")));
725+
responseObserver.onError(Status.INVALID_ARGUMENT
726+
.withDescription("'events' field is deprecated and no longer supported")
727+
.asRuntimeException());
727728
return;
728729
}
729730
long startNum = request.getStartBlockIndex();
@@ -747,8 +748,9 @@ public void scanShieldedTRC20NotesByIvk(IvkDecryptTRC20Parameters request,
747748
public void scanShieldedTRC20NotesByOvk(OvkDecryptTRC20Parameters request,
748749
StreamObserver<DecryptNotesTRC20> responseObserver) {
749750
if (!request.getEventsList().isEmpty()) {
750-
responseObserver.onError(getRunTimeException(
751-
new IllegalArgumentException("'events' field is deprecated and no longer supported")));
751+
responseObserver.onError(Status.INVALID_ARGUMENT
752+
.withDescription("'events' field is deprecated and no longer supported")
753+
.asRuntimeException());
752754
return;
753755
}
754756
long startNum = request.getStartBlockIndex();
@@ -2418,8 +2420,9 @@ public void scanShieldedTRC20NotesByIvk(
24182420
IvkDecryptTRC20Parameters request,
24192421
StreamObserver<org.tron.api.GrpcAPI.DecryptNotesTRC20> responseObserver) {
24202422
if (!request.getEventsList().isEmpty()) {
2421-
responseObserver.onError(getRunTimeException(
2422-
new IllegalArgumentException("'events' field is deprecated and no longer supported")));
2423+
responseObserver.onError(Status.INVALID_ARGUMENT
2424+
.withDescription("'events' field is deprecated and no longer supported")
2425+
.asRuntimeException());
24232426
return;
24242427
}
24252428
long startNum = request.getStartBlockIndex();
@@ -2448,8 +2451,9 @@ public void scanShieldedTRC20NotesByOvk(
24482451
OvkDecryptTRC20Parameters request,
24492452
StreamObserver<org.tron.api.GrpcAPI.DecryptNotesTRC20> responseObserver) {
24502453
if (!request.getEventsList().isEmpty()) {
2451-
responseObserver.onError(getRunTimeException(
2452-
new IllegalArgumentException("'events' field is deprecated and no longer supported")));
2454+
responseObserver.onError(Status.INVALID_ARGUMENT
2455+
.withDescription("'events' field is deprecated and no longer supported")
2456+
.asRuntimeException());
24532457
return;
24542458
}
24552459
long startNum = request.getStartBlockIndex();

‎framework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@
88
import java.io.UnsupportedEncodingException;
99
import javax.annotation.Resource;
1010
import org.apache.http.client.methods.HttpPost;
11+
import org.junit.AfterClass;
1112
import org.junit.Assert;
13+
import org.junit.BeforeClass;
1214
import org.junit.Test;
1315
import org.springframework.mock.web.MockHttpServletRequest;
1416
import org.springframework.mock.web.MockHttpServletResponse;
@@ -18,15 +20,27 @@
1820

1921
public class CreateSpendAuthSigServletTest extends BaseTest {
2022

23+
private static boolean origShieldedApi;
24+
2125
static {
2226
Args.setParam(
2327
new String[]{
2428
"--output-directory", dbPath(),
2529
}, TestConstants.TEST_CONF
2630
);
31+
}
32+
33+
@BeforeClass
34+
public static void enableShieldedApi() {
35+
origShieldedApi = Args.getInstance().allowShieldedTransactionApi;
2736
Args.getInstance().allowShieldedTransactionApi = true;
2837
}
2938

39+
@AfterClass
40+
public static void restoreShieldedApi() {
41+
Args.getInstance().allowShieldedTransactionApi = origShieldedApi;
42+
}
43+
3044
@Resource
3145
private CreateSpendAuthSigServlet createSpendAuthSigServlet;
3246

‎framework/src/test/java/org/tron/core/zksnark/MerkleContainerTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
import com.google.protobuf.Any;
44
import com.google.protobuf.ByteString;
55
import javax.annotation.Resource;
6+
import org.junit.AfterClass;
67
import org.junit.Assert;
8+
import org.junit.BeforeClass;
79
import org.junit.Test;
810
import org.tron.common.BaseTest;
911
import org.tron.common.TestConstants;
@@ -38,11 +40,23 @@ public class MerkleContainerTest extends BaseTest {
3840
// private static MerkleContainer merkleContainer;
3941

4042

43+
private static boolean origShieldedApi;
44+
4145
static {
4246
Args.setParam(new String[]{"-d", dbPath()}, TestConstants.TEST_CONF);
47+
}
48+
49+
@BeforeClass
50+
public static void enableShieldedApi() {
51+
origShieldedApi = Args.getInstance().allowShieldedTransactionApi;
4352
Args.getInstance().allowShieldedTransactionApi = true;
4453
}
4554

55+
@AfterClass
56+
public static void restoreShieldedApi() {
57+
Args.getInstance().allowShieldedTransactionApi = origShieldedApi;
58+
}
59+
4660
/*@Before
4761
public void init() {
4862
merkleContainer = MerkleContainer

0 commit comments

Comments
 (0)