RIN-02 Panic Due to Integer Overflow in Signature Size Calculation #401
Conversation
core/vm/privacy/ringct.go
Outdated
|
|
||
| // Calculate each term separately and check for overflow | ||
| term1 := 8 + 8 + 32 + 32 | ||
| term2 := numRing * ringSize * 32 |
There was a problem hiding this comment.
could numRing * ringSize overflow? could numRing * ringSize * 32 overflow?
There was a problem hiding this comment.
yes it could, their types are both int
There was a problem hiding this comment.
The value is actually reading from signature
func Deserialize(r []byte) (*RingSignature, error) {
if len(r) < 16 {
return nil, errors.New("Failed to deserialize ring signature")
}
offset := 0
sig := new(RingSignature)
numRing := r[offset : offset+8]
offset += 8
size := r[offset : offset+8]
offset += 8
size_uint := binary.BigEndian.Uint64(size)
size_int := int(size_uint)
sig.Size = size_int
There was a problem hiding this comment.
I guess if someone manipulate signature that intend to make it overflow, it's doable.
There was a problem hiding this comment.
maybe we should check multiplication overflow too
There was a problem hiding this comment.
In fact, the function computeSignatureSize should return uint, not int. The below function signature maybe better:
func computeSignatureSize(numRing uint64, ringSize uint64) uint64When the compute result is overflow, we can return 0.
There was a problem hiding this comment.
I think -1 is better since the final check is compared length, this quarantine is definitely return error
if len(r) != computeSignatureSize(sig.NumRing, sig.Size) {
return nil, fmt.Errorf("incorrect ring size, len r: %d, sig.NumRing: %d sig.Size: %d", len(r), sig.NumRing, sig.Size)
}
There was a problem hiding this comment.
maybe we should check multiplication overflow too
@wgr523 I didn't get you
gzliudan
left a comment
gzliudan
left a comment
There was a problem hiding this comment.
Please note in the function func (r *RingSignature) Serialize() ([]byte, error):
8+8+32+32+(32+33)*r.NumRing*r.Size+33*r.NumRing can be replaced with computeSignatureSize(r.NumRing, r.Size)
| if numRing < 0 || ringSize < 0 { | ||
| return -1 | ||
| } | ||
|
|
There was a problem hiding this comment.
Both of numRing and ringSize must greater than zero:
if numRing <= 0 || ringSize <= 0 {
return -1
}There was a problem hiding this comment.
It can be zero right? based on the logic
numRing := r[offset : offset+8]
offset += 8
size := r[offset : offset+8]
offset += 8
size_uint := binary.BigEndian.Uint64(size)
size_int := int(size_uint)
sig.Size = size_int
size_uint = binary.BigEndian.Uint64(numRing)
size_int = int(size_uint)
sig.NumRing = size_int
core/vm/privacy/ringct.go
Outdated
|
|
||
| // Calculate each term separately and check for overflow | ||
| term1 := 8 + 8 + 32 + 32 | ||
| term2 := numRing * ringSize * 32 |
There was a problem hiding this comment.
In fact, the function computeSignatureSize should return uint, not int. The below function signature maybe better:
func computeSignatureSize(numRing uint64, ringSize uint64) uint64When the compute result is overflow, we can return 0.
Proposed changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
Malicious users can create signatures with invalid sizes but with forged numRing and ringSize to bypass the signature size check.
As a result, a panic from a buffer overflow may happen when the implementation tries to deserialize sig.S from the input with a big offset value.
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that