removeLen32Prefixed: check for possible data overflow#629
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if itemLen < 0 { | ||
| return nil, nil, fmt.Errorf("negative data length: possible overflow") | ||
| } | ||
| if (4 + itemLen) > len(data) { | ||
| return nil, nil, fmt.Errorf("data too short") | ||
| } |
There was a problem hiding this comment.
🔴 Overflow check is incomplete — addition of offset and length can still wrap on 32-bit targets
The new negative-length check (itemLen < 0 at abe/cpabe/tkn20/internal/tkn/util.go:80) catches uint32 values ≥ 2³¹ on 32-bit platforms, but the subsequent addition (4 + itemLen at abe/cpabe/tkn20/internal/tkn/util.go:83) can still wrap negative for large positive values, so the bounds check passes and the slice operation panics on attacker-controlled ciphertext.
Impact: On 32-bit targets (including WASM, which CI tests), a crafted ciphertext with a length field near 2³¹ can crash the decryption routine.
Detailed overflow mechanism on 32-bit int
On 32-bit platforms (GOOS=js GOARCH=wasm), int is 32 bits. If the encoded uint32 length is e.g. 0x7FFFFFFC (2147483644), it passes the itemLen < 0 check since it's positive as int32. Then 4 + 2147483644 overflows int32 to -2147483648, which is less than len(data), so the (4 + itemLen) > len(data) check passes. The subsequent slice data[4 : 4+itemLen] uses the overflowed negative value as the high bound, causing a runtime panic.
This function is called from DecryptCCA (abe/cpabe/tkn20/internal/tkn/bk.go:156) on attacker-controlled ciphertext input. The REVIEW.md rule states: "No panic on attacker-controlled input."
A robust fix would avoid int arithmetic overflow entirely, e.g.:
itemLen := int(binary.LittleEndian.Uint32(data))
if itemLen < 0 || itemLen > len(data)-4 {
return nil, nil, fmt.Errorf("data too short")
}Since len(data) >= 4 is already guaranteed by the earlier check, len(data)-4 is non-negative, and comparing itemLen against it avoids any addition overflow.
| if itemLen < 0 { | |
| return nil, nil, fmt.Errorf("negative data length: possible overflow") | |
| } | |
| if (4 + itemLen) > len(data) { | |
| return nil, nil, fmt.Errorf("data too short") | |
| } | |
| if itemLen < 0 || itemLen > len(data)-4 { | |
| return nil, nil, fmt.Errorf("data too short") | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if itemLen < 0 { | ||
| return nil, nil, fmt.Errorf("negative data length: possible overflow") | ||
| } | ||
| if (4 + itemLen) > len(data) { | ||
| return nil, nil, fmt.Errorf("data too short") | ||
| } |
There was a problem hiding this comment.
🟨 Incomplete overflow guard still allows denial-of-service via crafted ciphertext on 32-bit targets
The new itemLen < 0 check at abe/cpabe/tkn20/internal/tkn/util.go:80 catches uint32 values ≥ 2³¹ on 32-bit platforms, but the expression 4 + itemLen at line 83 can still overflow for large positive int32 values (e.g., 0x7FFFFFFC). This causes the bounds check to pass, and the subsequent slice operation panics. Since removeLen32Prefixed is called from DecryptCCA (abe/cpabe/tkn20/internal/tkn/bk.go:156) on attacker-controlled ciphertext, a crafted 4-byte length header can crash the process on WASM and other 32-bit targets.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Dear Christopher,
On Mon Jul 6, 2026 at 21:55 CEST, Christopher Patton wrote:
@mdosch this looks useful?
sorry, but I don't understand what you are asking me.
I find this useful as the tests fail on 32 bit archs (see links in the
initial report), afaiu due to an overflow.
This PR will catch this, properly return an error and the tests pass
again.
Best regards,
Martin
|
2 participants
fix #628