encoding/gob: document or improve handling of untrusted data

[josharian]

It is pretty easy to construct small malicious encoding/gob packets with large impacts, e.g. by using a large map hint (CL 40113).

I don't see:

• anything in the encoding/gob docs that discusses safe handling of untrusted data
• anything in the encoding/gob API that would support checking whether untrusted data is safe to decode
• anything in the implementation that ensures e.g. that there is a relationship between the amount of data and the memory used; if there were limits on amplification, then a io.LimitedReader could be used

Although maybe I missed something.

It seems like we should add some or all of the above.

cc @bradfitz @gruszczy @randall77 @robpike

[gruszczy]

1. Untrusted data: Isn't it a generally good advice, that should be included regardless of the map behavior? Or was gob encoding guaranteed to behave well against untrusted data before? Python is pretty clear in pickle:

"Warning The pickle module is not secure against erroneous or maliciously constructed data. Never unpickle data received from an untrusted or unauthenticated source."

2. Implementation:
   a) Can we check when decoding the map that the hint is capped (this was contested before in https://go-review.googlesource.com/c/40113/) just like slices are?

   // Take care with overflow in this calculation.
   if n < 0 || uint64(n) != u || nBytes > tooBig || (size > 0 && nBytes/size != u) {
   // We don't check n against buffer length here because if it's a slice
   // of interfaces, there will be buffer reloads.
   errorf("%s slice too big: %d elements of %d bytes", typ.Elem(), u, size)
   }

b) Independent of capping the hint, can we compare the size of the message to the proposed hint and cap it based on that? Something along the lines (in decodeMap):

func (dec *Decoder) decodeMap(mtyp reflect.Type, state *decoderState, value reflect.Value, keyOp, elemOp decOp, ovfl error) {
n := int(state.decodeUint())
# Ensure that the hint is not maliciously set to be very big on a small message.
msgSize := len(state.b.data)
if msgSize / (mtyp.Key().Size() + mtyp.Elem().Size()) < n {
n = msgSize / (mtyp.Key().Size() + mtyp.Elem().Size())
}

If someone sends multiple maps with large hints and not content, the first map creation should consume remaining (and potentially panic not being able to decode something).

3. Encoding/gob documentation: After 2) can we guarantee that the decoded message will have roughly the same size as encoded message, so the user of the API can protect themselves against decoding maliciously large messages?

[josharian]

Untrusted data: Isn't it a generally good advice, that should be included regardless of the map behavior?

Yes. It should have been documented long ago, like pickle. I think this probably applies to arrays and slices as well, although I haven't checked.

can we compare the size of the message to the proposed hint and cap it based on that?

Seems like a reasonable approach to me, but the devil is in the details. (Does [1000]int{999: 1} get encoded as a key/value pair? Should we reject it, where we previously accepted it? Is is just a multiplication factor, or is it an absolute size + multiplication factor? How does that work with piecemeal encoding of[1000][1000]int? Etc.)

After 2) can we guarantee that the decoded message will have roughly the same size as encoded message, so the user of the API can protect themselves against decoding maliciously large messages?

Probably. Again, it depends on the exact details. It would be nice to provide a guarantee of the form "decoding a gob of length n will allocate at most ~100*n bytes", if that is possible, but I suspect it may not be.

Failing that, documentation.

[gopherbot]

CL https://golang.org/cl/47094 mentions this issue.

[bradfitz]

Documented for Go 1.9. Will leave open to decide whether there's more to do.