Fixed HBH ext parse panic when start >= end

[AlexanderKunze]

The router could crash if a packet arrived with data resulting in the function go/border/rpkt/extn_scmp.go/rSCMPExtFromRaw(...) receiving a start >= end, which would panic on line 38.

The error is caused by go/border/rpkt/parse.go/rp.parseHopExtns() calling rp.extnParseHBH(...) on line 123, which then calls rSCMPExtFromRaw(...) with no checks for start or end.

I decided to add an error check in rp.extnParseHBH(...) instead of where the panic happens (rSCMPExtFromRaw(...)), because a header of invalid offset or length should fail, even when start or end are not read, such as in rOneHopPathFromRaw.

The problem might also be fixed earlier, in rp.parseHopExtns(), where the erroneous values are actually read in. However, I was unsure if this is better and so left the error catching in rp.extnParseHBH(...)

I also chose to use as an error check that the total length of the extension must be greater than common.LineLen, since an extension of any less size should not be valid anyway.

For reference: The packet that caused the panic was:

00c10067070405000001ff00000001110001ff000000011080007f00000b0000857b3362e3400140003f0000014152c1000000000000000000000000000000001101010000000000792c000000271f920000001b10084002110582510801027f10065002020102

---

This change is 

[matzf]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AlexanderKunze and @sgmonroy)

a discussion (no related file):
Btw. looking at this parseHopExtns, I had the impression that there might be another lurking bug; I can't see a check that actually ensures that the packet data has even one more line before attempting to read next extension headers; in the following lines, access to rp.Raw at indices > *offset appear to be unchecked.

scion/go/border/rpkt/parse.go

Lines 121 to 122 in 82be069

	currExtn := common.ExtnType{Class: currHdr, Type: rp.Raw[*offset+2]}
	hdrLen := int(rp.Raw[*offset+1]) * common.LineLen

I think if it somehow possible to create a packet that ends on an extension saying that NextHdr will be another hop-by-hop extension, this would also panic.

---

---

go/border/rpkt/extns.go, line 44 at r1 (raw file):

func (rp *RtrPkt) extnParseHBH(extType common.ExtnType,
	start, end, pos int) (rExtension, error) {
	if end-start <= common.LineLen {

Shouldn't this only error if the length is strictly smaller than linelen?
Looking at both callers and callees of this function, it appears that start is already offset by ExtnSubHdrLen.
Putting this together, the check here should be: if end-start < common.LineLen - common.ExtnSubHdrLen.

To avoid this dance with adding/subtracting the ExtnSubHdrLen, I'd propose to move the check to parseHopExtns, as you suggested yourself.
There, you could do the check directly on the packet ExtHdrLen field of the packet, only checking that it's > 0.

[sgmonroy]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)

---

go/border/rpkt/extns.go, line 44 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Shouldn't this only error if the length is strictly smaller than linelen?
Looking at both callers and callees of this function, it appears that start is already offset by ExtnSubHdrLen.
Putting this together, the check here should be: if end-start < common.LineLen - common.ExtnSubHdrLen.

To avoid this dance with adding/subtracting the ExtnSubHdrLen, I'd propose to move the check to parseHopExtns, as you suggested yourself.
There, you could do the check directly on the packet ExtHdrLen field of the packet, only checking that it's > 0.

Agreed. Even if you leave this check here for safety, although it could just be an assert as this is BR specific, there should be checks in parseHopExtns given that you already access the packet data in that function.

[sgmonroy]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)

[AlexanderKunze]

Thank you @sgmonroy and @matzf for those excellent and extensive comments. I have made changes and issued a new commit.

The changes are as follows:

• extnParseHBH now has an assert that end-start >= common.LineLen.
• Following the comments here, I added an additional check that returns an error if *offset+2 >= len(rp.Raw), to ensure that hdrLen and currExtn cannot panic when accessing rp.Raw[*offset+2] and rp.Raw[*offset+1]. For this case, I copied the error message from below that the Extension header chain is too long.
• I moved the check for start >= end to rp.parseHopExtns(), as suggested, since I believe this possible panic is not caught by the *offset+2 >= len(rp.Raw) check. The function call to rp.extnParseHBH was adjusted and two variables were added so as to avoid double calculation of start and end
• An additional error message was created called ErrExtChainCorrupt for the case in which the ExtHdrLen field is corrupt, even if the extension header chain is long enough.

Please let me know any other feedback you have or if I've misunderstood something, made a mistake, or if there is something else I can do.

[sgmonroy]

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AlexanderKunze and @matzf)

---

go/border/rpkt/parse.go, line 128 at r2 (raw file):

		start := *offset + common.ExtnSubHdrLen
		end := *offset + hdrLen
		if start >= end {

you should also check in this function that end < len(rp.Raw)

[matzf]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AlexanderKunze and @sgmonroy)

---

go/border/rpkt/extns.go, line 44 at r1 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

Agreed. Even if you leave this check here for safety, although it could just be an assert as this is BR specific, there should be checks in parseHopExtns given that you already access the packet data in that function.

Now the check in the assert still needs to take into account that start is already offset by ExtnSubHdrLen:

assert.Must(end-start + common.ExtnSubHdrLen >= common.LineLen , "Extension must be at least one line length")

[AlexanderKunze]

Hi and thanks for the comments once again. Sorry it took so long to make these changes. I have issued the commit. Let me know if there's something else I can do.

[sgmonroy]

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AlexanderKunze)

---

go/border/rpkt/parse.go, line 128 at r2 (raw file):

Previously, sgmonroy (Sergio Gonzalez Monroy) wrote…

you should also check in this function that end < len(rp.Raw)

I saw that acceptance tests are failing. Indeed, we do support having an extension without any other layer afterwards, meaning the check should be end > len(rp.Raw)

[AlexanderKunze]

Thanks for the correction, adjusted it accordingly.

[matzf]

Reviewed 1 of 2 files at r3, 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AlexanderKunze)

[sgmonroy]

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