search

dhowden / tag

ID3, MP4 and OGG/FLAC metadata parsing in Go
BSD 2-Clause "Simplified" License
558 stars 72 forks source link

some vulnerability - 0x03 an out-of-bound vulnerability in readTextWithDescrFrame function #79

Closed Jayl1n closed 3 years ago

Jayl1n commented 3 years ago

This is the third vulnerability in id3v2frames.go

In readTextWithDescrFrame function, you don't check the size of b , program will happen panic when the size of b is 2 or less than 2 .

testcase 8eff69ad26a59a05ec11e38f3ca6c592f08dcc54.zip

panic: runtime error: slice bounds out of range [:3] with capacity 2

goroutine 1 [running]:
github.com/dhowden/tag.readTextWithDescrFrame(0xc0000da038, 0x3, 0x3, 0x101, 0x122b1a0, 0x0, 0x0)
        /Users/jaylin/go/pkg/mod/github.com/dhowden/tag@v0.0.0-20200828214007-46e57f75dbfc/id3v2frames.go:460 +0x46e
github.com/dhowden/tag.readID3v2Frames(0x114d680, 0xc0000d8000, 0x17, 0xc0000dc000, 0xc0000d8000, 0x0, 0xb)
        /Users/jaylin/go/pkg/mod/github.com/dhowden/tag@v0.0.0-20200828214007-46e57f75dbfc/id3v2.go:364 +0x90b
github.com/dhowden/tag.ReadID3v2Tags(0x114daa0, 0xc0000d8000, 0x1, 0x0, 0x0, 0x0)
        /Users/jaylin/go/pkg/mod/github.com/dhowden/tag@v0.0.0-20200828214007-46e57f75dbfc/id3v2.go:428 +0xde
github.com/dhowden/tag.ReadFrom(0x114daa0, 0xc0000d8000, 0xc0000d6000, 0x17, 0x217, 0x0)
        /Users/jaylin/go/pkg/mod/github.com/dhowden/tag@v0.0.0-20200828214007-46e57f75dbfc/tag.go:52 +0x324
main.main()
        /Users/jaylin/GolandProjects/gofuzz_test/main.go:20 +0xb5
dhowden commented 3 years ago

Duplicate of #76

Jayl1n commented 3 years ago

If the array size is less than 2, it still panic in the latest commit, just like in the figure below

图片

图片

@dhowden

dhowden commented 3 years ago

Ah yes! Thanks. Happy to receive a pull request to fix :-).

Jayl1n commented 3 years ago

Ah yes! Thanks. Happy to receive a pull request to fix :-).

@dhowden I'm Sorry. My code is so terrible, but I can give you an advice if you don't have a better way to fix such bugs.

You could use recover() function in which caller to regains control of a panicking goroutine. see more detail

dhowden commented 3 years ago

Ah yes! Thanks. Happy to receive a pull request to fix :-).

@dhowden I'm Sorry. My code is so terrible, but I can give you an advice if you don't have a better way to fix such bugs.

No worries :-)

I will have a look now.

dhowden commented 3 years ago

Just to note: the library was built to read data from valid files (and making it conform to all the specs was bad enough, so I mostly ignored safety measures to trap bad files).. Using a fuzzer will likely find lots of issues like this!

If people are using this in production environments, would definitely recommend that they wrap all alls to the library with recover (as you suggest above) to make sure that a panic here does not bring down their entire process.

wader commented 3 years ago

@Jayl1n @dhowden nice work! would be possible to add your fuzzing test code to the repo?