Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/net/icmp: Malformed Interface Identification Object #28686

Closed
uhei opened this issue Nov 9, 2018 · 3 comments
Closed

x/net/icmp: Malformed Interface Identification Object #28686

uhei opened this issue Nov 9, 2018 · 3 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@uhei
Copy link

uhei commented Nov 9, 2018 •
edited

What version of Go are you using (go version)?

go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"

What did you do?

Run a test script to send ICMP ExtendedEchoRequest package.
Script: https://play.golang.org/p/LztQf4qLl4l

What did you expect to see?

08:23:28.573685 IP6 fe80::1c7c:caa:f732:f468 > fe80::5: ICMP6, unknown icmp6 type (160), length 24
        0x0000:  a000 e91a 00c8 6401 2000 1554 000c 0301
        0x0010:  656e 3130 3100 0000

What did you see instead?

08:29:30.094564 IP6 fe80::1c7c:caa:f732:f468 > fe80::5: ICMP6, unknown icmp6 type (160), length 28
        0x0000:  a000 e916 00c8 6401 0000 0000 2000 1554
        0x0010:  000c 0301 656e 3130 3100 0000

Issue is that the marshalMultipartMessageBody() function in multipart.go returns a byte array which starts with four zero bytes (maybe this should be the header).
The Marshal() function in echo.go appends another four bytes header.

This results in a malformed packet with four zero bytes between ICMP header (here: 00c8 6401) and the Multi-Part Extension header (here: 2000 1554).

RFC 8335 Section 2 defines:

       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |     Type      |     Code      |          Checksum             |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |           Identifier          |Sequence Number|   Reserved  |L|
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |   ICMP Extension Structure

               Figure 1: ICMP Extended Echo Request Message

Two possible ways to fix this:

diff --git a/icmp/echo.go b/icmp/echo.go
index c611f65..19bbb79 100644
--- a/icmp/echo.go
+++ b/icmp/echo.go
@@ -74,7 +74,7 @@ func (p *ExtendedEchoRequest) Marshal(proto int) ([]byte, error) {
        if p.Local {
                bb[3] |= 0x01
        }
-       bb = append(bb, b...)
+       bb = append(bb, b[4:]...)
        return bb, nil
 }

or

diff --git a/icmp/echo.go b/icmp/echo.go
index c611f65..dbc89ea 100644
--- a/icmp/echo.go
+++ b/icmp/echo.go
@@ -68,14 +68,12 @@ func (p *ExtendedEchoRequest) Marshal(proto int) ([]byte, error) {
        if err != nil {
                return nil, err
        }
-       bb := make([]byte, 4)
-       binary.BigEndian.PutUint16(bb[:2], uint16(p.ID))
-       bb[2] = byte(p.Seq)
+       binary.BigEndian.PutUint16(b[:2], uint16(p.ID))
+       b[2] = byte(p.Seq)
        if p.Local {
-               bb[3] |= 0x01
+               b[3] |= 0x01
        }
-       bb = append(bb, b...)
-       return bb, nil
+       return b, nil
 }

 // parseExtendedEchoRequest parses b as an ICMP extended echo request

Both fixes breaks the test code. => Therefore test code must be fixed too. As I have no clue how the test code works I failed to do so. I won't open a PR.
@gopherbot gopherbot added this to the Unreleased milestone Nov 9, 2018
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 9, 2018
@ianlancetaylor
Copy link
Contributor

CC @mikioh

@gopherbot
Copy link

Change https://golang.org/cl/155859 mentions this issue: icmp: add simple multipart message validation

@mikioh
Copy link
Contributor

mikioh commented Dec 27, 2018

Nice catch, thanks for the report. Please feel free to review CL 155859.

@mikioh mikioh removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 28, 2018
@golang golang locked and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@mikioh @uhei @ianlancetaylor @gopherbot