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.

Sign up for GitHub

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

feat(net): verify columns' length of HelloMessage #5667

Merged
merged 1 commit into from
Jan 15, 2024
Merged

feat(net): verify columns' length of HelloMessage #5667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 17 additions & 0 deletions framework/src/main/java/org/tron/core/net/message/handshake/HelloMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import lombok.Getter;
import org.apache.commons.lang3.StringUtils;
import org.tron.common.utils.ByteArray;
import org.tron.common.utils.DecodeUtil;
import org.tron.common.utils.StringUtil;
import org.tron.core.ChainBaseManager;
import org.tron.core.capsule.BlockCapsule;
Expand Down Expand Up @@ -169,6 +170,22 @@ public boolean valid() {
return false;
}

int maxByteSize = 200;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a maximum byte size of 200 too large? Can it be set smaller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The target of method valid is to make sure that log's content is not too large, so the value of maxByteSize is not very important. The length of address may be 42 bytes or more (not certain), sig is 65, codeVersion may not stable(20 bytes?). Can you give a suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just take the maximum of the three values, for example 65.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, I don't test the true length of address ang sig, because i have no fastforword node to use. Only give a theoretical value. Give an upper value can be ok, but give an exact value is not necessary, because method checkHelloMessage will verify the address and sig again. The main purpose is to not record too many message in log file.

ByteString address = this.helloMessage.getAddress();
if (!address.isEmpty() && address.toByteArray().length > maxByteSize) {
return false;
}

ByteString sig = this.helloMessage.getSignature();
if (!sig.isEmpty() && sig.toByteArray().length > maxByteSize) {
return false;
}

ByteString codeVersion = this.helloMessage.getCodeVersion();
if (!codeVersion.isEmpty() && codeVersion.toByteArray().length > maxByteSize) {
return false;
}

Copy link
Contributor

@xxo1shine xxo1shine Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The length judgment of address and version can be more specific.
It is recommended to consider multi-signature within 3 keys, If there are 3 multi-signatures, will the length exceed 200?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this signature only come from SR's sig of timestamp in RelayService. it's 65 stablely, not related to multi-signature. You can read the code:

      String sig =
          TransactionCapsule.getBase64FromByteString(msg.getSignature());
      byte[] sigAddress = SignUtils.signatureToAddress(hash.getBytes(), sig,
          Args.getInstance().isECKeyCryptoEngine());

return true;
}

Expand Down
15 changes: 9 additions & 6 deletions framework/src/main/java/org/tron/core/net/service/handshake/HandshakeService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,15 @@ public void processHelloMessage(PeerConnection peer, HelloMessage msg) {
}

if (!msg.valid()) {
logger.warn("Peer {} invalid hello message parameters, "
+ "GenesisBlockId: {}, SolidBlockId: {}, HeadBlockId: {}",
peer.getInetSocketAddress(),
ByteArray.toHexString(msg.getInstance().getGenesisBlockId().getHash().toByteArray()),
ByteArray.toHexString(msg.getInstance().getSolidBlockId().getHash().toByteArray()),
ByteArray.toHexString(msg.getInstance().getHeadBlockId().getHash().toByteArray()));
logger.warn("Peer {} invalid hello message parameters, GenesisBlockId: {}, SolidBlockId: {}, "
+ "HeadBlockId: {}, address: {}, sig: {}, codeVersion: {}",
peer.getInetSocketAddress(),
ByteArray.toHexString(msg.getInstance().getGenesisBlockId().getHash().toByteArray()),
ByteArray.toHexString(msg.getInstance().getSolidBlockId().getHash().toByteArray()),
ByteArray.toHexString(msg.getInstance().getHeadBlockId().getHash().toByteArray()),
msg.getInstance().getAddress().toByteArray().length,
msg.getInstance().getSignature().toByteArray().length,
msg.getInstance().getCodeVersion().toByteArray().length);
peer.disconnect(ReasonCode.UNEXPECTED_IDENTITY);
return;
}
Expand Down
41 changes: 41 additions & 0 deletions framework/src/test/java/org/tron/core/net/services/HandShakeServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.mockito.Mockito;
import org.springframework.context.ApplicationContext;
import org.tron.common.application.TronApplicationContext;
import org.tron.common.utils.ByteArray;
import org.tron.common.utils.ReflectUtils;
import org.tron.common.utils.Sha256Hash;
import org.tron.core.ChainBaseManager;
Expand Down Expand Up @@ -137,6 +138,37 @@ public void testInvalidHelloMessage() {
}
}

@Test
public void testInvalidHelloMessage2() throws Exception {
Protocol.HelloMessage.Builder builder = getTestHelloMessageBuilder();
Assert.assertTrue(new HelloMessage(builder.build().toByteArray()).valid());

builder.setAddress(ByteString.copyFrom(new byte[201]));
HelloMessage helloMessage = new HelloMessage(builder.build().toByteArray());
Assert.assertFalse(helloMessage.valid());

builder.setAddress(ByteString.copyFrom(new byte[200]));
helloMessage = new HelloMessage(builder.build().toByteArray());
Assert.assertTrue(helloMessage.valid());

builder.setSignature(ByteString.copyFrom(new byte[201]));
helloMessage = new HelloMessage(builder.build().toByteArray());
Assert.assertFalse(helloMessage.valid());

builder.setSignature(ByteString.copyFrom(new byte[200]));
helloMessage = new HelloMessage(builder.build().toByteArray());
Assert.assertTrue(helloMessage.valid());

builder.setCodeVersion(ByteString.copyFrom(new byte[201]));
helloMessage = new HelloMessage(builder.build().toByteArray());
Assert.assertFalse(helloMessage.valid());

builder.setCodeVersion(ByteString.copyFrom(new byte[200]));
helloMessage = new HelloMessage(builder.build().toByteArray());
Assert.assertTrue(helloMessage.valid());
}


@Test
public void testRelayHelloMessage() throws NoSuchMethodException {
InetSocketAddress a1 = new InetSocketAddress("127.0.0.1", 10001);
Expand Down Expand Up @@ -266,4 +298,13 @@ private Protocol.HelloMessage.Builder getHelloMessageBuilder(Node from, long tim

return builder;
}

private Protocol.HelloMessage.Builder getTestHelloMessageBuilder() {
InetSocketAddress a1 = new InetSocketAddress("127.0.0.1", 10001);
Node node = new Node(NetUtil.getNodeId(), a1.getAddress().getHostAddress(), null, a1.getPort());
Protocol.HelloMessage.Builder builder =
getHelloMessageBuilder(node, System.currentTimeMillis(),
ChainBaseManager.getChainBaseManager());
return builder;
}
}