Check buffer size before reading in raw functions #3060

karampok · 2019-08-28T12:23:25Z

Fixes #2050

This change is

oncilla

Reviewed 3 of 3 files at r1, 5 of 5 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @karampok)

go/lib/hpkt/hpkt_test.go, line 165 at r2 (raw file):

			raw: append(makeCmnHdr(512+3, 64, 512, 0), make([]byte, 3)...),
		},
		"error x": {

choose a better name for x

go/lib/hpkt/read.go, line 154 at r2 (raw file):

		p.nextHdr = extn.NextHeader
		p.offset += len(extn.Contents)

remove

go/lib/hpkt/read.go, line 176 at r2 (raw file):

			"expected", p.cmnHdr.HdrLenBytes(), "larger than ", len(p.b))
	}

remove

go/lib/hpkt/read.go, line 182 at r2 (raw file):

func (p *parseCtx) DefaultAddrHdrParser() error {
	var err error
	//TODO(karampok). Hard code hdr start at (int)common.CmnHdrLen

Todo for this PR, or a future one?

go/lib/layers/extensions_layer.go, line 70 at r2 (raw file):

		df.SetTruncated()
		return common.NewBasicError("Invalid SCION Extension header, expected length is invalid",
			fmt.Errorf("value %v is < 0", expectedLength))

return common.NewBasicError("Invalid SCION Extension header, length is zero", nil)

karampok · 2019-08-30T07:25:15Z

go/lib/hpkt/read.go, line 182 at r2 (raw file):

Previously, Oncilla wrote…

Todo for this PR, or a future one?

Topic to discuss, I would not hardcoded but this makes is safe. What do you think?
(I vote to just remove the todo)

karampok · 2019-08-30T07:27:16Z

go/lib/hpkt/read.go, line 176 at r2 (raw file):

Previously, Oncilla wrote…

remove

I think this error is needed because I have removed it (or I think) from the function we call the CmdHdrParser() . If you can check versus the master commit

oncilla

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @karampok)

go/lib/hpkt/read.go, line 176 at r2 (raw file):

Previously, karampok (Konstantinos) wrote…

I think this error is needed because I have removed it (or I think) from the function we call the CmdHdrParser() . If you can check versus the master commit

sorry I was unclear. Remove the trailing line break 😄

go/lib/hpkt/read.go, line 182 at r2 (raw file):

Previously, karampok (Konstantinos) wrote…

Topic to discuss, I would not hardcoded but this makes is safe. What do you think?
(I vote to just remove the todo)

yeah, let's just remove the todo

karampok

Reviewable status: 5 of 32 files reviewed, 5 unresolved discussions (waiting on @oncilla)

go/lib/hpkt/hpkt_test.go, line 165 at r2 (raw file):

Previously, Oncilla wrote…

choose a better name for x

Done.

go/lib/hpkt/read.go, line 154 at r2 (raw file):

Previously, Oncilla wrote…

remove

i suppose you mean also \n

go/lib/hpkt/read.go, line 176 at r2 (raw file):

Previously, Oncilla wrote…

sorry I was unclear. Remove the trailing line break 😄

Done.

go/lib/hpkt/read.go, line 182 at r2 (raw file):

Previously, Oncilla wrote…

yeah, let's just remove the todo

Done.

go/lib/layers/extensions_layer.go, line 70 at r2 (raw file):

Previously, Oncilla wrote…

return common.NewBasicError("Invalid SCION Extension header, length is zero", nil)

Done.

oncilla

Reviewed 26 of 27 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @karampok)

go/lib/hpkt/hpkt_test.go, line 162 at r4 (raw file):

	require.NoError(t, err)
	for _, f := range fs {
		b := xtest.MustReadFromFile(t, filepath.Join("fuzz-inputs", f.Name()))

This is good enough for now. But we should think about how we can incorporate fuzzing in our workflow, and where to put the corpus.

go/lib/hpkt/hpkt_test.go, line 173 at r4 (raw file):

				err = ParseScnPkt(s, b)
			}
			require.NotPanics(t, w)

They way it is now, it cannot panic in any case.
Add a require.NotContains(t, err.Error(), "panic")

go/lib/scmp/info.go, line 253 at r4 (raw file):

func InfoExtIdxFromRaw(b common.RawBytes) (*InfoExtIdx, error) {
	if len(b) == 0 {
		return nil, common.NewBasicError("Unable to parse", nil)

"Unable to parse InfoExtIdx", otherwise the error is not really helpful.

go/lib/scmp/info_traceroute.go, line 60 at r4 (raw file):

func InfoTraceRouteFromRaw(b common.RawBytes) (*InfoTraceRoute, error) {
	if len(b) < traceRouteLen {
		return nil, common.NewBasicError("Unable to parse, small buffer size", nil)

"unable to parse InfoTraceRoute, small buffer size"

karampok · 2019-08-30T10:28:33Z

go/lib/hpkt/hpkt_test.go, line 173 at r4 (raw file):

require.NotContains(t, err.Error(), "panic")
the thing is I want to have a input that does panic, I make sure that nobody remove the defer/recover from the flow. Does it makes sense?

karampok

Reviewable status: 28 of 32 files reviewed, 3 unresolved discussions (waiting on @oncilla)

go/lib/scmp/info.go, line 253 at r4 (raw file):

Previously, Oncilla wrote…

"Unable to parse InfoExtIdx", otherwise the error is not really helpful.

Done.

go/lib/scmp/info_traceroute.go, line 60 at r4 (raw file):

Previously, Oncilla wrote…

"unable to parse InfoTraceRoute, small buffer size"

Done.

oncilla

Reviewed 2 of 4 files at r5.
Reviewable status: 30 of 32 files reviewed, 2 unresolved discussions (waiting on @karampok and @oncilla)

go/lib/hpkt/read.go, line 227 at r5 (raw file):

	case common.L4UDP:
		if len(p.b) < p.offset+l4.UDPLen {
			return common.NewBasicError("Unable to parse UDP header", err)

"Unable to parse UDP header, small buffer size", nil

go/lib/hpkt/read.go, line 234 at r5 (raw file):

	case common.L4SCMP:
		if len(p.b) < p.offset+scmp.HdrLen {
			return common.NewBasicError("Unable to parse SCMP header", err)

"Unable to parse SCMP header, small buffer size", nil

* Check buffer size before reading in raw functions * Safe when hdr length looks at last byte * Make parsing of extension safer * Add fuzz-inputs for testing Fixes scionproto#2050

karampok

Reviewable status: 29 of 32 files reviewed, 2 unresolved discussions (waiting on @oncilla)

go/lib/hpkt/read.go, line 227 at r5 (raw file):

Previously, Oncilla wrote…

"Unable to parse UDP header, small buffer size", nil

Done.

go/lib/hpkt/read.go, line 234 at r5 (raw file):

Previously, Oncilla wrote…

"Unable to parse SCMP header, small buffer size", nil

Done.

oncilla

Reviewed 2 of 4 files at r5, 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

oncilla

Reviewable status: complete! all files reviewed, all discussions resolved

karampok force-pushed the pub-check-buffer-size branch 3 times, most recently from 9e7ce29 to 9a5ae84 Compare August 29, 2019 13:07

karampok requested a review from oncilla August 29, 2019 13:43

oncilla requested changes Aug 30, 2019

View reviewed changes

karampok force-pushed the pub-check-buffer-size branch 3 times, most recently from a3d2110 to 3b1aa58 Compare August 30, 2019 09:04

karampok commented Aug 30, 2019

View reviewed changes

oncilla requested changes Aug 30, 2019

View reviewed changes

karampok force-pushed the pub-check-buffer-size branch from 3b1aa58 to f15ad35 Compare August 30, 2019 11:06

karampok commented Aug 30, 2019

View reviewed changes

oncilla requested changes Aug 30, 2019

View reviewed changes

Guard pkt parsing through defer/recover

73d780c

* Check buffer size before reading in raw functions * Safe when hdr length looks at last byte * Make parsing of extension safer * Add fuzz-inputs for testing Fixes scionproto#2050

karampok force-pushed the pub-check-buffer-size branch from f15ad35 to 73d780c Compare August 30, 2019 11:22

karampok commented Aug 30, 2019

View reviewed changes

oncilla approved these changes Aug 30, 2019

View reviewed changes

karampok merged commit 2d15055 into scionproto:master Aug 30, 2019

karampok deleted the pub-check-buffer-size branch August 30, 2019 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Check buffer size before reading in raw functions #3060

Check buffer size before reading in raw functions #3060

Uh oh!

karampok commented Aug 28, 2019 •

edited by kormat

Loading

Uh oh!

oncilla left a comment

Uh oh!

karampok commented Aug 30, 2019

Uh oh!

karampok commented Aug 30, 2019

Uh oh!

oncilla left a comment

Uh oh!

karampok left a comment

Uh oh!

oncilla left a comment

Uh oh!

karampok commented Aug 30, 2019

Uh oh!

karampok left a comment

Uh oh!

oncilla left a comment

Uh oh!

karampok left a comment

Uh oh!

oncilla left a comment

Uh oh!

oncilla left a comment

Uh oh!

Uh oh!

Check buffer size before reading in raw functions #3060

Check buffer size before reading in raw functions #3060

Uh oh!

Conversation

karampok commented Aug 28, 2019 • edited by kormat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

karampok commented Aug 30, 2019

Uh oh!

karampok commented Aug 30, 2019

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

karampok left a comment

Choose a reason for hiding this comment

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

karampok commented Aug 30, 2019

Uh oh!

karampok left a comment

Choose a reason for hiding this comment

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

karampok left a comment

Choose a reason for hiding this comment

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karampok commented Aug 28, 2019 •

edited by kormat

Loading