search

remko / go-mkvparse

Fast Matroska parser in Go
MIT License
34 stars 11 forks source link

Panic in parseElement when element reports extremely large size #4

Closed derat closed 4 years ago

derat commented 4 years ago

I noticed (while accidentally parsing the middle of an MKV container) that it's quite easy to cause a panic in parseElement by supplying data that is interpreted as an element with an excessively-large size. The panic occurs in the data := make([]byte, size) call in the non-master-element case:

panic: runtime error: makeslice: len out of range                                                                                                     

goroutine 46 [running]:                                                                                                                               
github.com/remko/go-mkvparse.parseElement(0xe7d5a0, 0xc002816d80, 0x96, 0x0, 0xe964c0, 0xc0003667e0, 0x14, 0x0, 0x0)                                  
        /home/danerat/go/pkg/mod/github.com/remko/go-mkvparse@v0.0.0-20190519181556-e343daff3525/mkvparse.go:133 +0x380                               
github.com/remko/go-mkvparse.parseElements(0xe7d5a0, 0xc002816d80, 0x0, 0xffffffffffffffff, 0x0, 0xe964c0, 0xc0003667e0, 0x4, 0xc002826740, 0x452eef) 
        /home/danerat/go/pkg/mod/github.com/remko/go-mkvparse@v0.0.0-20190519181556-e343daff3525/mkvparse.go:84 +0xce
github.com/remko/go-mkvparse.Parse(0xe7d5a0, 0xc002816d80, 0xe964c0, 0xc0003667e0, 0x13c16a0, 0x79a1c8858d01)
        /home/danerat/go/pkg/mod/github.com/remko/go-mkvparse@v0.0.0-20190519181556-e343daff3525/mkvparse.go:73 +0x6e
...

For example, I'm able to trigger this by passing []byte{0xa3, 0x01, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, which I believe corresponds to a SimpleBlock element of length 2^64-1. This makes it hard to use this package to parse untrusted data.

Would it be reasonable to have a maximum element size? I'm not familiar enough with the MKV spec to know if it would make sense to hardcode a limit in go-mkvparse, or if users should be able to specify one (perhaps via a function on the Handler interface, or if you want to avoid non-backward-compatible changes, by making Parse accept a new ...Option arg in the pattern described at e.g. https://www.sohamkamani.com/golang/options-pattern/).

And thanks for making this package! I had trouble finding any other EBML/MKV parsers that would actually let me access raw block data.

derat commented 4 years ago

(Oh, and I'm happy to provide a PR if any of those proposed fixes sound good.)

derat commented 4 years ago

Thanks for the quick reply! Yes, that seems like it would work -- memory consumption would be bounded by the size of the input.

Is https://github.com/remko/go-mkvparse/pull/5 along the lines of what you're thinking? (I'm also adding some error-checking that looks like it might've been missing.)

remko commented 4 years ago

That looks great, thanks!

Btw, I think the issue of too large blocks may still need to be addressed at some point in one way or another, in the case the MKV file is being streamed (which would still allow memory to be exhausted).