-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(net): verify columns' length of HelloMessage #5667
feat(net): verify columns' length of HelloMessage #5667
Conversation
@@ -169,6 +170,22 @@ public boolean valid() { | |||
return false; | |||
} | |||
|
|||
int maxByteSize = 200; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
if (!codeVersion.isEmpty() && codeVersion.toByteArray().length > maxByteSize) { | ||
return false; | ||
} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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());
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details