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.

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

BufferPool.Get doesn't detect negative length values and blindly converts them into uint32(negative) which can trigger out of memory issues #26

Closed
Tracked by #1514
odeke-em opened this issue May 30, 2022 · 0 comments · Fixed by #28
Labels
good first issue Good issue for new contributors P0 Critical: Tackled by core team ASAP

Comments

@odeke-em
Copy link

odeke-em commented May 30, 2022

If I pass in a negative value whose uint32 overflown value fits within 31 bits, the code in (*BufferPool).Get or just .Get blindly converts it to a uint32 which wraps around to a very large positive number and causes massive memory allocations that'll cause out of memory errors to be triggered and the process will freeze and hopefully your OS will kill it but essentially it can freeze.

Repro

Here is the repro test that'll consume your RAM fast and freeze your other processes

var sink interface{}

func TestBufferPoolGetWithNegativeValue(t *testing.T) {
    for i := 0; i < 50; i++ {
        b := Get(-20008899456)
        sink = b
        Put(b)
    }
    if sink == nil { 
        t.Fatal("Test didn't run!")
    }   
    sink = (interface{})(nil)
} 

Remedy

The fix for this is rather trivial; when values are less than 0, please return nil ASAP

diff --git a/pool.go b/pool.go
index d812840..a83420e 100644
--- a/pool.go
+++ b/pool.go
@@ -55,7 +55,7 @@ type bufp struct {
 //
 // If no suitable buffer exists in the pool, Get creates one.
 func (p *BufferPool) Get(length int) []byte {
-       if length == 0 {
+       if length <= 0 {
                return nil
        }
        if length > MaxLength {

Suggestion

I kindly ask that if y'all have a bug bounty/security vulnerability reporting process that you paste something in your README.md so that I can report sensitive issues there. This work is courtesy of the Cosmos Network security team program to secure all dependencies. Thank you!

odeke-em added a commit to orijtech/go-buffer-pool that referenced this issue May 30, 2022
Reject negative lengths that can wrap around to positive values that
fit within 32 bits, then cause massive RAM consumption and processes
to freeze.

This work was discovered & fixed courtesy of the Cosmos Network's
ambitious security program to secure all software dependencies & supply chains.

Fixes libp2p#26
odeke-em added a commit to orijtech/go-buffer-pool that referenced this issue May 30, 2022
Reject negative lengths that can wrap around to positive values that
fit within 32 bits, then cause massive RAM consumption and processes
to freeze.

This work was discovered & fixed courtesy of the Cosmos Network's
ambitious security program to secure all software dependencies & supply chains.

Fixes libp2p#26
@BigLep BigLep added the P2 Medium: Good to have, but can wait until someone steps up label Jun 3, 2022
odeke-em added a commit to orijtech/go-buffer-pool that referenced this issue Jun 24, 2022
Reject negative lengths that can wrap around to positive values that
fit within 32 bits, then cause massive RAM consumption and processes
to freeze. This change panics ASAP.

This work was discovered & fixed courtesy of the Cosmos Network's
ambitious security program to secure all software dependencies & supply chains.

Fixes libp2p#26
@BigLep BigLep added good first issue Good issue for new contributors P0 Critical: Tackled by core team ASAP and removed P2 Medium: Good to have, but can wait until someone steps up labels Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good issue for new contributors P0 Critical: Tackled by core team ASAP
Projects
Archived in project
2 participants