go-gtp icon indicating copy to clipboard operation
go-gtp copied to clipboard

Published 20 hours ago • verified publisher icon wmnsk

Do not panic when decoding corrupted headers

Open fawrk opened this issue 3 years ago • 1 comments

This PR fixes two issues related to decoding gtpv1 headers that contain inconsistent/corrupted data. It also adds unit tests to check that these headers are handled correctly. Each fix has its own commit.

Issue 1: Missing extension header

Sending a packet with the extension header flag set, but without the actual extension header data, would panic with an "index out of range" error. Running the new tests without the fix would give:

Panic + Stacktrace 
$ go test gtpv1/message/header_test.go
--- FAIL: TestHeaderErrorDetection (0.00s)
    --- FAIL: TestHeaderErrorDetection/ExtFlagSetButMissingExt (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered]
        panic: runtime error: index out of range [0] with length 0

goroutine 56 [running]:
testing.tRunner.func1.2({0x53e680, 0xc00015c0f0})
        /usr/lib/golang/src/testing/testing.go:1389 +0x24e
testing.tRunner.func1()
        /usr/lib/golang/src/testing/testing.go:1392 +0x39f
panic({0x53e680, 0xc00015c0f0})
        /usr/lib/golang/src/runtime/panic.go:838 +0x207
github.com/wmnsk/go-gtp/gtpv1/message.(*Header).UnmarshalBinary(0xc000156b80, {0xc00012ec90, 0x61e330?, 0xc})
        /home/useracnt/code/go-gtp/gtpv1/message/header.go:162 +0x27c
github.com/wmnsk/go-gtp/gtpv1/message.ParseHeader({0xc00012ec90, 0xc, 0xc})
        /home/useracnt/code/go-gtp/gtpv1/message/header.go:115 +0x48
command-line-arguments_test.TestHeaderErrorDetection.func1(0xc000171ba0)
        /home/useracnt/code/go-gtp/gtpv1/message/header_test.go:235 +0x36
testing.tRunner(0xc000171ba0, 0xc000112fd0)
        /usr/lib/golang/src/testing/testing.go:1439 +0x102
created by testing.(*T).Run
        /usr/lib/golang/src/testing/testing.go:1486 +0x35f
FAIL    command-line-arguments  0.004s
FAIL

The underlying issue was that if no extension headers were found and if no error was reported during unmarshal, the following line would panic:

h.ExtensionHeaders[0].Type = h.NextExtensionHeaderType

The proposed fix is to no longer silently break and return if an extension header is expected but the remaining pkt length is zero.

Issue 2: An incorrect (too long) length in the header causes a panic

When extracting the payload from the remaining packet bytes, the remaining length is not checked and an out-of-bounds panic will result if there is not enough data.

Here is the panic+stacktrace resulting from the 2nd unit test before the fix:

Panic + Stacktrace 
$ go test gtpv1/message/header_test.go
--- FAIL: TestHeaderErrorDetection (0.00s)
    --- FAIL: TestHeaderErrorDetection/IncorrectLength (0.00s)
panic: runtime error: slice bounds out of range [:12347] with capacity 12 [recovered]
        panic: runtime error: slice bounds out of range [:12347] with capacity 12

goroutine 45 [running]:
testing.tRunner.func1.2({0x53e680, 0xc00001a288})
        /usr/lib/golang/src/testing/testing.go:1389 +0x24e
testing.tRunner.func1()
        /usr/lib/golang/src/testing/testing.go:1392 +0x39f
panic({0x53e680, 0xc00001a288})
        /usr/lib/golang/src/runtime/panic.go:838 +0x207
github.com/wmnsk/go-gtp/gtpv1/message.(*Header).UnmarshalBinary(0x4b9a40?, {0xc000016db0?, 0x634520?, 0x4b9a73?})
        /home/useracnt/code/go-gtp/gtpv1/message/header.go:169 +0x237
github.com/wmnsk/go-gtp/gtpv1/message.ParseHeader({0xc000016db0, 0xc, 0xc})
        /home/useracnt/code/go-gtp/gtpv1/message/header.go:115 +0x48
command-line-arguments_test.TestHeaderErrorDetection.func1(0xc00014d1e0)
        /home/useracnt/code/go-gtp/gtpv1/message/header_test.go:248 +0x3a
testing.tRunner(0xc00014d1e0, 0xc000067010)
        /usr/lib/golang/src/testing/testing.go:1439 +0x102
created by testing.(*T).Run
        /usr/lib/golang/src/testing/testing.go:1486 +0x35f
FAIL    command-line-arguments  0.004s
FAIL

The offending line in this case was:

h.Payload = b[offset : 8+h.Length]

Issue 3: Added a fuzzing test and found a couple more issues about unchecked slice access

(see 3rd commit)

fawrk avatar Oct 04 '22 11:10 fawrk

Hi @wmnsk!

I played around with fuzzing a bit and found another issue... go-gtp has to only support the latest two versions, right? In that case I would go ahead and just add a fuzzing test for the header. Would keep the original tests with explicit error checks as they are more descriptive of what is happening.

fawrk avatar Oct 05 '22 14:10 fawrk