Skip to content

Commit 1691fdd

Browse files
Federico2014Federico2014
authored
fix(api): bound and truncate api signatures (tronprotocol#6820)
1 parent 8bffa19 commit 1691fdd

7 files changed

Lines changed: 352 additions & 24 deletions

File tree

‎actuator/src/main/java/org/tron/core/utils/TransactionUtil.java‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import static org.tron.common.crypto.Hash.sha3omit12;
1919
import static org.tron.common.math.Maths.max;
2020
import static org.tron.core.config.Parameter.ChainConstant.DELEGATE_COST_BASE_SIZE;
21+
import static org.tron.core.Constant.PER_SIGN_LENGTH;
2122
import static org.tron.core.config.Parameter.ChainConstant.TRX_PRECISION;
2223

2324
import com.google.common.base.CaseFormat;
@@ -183,8 +184,30 @@ public static String makeUpperCamelMethod(String originName) {
183184
.replace("_", "");
184185
}
185186

187+
public static Transaction truncateSignatures(Transaction trx) {
188+
Transaction.Builder builder = trx.toBuilder().clearSignature();
189+
for (ByteString sig : trx.getSignatureList()) {
190+
if (sig.size() > PER_SIGN_LENGTH) {
191+
builder.addSignature(ByteString.copyFrom(sig.substring(0, PER_SIGN_LENGTH).toByteArray()));
192+
} else {
193+
builder.addSignature(sig);
194+
}
195+
}
196+
return builder.build();
197+
}
198+
186199
public TransactionSignWeight getTransactionSignWeight(Transaction trx) {
187200
TransactionSignWeight.Builder tswBuilder = TransactionSignWeight.newBuilder();
201+
Result.Builder resultBuilder = Result.newBuilder();
202+
if (trx.getSignatureCount() > chainBaseManager.getDynamicPropertiesStore()
203+
.getTotalSignNum()) {
204+
resultBuilder.setCode(Result.response_code.OTHER_ERROR);
205+
resultBuilder.setMessage("too many signatures");
206+
tswBuilder.setResult(resultBuilder);
207+
return tswBuilder.build();
208+
}
209+
210+
trx = truncateSignatures(trx);
188211
TransactionExtention.Builder trxExBuilder = TransactionExtention.newBuilder();
189212
trxExBuilder.setTransaction(trx);
190213
trxExBuilder.setTxid(ByteString.copyFrom(Sha256Hash.hash(CommonParameter
@@ -193,7 +216,6 @@ public TransactionSignWeight getTransactionSignWeight(Transaction trx) {
193216
retBuilder.setResult(true).setCode(response_code.SUCCESS);
194217
trxExBuilder.setResult(retBuilder);
195218
tswBuilder.setTransaction(trxExBuilder);
196-
Result.Builder resultBuilder = Result.newBuilder();
197219

198220
if (trx.getRawData().getContractCount() == 0) {
199221
resultBuilder.setCode(Result.response_code.OTHER_ERROR);

‎chainbase/src/main/java/org/tron/core/capsule/TransactionCapsule.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ public static long checkWeight(Permission permission, List<ByteString> sigs, byt
251251
long weight = getWeight(permission, address);
252252
if (weight == 0) {
253253
throw new PermissionException(
254-
ByteArray.toHexString(sig.toByteArray()) + " is signed by " + encode58Check(address)
254+
ByteArray.toHexString(hash) + " is signed by " + encode58Check(address)
255255
+ " but it is not contained of permission.");
256256
}
257257
if (ForkController.instance().pass(Parameter.ForkBlockVersionEnum.VERSION_4_7_1)) {
@@ -631,7 +631,7 @@ public void addSign(byte[] privateKey, AccountStore accountStore)
631631
.signHash(getTransactionId().getBytes())));
632632
this.transaction = this.transaction.toBuilder().addSignature(sig).build();
633633
}
634-
634+
635635
private static void checkPermission(int permissionId, Permission permission, Transaction.Contract contract) throws PermissionException {
636636
if (permissionId != 0) {
637637
if (permission.getType() != PermissionType.Active) {
@@ -714,7 +714,7 @@ public boolean validateSignature(AccountStore accountStore,
714714
}
715715
}
716716
isVerified = true;
717-
}
717+
}
718718
return true;
719719
}
720720

‎framework/src/main/java/org/tron/core/Wallet.java‎

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,8 @@
228228
import org.tron.protos.Protocol.MarketOrderPairList;
229229
import org.tron.protos.Protocol.MarketPrice;
230230
import org.tron.protos.Protocol.MarketPriceList;
231+
import org.tron.protos.Protocol.Permission;
232+
import org.tron.protos.Protocol.Permission.PermissionType;
231233
import org.tron.protos.Protocol.Proposal;
232234
import org.tron.protos.Protocol.Transaction;
233235
import org.tron.protos.Protocol.Transaction.Contract;
@@ -628,6 +630,17 @@ public GrpcAPI.Return broadcastTransaction(Transaction signedTransaction) {
628630

629631
public TransactionApprovedList getTransactionApprovedList(Transaction trx) {
630632
TransactionApprovedList.Builder tswBuilder = TransactionApprovedList.newBuilder();
633+
TransactionApprovedList.Result.Builder resultBuilder = TransactionApprovedList.Result
634+
.newBuilder();
635+
if (trx.getSignatureCount() > chainBaseManager.getDynamicPropertiesStore()
636+
.getTotalSignNum()) {
637+
resultBuilder.setCode(TransactionApprovedList.Result.response_code.OTHER_ERROR);
638+
resultBuilder.setMessage("too many signatures");
639+
tswBuilder.setResult(resultBuilder);
640+
return tswBuilder.build();
641+
}
642+
643+
trx = TransactionUtil.truncateSignatures(trx);
631644
TransactionExtention.Builder trxExBuilder = TransactionExtention.newBuilder();
632645
trxExBuilder.setTransaction(trx);
633646
trxExBuilder.setTxid(ByteString.copyFrom(Sha256Hash.hash(CommonParameter
@@ -636,8 +649,6 @@ public TransactionApprovedList getTransactionApprovedList(Transaction trx) {
636649
retBuilder.setResult(true).setCode(response_code.SUCCESS);
637650
trxExBuilder.setResult(retBuilder);
638651
tswBuilder.setTransaction(trxExBuilder);
639-
TransactionApprovedList.Result.Builder resultBuilder = TransactionApprovedList.Result
640-
.newBuilder();
641652

642653
if (trx.getRawData().getContractCount() == 0) {
643654
resultBuilder.setCode(TransactionApprovedList.Result.response_code.OTHER_ERROR);
@@ -650,21 +661,26 @@ public TransactionApprovedList getTransactionApprovedList(Transaction trx) {
650661
if (account == null) {
651662
throw new PermissionException("Account does not exist!");
652663
}
664+
int permissionId = contract.getPermissionId();
665+
Permission permission = account.getPermissionById(permissionId);
666+
if (permission == null) {
667+
throw new PermissionException("Permission for this, does not exist!");
668+
}
669+
if (permissionId != 0) {
670+
if (permission.getType() != PermissionType.Active) {
671+
throw new PermissionException("Permission type is wrong!");
672+
}
673+
//check operations
674+
if (!WalletUtil.checkPermissionOperations(permission, contract)) {
675+
throw new PermissionException("Permission denied!");
676+
}
677+
}
653678

654679
if (trx.getSignatureCount() > 0) {
655-
List<ByteString> approveList = new ArrayList<ByteString>();
680+
List<ByteString> approveList = new ArrayList<>();
656681
byte[] hash = Sha256Hash.hash(CommonParameter
657682
.getInstance().isECKeyCryptoEngine(), trx.getRawData().toByteArray());
658-
for (ByteString sig : trx.getSignatureList()) {
659-
if (sig.size() < 65) {
660-
throw new SignatureFormatException(
661-
"Signature size is " + sig.size());
662-
}
663-
String base64 = TransactionCapsule.getBase64FromByteString(sig);
664-
byte[] address = SignUtils.signatureToAddress(hash, base64, Args.getInstance()
665-
.isECKeyCryptoEngine());
666-
approveList.add(ByteString.copyFrom(address)); //out put approve list.
667-
}
683+
TransactionCapsule.checkWeight(permission, trx.getSignatureList(), hash, approveList);
668684
tswBuilder.addAllApprovedList(approveList);
669685
}
670686
resultBuilder.setCode(TransactionApprovedList.Result.response_code.SUCCESS);

‎framework/src/main/java/org/tron/core/services/http/Util.java‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -210,9 +210,12 @@ public static String printTransactionSignWeight(TransactionSignWeight transactio
210210
String string = JsonFormat.printToString(transactionSignWeight, selfType);
211211
JSONObject jsonObject = JSONObject.parseObject(string);
212212
JSONObject jsonObjectExt = jsonObject.getJSONObject(TRANSACTION);
213-
jsonObjectExt.put(TRANSACTION,
214-
printTransactionToJSON(transactionSignWeight.getTransaction().getTransaction(), selfType));
215-
jsonObject.put(TRANSACTION, jsonObjectExt);
213+
if (jsonObjectExt != null) {
214+
jsonObjectExt.put(TRANSACTION,
215+
printTransactionToJSON(transactionSignWeight.getTransaction().getTransaction(),
216+
selfType));
217+
jsonObject.put(TRANSACTION, jsonObjectExt);
218+
}
216219
return jsonObject.toJSONString();
217220
}
218221

@@ -221,10 +224,12 @@ public static String printTransactionApprovedList(TransactionApprovedList transa
221224
String string = JsonFormat.printToString(transactionApprovedList, selfType);
222225
JSONObject jsonObject = JSONObject.parseObject(string);
223226
JSONObject jsonObjectExt = jsonObject.getJSONObject(TRANSACTION);
224-
jsonObjectExt.put(TRANSACTION,
225-
printTransactionToJSON(transactionApprovedList.getTransaction().getTransaction(),
226-
selfType));
227-
jsonObject.put(TRANSACTION, jsonObjectExt);
227+
if (jsonObjectExt != null) {
228+
jsonObjectExt.put(TRANSACTION,
229+
printTransactionToJSON(transactionApprovedList.getTransaction().getTransaction(),
230+
selfType));
231+
jsonObject.put(TRANSACTION, jsonObjectExt);
232+
}
228233
return jsonObject.toJSONString();
229234
}
230235

‎framework/src/test/java/org/tron/core/WalletTest.java‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1440,5 +1440,133 @@ public void testGetSolidBlock() {
14401440
Block block = wallet.getSolidBlock();
14411441
assertEquals(block2, block);
14421442
}
1443+
1444+
@Test
1445+
public void testApprovedListSigBound() {
1446+
ECKey ecKey = new ECKey(Utils.getRandom());
1447+
AccountCapsule owner = new AccountCapsule(
1448+
ByteString.copyFromUtf8("approved-owner"),
1449+
ByteString.copyFrom(ecKey.getAddress()),
1450+
Protocol.AccountType.Normal,
1451+
initBalance);
1452+
chainBaseManager.getAccountStore().put(ecKey.getAddress(), owner);
1453+
// Default owner permission: a single key with weight 1, so keysCount == 1.
1454+
int keysCount = owner.getPermissionById(0).getKeysCount();
1455+
assertEquals(1, keysCount);
1456+
1457+
Transaction unsigned = Transaction.newBuilder().setRawData(
1458+
Transaction.raw.newBuilder().addContract(
1459+
Contract.newBuilder().setType(ContractType.TransferContract)
1460+
.setParameter(Any.pack(TransferContract.newBuilder().setAmount(1)
1461+
.setOwnerAddress(ByteString.copyFrom(ecKey.getAddress()))
1462+
.setToAddress(ByteString.copyFrom(
1463+
ByteArray.fromHexString(RECEIVER_ADDRESS)))
1464+
.build())).build()).build()).build();
1465+
1466+
// One valid 65-byte [r][s][recId] signature by the owner.
1467+
TransactionCapsule capsule = new TransactionCapsule(unsigned);
1468+
capsule.sign(ecKey.getPrivKeyBytes());
1469+
ByteString oneSig = capsule.getInstance().getSignature(0);
1470+
1471+
// Within keysCount: the single valid signature is recovered, result is SUCCESS.
1472+
GrpcAPI.TransactionApprovedList okList = wallet.getTransactionApprovedList(
1473+
unsigned.toBuilder().addSignature(oneSig).build());
1474+
assertEquals(GrpcAPI.TransactionApprovedList.Result.response_code.SUCCESS,
1475+
okList.getResult().getCode());
1476+
assertEquals(1, okList.getApprovedListCount());
1477+
1478+
// More signatures than keysCount: checkWeight rejects before recovering any of them,
1479+
// so the unbounded ecrecover loop can no longer be triggered.
1480+
Transaction.Builder overLimit = unsigned.toBuilder();
1481+
for (int i = 0; i < keysCount + 1; i++) {
1482+
overLimit.addSignature(oneSig);
1483+
}
1484+
GrpcAPI.TransactionApprovedList rejected =
1485+
wallet.getTransactionApprovedList(overLimit.build());
1486+
assertEquals(GrpcAPI.TransactionApprovedList.Result.response_code.OTHER_ERROR,
1487+
rejected.getResult().getCode());
1488+
assertEquals(0, rejected.getApprovedListCount());
1489+
Assert.assertFalse(rejected.getResult().getMessage().isEmpty());
1490+
}
1491+
1492+
@Test
1493+
public void testApprovedListSigTruncate() {
1494+
ECKey ecKey = new ECKey(Utils.getRandom());
1495+
AccountCapsule owner = new AccountCapsule(
1496+
ByteString.copyFromUtf8("approved-owner-trunc"),
1497+
ByteString.copyFrom(ecKey.getAddress()),
1498+
Protocol.AccountType.Normal,
1499+
initBalance);
1500+
chainBaseManager.getAccountStore().put(ecKey.getAddress(), owner);
1501+
1502+
Transaction unsigned = Transaction.newBuilder().setRawData(
1503+
Transaction.raw.newBuilder().addContract(
1504+
Contract.newBuilder().setType(ContractType.TransferContract)
1505+
.setParameter(Any.pack(TransferContract.newBuilder().setAmount(1)
1506+
.setOwnerAddress(ByteString.copyFrom(ecKey.getAddress()))
1507+
.setToAddress(ByteString.copyFrom(
1508+
ByteArray.fromHexString(RECEIVER_ADDRESS)))
1509+
.build())).build()).build()).build();
1510+
1511+
TransactionCapsule capsule = new TransactionCapsule(unsigned);
1512+
capsule.sign(ecKey.getPrivKeyBytes());
1513+
ByteString validSig = capsule.getInstance().getSignature(0);
1514+
assertEquals(65, validSig.size());
1515+
1516+
// Pad the 65-byte signature with trailing junk bytes.
1517+
ByteString oversized = validSig.concat(
1518+
ByteString.copyFrom(new byte[] {1, 2, 3, 4, 5}));
1519+
assertEquals(70, oversized.size());
1520+
1521+
GrpcAPI.TransactionApprovedList reply = wallet.getTransactionApprovedList(
1522+
unsigned.toBuilder().addSignature(oversized).build());
1523+
1524+
// Recovery still succeeds and resolves the owner.
1525+
assertEquals(GrpcAPI.TransactionApprovedList.Result.response_code.SUCCESS,
1526+
reply.getResult().getCode());
1527+
assertEquals(1, reply.getApprovedListCount());
1528+
// The echoed-back transaction has the signature truncated to 65 bytes.
1529+
Transaction echoed = reply.getTransaction().getTransaction();
1530+
assertEquals(1, echoed.getSignatureCount());
1531+
assertEquals(65, echoed.getSignature(0).size());
1532+
assertEquals(validSig, echoed.getSignature(0));
1533+
}
1534+
1535+
@Test
1536+
public void testApprovedListTooManySigs() {
1537+
ECKey ecKey = new ECKey(Utils.getRandom());
1538+
AccountCapsule owner = new AccountCapsule(
1539+
ByteString.copyFromUtf8("total-sign-num-owner"),
1540+
ByteString.copyFrom(ecKey.getAddress()),
1541+
Protocol.AccountType.Normal,
1542+
initBalance);
1543+
chainBaseManager.getAccountStore().put(ecKey.getAddress(), owner);
1544+
1545+
Transaction unsigned = Transaction.newBuilder().setRawData(
1546+
Transaction.raw.newBuilder().addContract(
1547+
Contract.newBuilder().setType(ContractType.TransferContract)
1548+
.setParameter(Any.pack(TransferContract.newBuilder().setAmount(1)
1549+
.setOwnerAddress(ByteString.copyFrom(ecKey.getAddress()))
1550+
.setToAddress(ByteString.copyFrom(
1551+
ByteArray.fromHexString(RECEIVER_ADDRESS)))
1552+
.build())).build()).build()).build();
1553+
1554+
TransactionCapsule capsule = new TransactionCapsule(unsigned);
1555+
capsule.sign(ecKey.getPrivKeyBytes());
1556+
ByteString oneSig = capsule.getInstance().getSignature(0);
1557+
1558+
int totalSignNum = chainBaseManager.getDynamicPropertiesStore().getTotalSignNum();
1559+
Transaction.Builder overLimit = unsigned.toBuilder();
1560+
for (int i = 0; i < totalSignNum + 1; i++) {
1561+
overLimit.addSignature(oneSig);
1562+
}
1563+
1564+
GrpcAPI.TransactionApprovedList rejected =
1565+
wallet.getTransactionApprovedList(overLimit.build());
1566+
assertEquals(GrpcAPI.TransactionApprovedList.Result.response_code.OTHER_ERROR,
1567+
rejected.getResult().getCode());
1568+
Assert.assertTrue(rejected.getResult().getMessage().contains("too many signatures"));
1569+
assertEquals(0, rejected.getApprovedListCount());
1570+
}
14431571
}
14441572

0 commit comments

Comments
 (0)