agl / pond

Pond
BSD 3-Clause "New" or "Revised" License
911 stars 109 forks source link

Potential memory overflow in Transport.go #168

Closed unixninja92 closed 9 years ago

unixninja92 commented 9 years ago

I found the following line in the Write function of Transport.go:

144     l := len(secretbox.Seal(c.writeBuffer[2:2], buf[:m], &c.writeSequence, &c.writeKey))

From my understanding of the code and go, setting c.writeBuffer[2:2] as the out for secretbox.Seal means that each time Write() is called, the writeBuffer increases in size by m. I believe it should either be c.writeBuffer[2:m+2] or c.writeBuffer[2:].

Either that or the encrypted bytes are never actually stored in writeBuffer, but I assume unit tests would have caught that by now.

If I'm correct in this finding, I'd be happy to make a pull request. (also, not sure if memory overflow is the right term here)

agl commented 9 years ago

I think it would be called a "space leak" in this case, but I don't believe that it's happening.

secretbox.Seal works in the same way as the built-in function append—it takes a slice and will append to it if possible, create a new slice if not, and returns whichever contains the result.

In this case, c.writeBuffer[2:2] is an empty slice (its len is 0), but it always has enough capacity to hold the result. This secretbox.Seal will always append to it and return the resliced alias to c.writeBuffer. Thus taking the len() of the result gives the number of ciphertext bytes.