fix(crypto): address review findings

Author: Federico2014

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

@@ -17,7 +17,6 @@
 import java.util.Objects;
 import javax.annotation.Nullable;
 import lombok.extern.slf4j.Slf4j;
-import org.bouncycastle.asn1.ASN1InputStream;
 import org.bouncycastle.asn1.ASN1Integer;
 import org.bouncycastle.asn1.ASN1Primitive;
 import org.bouncycastle.asn1.ASN1Sequence;
@@ -161,11 +160,10 @@ public SM2(@Nullable PrivateKey privKey, ECPoint pub) {
               + privKey.getAlgorithm());
     }
 
-    if (pub == null) {
-      throw new IllegalArgumentException("Public key should not be null");
-    } else {
-      this.pub = pub;
+    if (pub == null || pub.isInfinity() || !pub.isValid()) {
+      throw new IllegalArgumentException("Public key is not a valid point on SM2 curve.");
     }
+    this.pub = pub;
   }
 
   /**
@@ -988,8 +986,8 @@ public static SM2Signature decodeFromDER(byte[] signature) throws SignatureExcep
         throw new SignatureException("Invalid DER signature length");
       }
 
-      try (ASN1InputStream inputStream = new ASN1InputStream(signature)) {
-        ASN1Primitive primitive = inputStream.readObject();
+      try {
+        ASN1Primitive primitive = ASN1Primitive.fromByteArray(signature);
         if (!(primitive instanceof ASN1Sequence)) {
           throw new SignatureException("Invalid DER signature format");
         }

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

@@ -186,8 +186,9 @@ public boolean verifySignature(byte[] message, BigInteger r, BigInteger s,
    * verify the hash signature
    */
   public boolean verifyHashSignature(byte[] hash, BigInteger r, BigInteger s) {
-    if (ByteArray.isEmpty(hash)) {
-      throw new IllegalArgumentException("Hash cannot be empty");
+    if (ByteArray.isEmpty(hash) || hash.length != 32) {
+      throw new IllegalArgumentException("Expected 32 byte input to "
+          + "SM2 signature, not " + (hash == null ? "null" : hash.length));
     }
     if (r == null || s == null) {
       throw new IllegalArgumentException("R or S cannot be null");

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

@@ -80,21 +80,25 @@ public static LocalWitnesses initFromKeystore(
     }
 
     List<String> privateKeys = new ArrayList<>();
+    byte[] privKeyBytes = null;
     try {
       Credentials credentials = WalletUtils.loadCredentials(pwd, new File(fileName));
       SignInterface sign = credentials.getSignInterface();
-      byte[] privKeyBytes = sign.getPrivateKey();
-      if (!SignUtils.isValidPrivateKey(privKeyBytes, Args.getInstance().isECKeyCryptoEngine())) {
-        Arrays.fill(privKeyBytes, (byte) 0);
+      privKeyBytes = sign.getPrivateKey();
+      if (privKeyBytes == null
+          || !SignUtils.isValidPrivateKey(privKeyBytes, Args.getInstance().isECKeyCryptoEngine())) {
         throw new TronError(
             "Keystore contains an invalid private key",
             TronError.ErrCode.WITNESS_KEYSTORE_LOAD);
       }
       privateKeys.add(ByteArray.toHexString(privKeyBytes));
-      Arrays.fill(privKeyBytes, (byte) 0);
     } catch (IOException | CipherException e) {
       logger.error("Witness node start failed!");
       throw new TronError(e, TronError.ErrCode.WITNESS_KEYSTORE_LOAD);
+    } finally {
+      if (privKeyBytes != null) {
+        Arrays.fill(privKeyBytes, (byte) 0);
+      }
     }
 
     LocalWitnesses witnesses = new LocalWitnesses();

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

@@ -722,8 +722,9 @@ public void isSpend(NoteParameters request, StreamObserver<SpendResult> response
     public void scanShieldedTRC20NotesByIvk(IvkDecryptTRC20Parameters request,
         StreamObserver<DecryptNotesTRC20> responseObserver) {
       if (!request.getEventsList().isEmpty()) {
-        responseObserver.onError(getRunTimeException(
-            new IllegalArgumentException("'events' field is deprecated and no longer supported")));
+        responseObserver.onError(Status.INVALID_ARGUMENT
+            .withDescription("'events' field is deprecated and no longer supported")
+            .asRuntimeException());
         return;
       }
       long startNum = request.getStartBlockIndex();
@@ -747,8 +748,9 @@ public void scanShieldedTRC20NotesByIvk(IvkDecryptTRC20Parameters request,
     public void scanShieldedTRC20NotesByOvk(OvkDecryptTRC20Parameters request,
         StreamObserver<DecryptNotesTRC20> responseObserver) {
       if (!request.getEventsList().isEmpty()) {
-        responseObserver.onError(getRunTimeException(
-            new IllegalArgumentException("'events' field is deprecated and no longer supported")));
+        responseObserver.onError(Status.INVALID_ARGUMENT
+            .withDescription("'events' field is deprecated and no longer supported")
+            .asRuntimeException());
         return;
       }
       long startNum = request.getStartBlockIndex();
@@ -2418,8 +2420,9 @@ public void scanShieldedTRC20NotesByIvk(
         IvkDecryptTRC20Parameters request,
         StreamObserver<org.tron.api.GrpcAPI.DecryptNotesTRC20> responseObserver) {
       if (!request.getEventsList().isEmpty()) {
-        responseObserver.onError(getRunTimeException(
-            new IllegalArgumentException("'events' field is deprecated and no longer supported")));
+        responseObserver.onError(Status.INVALID_ARGUMENT
+            .withDescription("'events' field is deprecated and no longer supported")
+            .asRuntimeException());
         return;
       }
       long startNum = request.getStartBlockIndex();
@@ -2448,8 +2451,9 @@ public void scanShieldedTRC20NotesByOvk(
         OvkDecryptTRC20Parameters request,
         StreamObserver<org.tron.api.GrpcAPI.DecryptNotesTRC20> responseObserver) {
       if (!request.getEventsList().isEmpty()) {
-        responseObserver.onError(getRunTimeException(
-            new IllegalArgumentException("'events' field is deprecated and no longer supported")));
+        responseObserver.onError(Status.INVALID_ARGUMENT
+            .withDescription("'events' field is deprecated and no longer supported")
+            .asRuntimeException());
         return;
       }
       long startNum = request.getStartBlockIndex();

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

@@ -3,7 +3,9 @@
 import com.google.protobuf.Any;
 import com.google.protobuf.ByteString;
 import javax.annotation.Resource;
+import org.junit.AfterClass;
 import org.junit.Assert;
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.tron.common.BaseTest;
 import org.tron.common.TestConstants;
@@ -38,11 +40,23 @@ public class MerkleContainerTest extends BaseTest {
   //  private static MerkleContainer merkleContainer;
 
 
+  private static boolean origShieldedApi;
+
   static {
     Args.setParam(new String[]{"-d", dbPath()}, TestConstants.TEST_CONF);
+  }
+
+  @BeforeClass
+  public static void enableShieldedApi() {
+    origShieldedApi = Args.getInstance().allowShieldedTransactionApi;
     Args.getInstance().allowShieldedTransactionApi = true;
   }
 
+  @AfterClass
+  public static void restoreShieldedApi() {
+    Args.getInstance().allowShieldedTransactionApi = origShieldedApi;
+  }
+
   /*@Before
   public void init() {
     merkleContainer = MerkleContainer