Conversation
Co-authored-by: Alok Nerurkar <alok@no-reply.com>
| func deserialize(data []byte) []*ecdsa.PublicKey { | ||
| if len(data) == 0 { | ||
| if len(data) == 0 || len(data)%publicKeyLen != 0 { | ||
| return []*ecdsa.PublicKey{} |
There was a problem hiding this comment.
deserialize returns an empty slice [] if the stored data has an invalid length or corrupted keys.
NewGranteeListReference returns &GranteeListStruct{grantees: []}, nil. Corrupted data is silently masked as an empty ACL. Should we split the IFs and return error if len(data)%publicKeyLen != 0
| $(GOLANGCI_LINT) run ./... | ||
|
|
||
| .PHONY: nilaway | ||
| nilaway: |
There was a problem hiding this comment.
Missing nilaway Target:
+.PHONY: nilaway-bin
+nilaway-bin:
- test -f
$(GOBIN)/nilaway || $ (GO) install go.uber.org/nilaway/cmd/nilaway@latest
.PHONY: nilaway
-nilaway:
+nilaway: nilaway-bin
$(GOBIN)/nilaway ./...
| @@ -147,8 +148,14 @@ func New(ctx context.Context, g storage.Getter, putter storage.Putter, address s | |||
| // A Joiner provides Read, Seek and Size functionalities. | |||
| func NewJoiner(ctx context.Context, g storage.Getter, putter storage.Putter, address swarm.Address, rootChunk swarm.Chunk) (file.Joiner, int64, error) { | |||
| chunkData := rootChunk.Data() | |||
There was a problem hiding this comment.
should we also check rootChunk if nil ?
| pool := make([]byte, swarm.ChunkWithSpanSize*2) | ||
| f.Add(append(inter, pool...)) | ||
|
|
||
| f.Fuzz(func(t *testing.T, data []byte) { |
There was a problem hiding this comment.
test encrypted references (64 bytes) alongside standard 32-byte references:
- f.Fuzz(func(t *testing.T, data []byte) {
+ f.Fuzz(func(t *testing.T, data []byte, encryptedRef bool) {
....
| // the fields are copied into fixed size windows; an oversized field would | ||
| // either silently overwrite a neighbouring field or, for the value, index | ||
| // out of range. | ||
| if len(value) > 32 || len(b.ID) > 32 || len(b.Owner) > 20 { |
There was a problem hiding this comment.
Checking len(b.ID) > 32 and len(b.Owner) > 20 allows short byte slices. A 10-byte ID or 12-byte Owner is copied into the binary output left-aligned. Deserialization slices 32 and 20 bytes respectively, which mutates the data.
Should we have: if len(value) > 32 || len(b.ID) != 32 || len(b.Owner) != 20 { ?
| func (b *Batch) MarshalBinary() ([]byte, error) { | ||
| out := make([]byte, 95) | ||
| copy(out, b.ID) | ||
| if b.Value == nil { |
There was a problem hiding this comment.
If b.Value is negative, big.Int.Bytes() serializes the absolute magnitude, and deserialization recovers it as a positive number
| if len(value) > 32 || len(b.ID) > 32 || len(b.Owner) > 20 { | ||
| return nil, ErrBatchInvalid | ||
| } | ||
| out := make([]byte, batchSize) |
There was a problem hiding this comment.
Should we also have this checks?
if b.BucketDepth > b.Depth || b.Depth > swarm.MaxPO {
return nil, ErrBatchInvalid
}
| b.Start = binary.BigEndian.Uint64(buf[64:72]) | ||
| b.Owner = buf[72:92] | ||
| b.BucketDepth = buf[92] | ||
| b.Depth = buf[93] |
There was a problem hiding this comment.
Same check here?
if b.BucketDepth > b.Depth || b.Depth > swarm.MaxPO {
return ErrBatchInvalid
}
| // maxScryptMem bounds the memory scrypt.Key is allowed to allocate for a | ||
| // keyfile supplied set of parameters (it allocates 128*N*r bytes), so that | ||
| // a malformed or hostile keyfile cannot exhaust the node's memory. | ||
| maxScryptMem = 1 << 30 |
There was a problem hiding this comment.
AI:
maxScryptP = 256 and maxScryptMem = 1 GiB. This permits hostile keyfiles to demand 256 sequential passes and up to 1 GiB of RAM, causing severe CPU exhaustion or OOM-killing low-memory nodes.
- maxScryptMem = 1 << 30
- maxScryptMem = 256 * (1 << 20) // 256 MiB: matches standard Ethereum N=262144, r=8
// maxScryptP bounds the parallelization factor, which drives both the
// number of sequential smix passes and the size of the pbkdf2 block.
- maxScryptP = 1 << 8
- maxScryptP = 1 // Ethereum keystores universally use p=1
2 participants
Description
Adds fuzz testing across the codebase and fixes a few paths that needed patching.
AI Disclosure