diff options
author | Julian Elischer <jrelis@google.com> | 2021-01-20 15:33:53 -0800 |
---|---|---|
committer | gVisor bot <gvisor-bot@google.com> | 2021-01-20 15:36:03 -0800 |
commit | 2865166403896a668167678ba0a8c2ef24268ca0 (patch) | |
tree | a27432b3ce33de82472adf4e56f3ec6832f90e08 /pkg/tcpip/network | |
parent | 7ff5ceaeae66303ed6a2199963c00cb08b2fe7ca (diff) |
Change the way the IP options report problems
The error messages are not needed or used as these are not processing errors
so much as errors to be reported back to the packet sender. Implicitly
describe whether each error should generate ICMP packets or not. Most do
but there are a couple that do not.
Slightly alter some test expectations for Linux compatibility and add a
couple more. Improve Linux compatibility on error packet returns. Some
cosmetic changes to tests to match the upcoming packet impact version
of the same tests.
PiperOrigin-RevId: 352889785
Diffstat (limited to 'pkg/tcpip/network')
-rw-r--r-- | pkg/tcpip/network/ipv4/icmp.go | 20 | ||||
-rw-r--r-- | pkg/tcpip/network/ipv4/ipv4.go | 123 | ||||
-rw-r--r-- | pkg/tcpip/network/ipv4/ipv4_test.go | 86 |
3 files changed, 142 insertions, 87 deletions
diff --git a/pkg/tcpip/network/ipv4/icmp.go b/pkg/tcpip/network/ipv4/icmp.go index 3f60de749..284ad89cf 100644 --- a/pkg/tcpip/network/ipv4/icmp.go +++ b/pkg/tcpip/network/ipv4/icmp.go @@ -1,4 +1,4 @@ -// Copyright 2018 The gVisor Authors. +// Copyright 2021 The gVisor Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,7 +15,6 @@ package ipv4 import ( - "errors" "fmt" "gvisor.dev/gvisor/pkg/tcpip" @@ -105,17 +104,12 @@ func (e *endpoint) handleICMP(pkt *stack.PacketBuffer) { } else { op = &optionUsageReceive{} } - aux, tmp, err := e.processIPOptions(pkt, opts, op) - if err != nil { - switch { - case - errors.Is(err, header.ErrIPv4OptDuplicate), - errors.Is(err, errIPv4RecordRouteOptInvalidLength), - errors.Is(err, errIPv4RecordRouteOptInvalidPointer), - errors.Is(err, errIPv4TimestampOptInvalidLength), - errors.Is(err, errIPv4TimestampOptInvalidPointer), - errors.Is(err, errIPv4TimestampOptOverflow): - _ = e.protocol.returnError(&icmpReasonParamProblem{pointer: aux}, pkt) + tmp, optProblem := e.processIPOptions(pkt, opts, op) + if optProblem != nil { + if optProblem.NeedICMP { + _ = e.protocol.returnError(&icmpReasonParamProblem{ + pointer: optProblem.Pointer, + }, pkt) e.protocol.stack.Stats().MalformedRcvdPackets.Increment() e.stats.ip.MalformedPacketsReceived.Increment() } diff --git a/pkg/tcpip/network/ipv4/ipv4.go b/pkg/tcpip/network/ipv4/ipv4.go index 7f03696ae..f295a9192 100644 --- a/pkg/tcpip/network/ipv4/ipv4.go +++ b/pkg/tcpip/network/ipv4/ipv4.go @@ -1,4 +1,4 @@ -// Copyright 2018 The gVisor Authors. +// Copyright 2021 The gVisor Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -16,7 +16,6 @@ package ipv4 import ( - "errors" "fmt" "math" "reflect" @@ -780,17 +779,11 @@ func (e *endpoint) handlePacket(pkt *stack.PacketBuffer) { // TODO(gvisor.dev/issue/4586): // When we add forwarding support we should use the verified options // rather than just throwing them away. - aux, _, err := e.processIPOptions(pkt, opts, &optionUsageReceive{}) - if err != nil { - switch { - case - errors.Is(err, header.ErrIPv4OptDuplicate), - errors.Is(err, errIPv4RecordRouteOptInvalidPointer), - errors.Is(err, errIPv4RecordRouteOptInvalidLength), - errors.Is(err, errIPv4TimestampOptInvalidLength), - errors.Is(err, errIPv4TimestampOptInvalidPointer), - errors.Is(err, errIPv4TimestampOptOverflow): - _ = e.protocol.returnError(&icmpReasonParamProblem{pointer: aux}, pkt) + if _, optProblem := e.processIPOptions(pkt, opts, &optionUsageReceive{}); optProblem != nil { + if optProblem.NeedICMP { + _ = e.protocol.returnError(&icmpReasonParamProblem{ + pointer: optProblem.Pointer, + }, pkt) e.protocol.stack.Stats().MalformedRcvdPackets.Increment() stats.ip.MalformedPacketsReceived.Increment() } @@ -1233,16 +1226,9 @@ func (*optionUsageEcho) actions() optionActions { } } -var ( - errIPv4TimestampOptInvalidLength = errors.New("invalid Timestamp length") - errIPv4TimestampOptInvalidPointer = errors.New("invalid Timestamp pointer") - errIPv4TimestampOptOverflow = errors.New("overflow in Timestamp") - errIPv4TimestampOptInvalidFlags = errors.New("invalid Timestamp flags") -) - // handleTimestamp does any required processing on a Timestamp option // in place. -func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Address, clock tcpip.Clock, usage optionsUsage) (uint8, error) { +func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Address, clock tcpip.Clock, usage optionsUsage) *header.IPv4OptParameterProblem { flags := tsOpt.Flags() var entrySize uint8 switch flags { @@ -1253,7 +1239,10 @@ func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Addres header.IPv4OptionTimestampWithPredefinedIPFlag: entrySize = header.IPv4OptionTimestampWithAddrSize default: - return header.IPv4OptTSOFLWAndFLGOffset, errIPv4TimestampOptInvalidFlags + return &header.IPv4OptParameterProblem{ + Pointer: header.IPv4OptTSOFLWAndFLGOffset, + NeedICMP: true, + } } pointer := tsOpt.Pointer() @@ -1261,7 +1250,10 @@ func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Addres // Since the pointer is 1 based, and the header is 4 bytes long the // pointer must point beyond the header therefore 4 or less is bad. if pointer <= header.IPv4OptionTimestampHdrLength { - return header.IPv4OptTSPointerOffset, errIPv4TimestampOptInvalidPointer + return &header.IPv4OptParameterProblem{ + Pointer: header.IPv4OptTSPointerOffset, + NeedICMP: true, + } } // To simplify processing below, base further work on the array of timestamps // beyond the header, rather than on the whole option. Also to aid @@ -1295,14 +1287,17 @@ func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Addres // timestamp, but the overflow count is incremented by one. if flags == header.IPv4OptionTimestampWithPredefinedIPFlag { // By definition we have nothing to do. - return 0, nil + return nil } if tsOpt.IncOverflow() != 0 { - return 0, nil + return nil } // The overflow count is also full. - return header.IPv4OptTSOFLWAndFLGOffset, errIPv4TimestampOptOverflow + return &header.IPv4OptParameterProblem{ + Pointer: header.IPv4OptTSOFLWAndFLGOffset, + NeedICMP: true, + } } if nextSlot+entrySize > dataLength { // The data area isn't full but there isn't room for a new entry. @@ -1321,32 +1316,36 @@ func handleTimestamp(tsOpt header.IPv4OptionTimestamp, localAddress tcpip.Addres if dataLength%entrySize != 0 { // The Data section size should be a multiple of the expected // timestamp entry size. - return header.IPv4OptionLengthOffset, errIPv4TimestampOptInvalidLength + return &header.IPv4OptParameterProblem{ + Pointer: header.IPv4OptionLengthOffset, + NeedICMP: false, + } } // If the size is OK, the pointer must be corrupted. } - return header.IPv4OptTSPointerOffset, errIPv4TimestampOptInvalidPointer + return &header.IPv4OptParameterProblem{ + Pointer: header.IPv4OptTSPointerOffset, + NeedICMP: true, + } } if usage.actions().timestamp == optionProcess { tsOpt.UpdateTimestamp(localAddress, clock) } - return 0, nil + return nil } -var ( - errIPv4RecordRouteOptInvalidLength = errors.New("invalid length in Record Route") - errIPv4RecordRouteOptInvalidPointer = errors.New("invalid pointer in Record Route") -) - // handleRecordRoute checks and processes a Record route option. It is much // like the timestamp type 1 option, but without timestamps. The passed in // address is stored in the option in the correct spot if possible. -func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Address, usage optionsUsage) (uint8, error) { +func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Address, usage optionsUsage) *header.IPv4OptParameterProblem { optlen := rrOpt.Size() if optlen < header.IPv4AddressSize+header.IPv4OptionRecordRouteHdrLength { - return header.IPv4OptionLengthOffset, errIPv4RecordRouteOptInvalidLength + return &header.IPv4OptParameterProblem{ + Pointer: header.IPv4OptionLengthOffset, + NeedICMP: true, + } } pointer := rrOpt.Pointer() @@ -1356,7 +1355,10 @@ func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Ad // Since the pointer is 1 based, and the header is 3 bytes long the // pointer must point beyond the header therefore 3 or less is bad. if pointer <= header.IPv4OptionRecordRouteHdrLength { - return header.IPv4OptRRPointerOffset, errIPv4RecordRouteOptInvalidPointer + return &header.IPv4OptParameterProblem{ + Pointer: header.IPv4OptRRPointerOffset, + NeedICMP: true, + } } // RFC 791 page 21 says @@ -1373,7 +1375,7 @@ func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Ad // of these words is a copy/paste error from the timestamp option where // there are two failure reasons given. if pointer > optlen { - return 0, nil + return nil } // The data area isn't full but there isn't room for a new entry. @@ -1398,17 +1400,23 @@ func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Ad // } if (optlen-header.IPv4OptionRecordRouteHdrLength)%header.IPv4AddressSize != 0 { // Length is bad, not on integral number of slots. - return header.IPv4OptionLengthOffset, errIPv4RecordRouteOptInvalidLength + return &header.IPv4OptParameterProblem{ + Pointer: header.IPv4OptionLengthOffset, + NeedICMP: true, + } } // If not length, the fault must be with the pointer. } - return header.IPv4OptRRPointerOffset, errIPv4RecordRouteOptInvalidPointer + return &header.IPv4OptParameterProblem{ + Pointer: header.IPv4OptRRPointerOffset, + NeedICMP: true, + } } if usage.actions().recordRoute == optionVerify { - return 0, nil + return nil } rrOpt.StoreAddress(localAddress) - return 0, nil + return nil } // processIPOptions parses the IPv4 options and produces a new set of options @@ -1419,7 +1427,7 @@ func handleRecordRoute(rrOpt header.IPv4OptionRecordRoute, localAddress tcpip.Ad // - The location of an error if there was one (or 0 if no error) // - If there is an error, information as to what it was was. // - The replacement option set. -func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Options, usage optionsUsage) (uint8, header.IPv4Options, error) { +func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Options, usage optionsUsage) (header.IPv4Options, *header.IPv4OptParameterProblem) { stats := e.stats.ip opts := header.IPv4Options(orig) optIter := opts.MakeIterator() @@ -1439,15 +1447,17 @@ func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Opt h := header.IPv4(pkt.NetworkHeader().View()) dstAddr := h.DestinationAddress() if pkt.NetworkPacketInfo.LocalAddressBroadcast || header.IsV4MulticastAddress(dstAddr) { - return 0 /* errCursor */, nil, header.ErrIPv4OptionAddress + return nil, &header.IPv4OptParameterProblem{ + NeedICMP: false, + } } localAddress = dstAddr } for { - option, done, err := optIter.Next() - if done || err != nil { - return optIter.ErrCursor, optIter.Finalize(), err + option, done, optProblem := optIter.Next() + if done || optProblem != nil { + return optIter.Finalize(), optProblem } optType := option.Type() if optType == header.IPv4OptionNOPType { @@ -1456,12 +1466,15 @@ func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Opt } if optType == header.IPv4OptionListEndType { optIter.PushNOPOrEnd(optType) - return 0 /* errCursor */, optIter.Finalize(), nil /* err */ + return optIter.Finalize(), nil } // check for repeating options (multiple NOPs are OK) if seenOptions[optType] { - return optIter.ErrCursor, nil, header.ErrIPv4OptDuplicate + return nil, &header.IPv4OptParameterProblem{ + Pointer: optIter.ErrCursor, + NeedICMP: true, + } } seenOptions[optType] = true @@ -1473,9 +1486,9 @@ func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Opt clock := e.protocol.stack.Clock() newBuffer := optIter.RemainingBuffer()[:len(*option)] _ = copy(newBuffer, option.Contents()) - offset, err := handleTimestamp(header.IPv4OptionTimestamp(newBuffer), localAddress, clock, usage) - if err != nil { - return optIter.ErrCursor + offset, nil, err + if optProblem := handleTimestamp(header.IPv4OptionTimestamp(newBuffer), localAddress, clock, usage); optProblem != nil { + optProblem.Pointer += optIter.ErrCursor + return nil, optProblem } optIter.ConsumeBuffer(optLen) } @@ -1485,9 +1498,9 @@ func (e *endpoint) processIPOptions(pkt *stack.PacketBuffer, orig header.IPv4Opt if usage.actions().recordRoute != optionRemove { newBuffer := optIter.RemainingBuffer()[:len(*option)] _ = copy(newBuffer, option.Contents()) - offset, err := handleRecordRoute(header.IPv4OptionRecordRoute(newBuffer), localAddress, usage) - if err != nil { - return optIter.ErrCursor + offset, nil, err + if optProblem := handleRecordRoute(header.IPv4OptionRecordRoute(newBuffer), localAddress, usage); optProblem != nil { + optProblem.Pointer += optIter.ErrCursor + return nil, optProblem } optIter.ConsumeBuffer(optLen) } diff --git a/pkg/tcpip/network/ipv4/ipv4_test.go b/pkg/tcpip/network/ipv4/ipv4_test.go index a9e137c24..af6f22598 100644 --- a/pkg/tcpip/network/ipv4/ipv4_test.go +++ b/pkg/tcpip/network/ipv4/ipv4_test.go @@ -1,4 +1,4 @@ -// Copyright 2018 The gVisor Authors. +// Copyright 2021 The gVisor Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -270,6 +270,11 @@ func TestIPv4Sanity(t *testing.T) { nicID = 1 randomSequence = 123 randomIdent = 42 + // In some cases Linux sets the error pointer to the start of the option + // (offset 0) instead of the actual wrong value, which is the length byte + // (offset 1). For compatibility we must do the same. Use this constant + // to indicate where this happens. + pointerOffsetForInvalidLength = 0 ) var ( ipv4Addr = tcpip.AddressWithPrefix{ @@ -439,6 +444,21 @@ func TestIPv4Sanity(t *testing.T) { replyOptions: header.IPv4Options{}, }, { + name: "bad option - no length", + maxTotalLength: ipv4.MaxTotalSize, + transportProtocol: uint8(header.ICMPv4ProtocolNumber), + TTL: ttl, + options: header.IPv4Options{ + 1, 1, 1, 68, + // ^-start of timestamp.. but no length.. + }, + shouldFail: true, + expectErrorICMP: true, + ICMPType: header.ICMPv4ParamProblem, + ICMPCode: header.ICMPv4UnusedCode, + paramProblemPointer: header.IPv4MinimumSize + 3, + }, + { name: "bad option - length 0", maxTotalLength: ipv4.MaxTotalSize, transportProtocol: uint8(header.ICMPv4ProtocolNumber), @@ -448,7 +468,27 @@ func TestIPv4Sanity(t *testing.T) { // ^ 1, 2, 3, 4, }, - shouldFail: true, + shouldFail: true, + expectErrorICMP: true, + ICMPType: header.ICMPv4ParamProblem, + ICMPCode: header.ICMPv4UnusedCode, + paramProblemPointer: header.IPv4MinimumSize + pointerOffsetForInvalidLength, + }, + { + name: "bad option - length 1", + maxTotalLength: ipv4.MaxTotalSize, + transportProtocol: uint8(header.ICMPv4ProtocolNumber), + TTL: ttl, + options: header.IPv4Options{ + 68, 1, 9, 0, + // ^ + 1, 2, 3, 4, + }, + shouldFail: true, + expectErrorICMP: true, + ICMPType: header.ICMPv4ParamProblem, + ICMPCode: header.ICMPv4UnusedCode, + paramProblemPointer: header.IPv4MinimumSize + pointerOffsetForInvalidLength, }, { name: "bad option - length big", @@ -462,7 +502,11 @@ func TestIPv4Sanity(t *testing.T) { // space is not possible. (Second byte) 1, 2, 3, 4, }, - shouldFail: true, + shouldFail: true, + expectErrorICMP: true, + ICMPType: header.ICMPv4ParamProblem, + ICMPCode: header.ICMPv4UnusedCode, + paramProblemPointer: header.IPv4MinimumSize + pointerOffsetForInvalidLength, }, { // This tests for some linux compatible behaviour. @@ -484,7 +528,7 @@ func TestIPv4Sanity(t *testing.T) { expectErrorICMP: true, ICMPType: header.ICMPv4ParamProblem, ICMPCode: header.ICMPv4UnusedCode, - paramProblemPointer: header.IPv4MinimumSize + 2, + paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptTSPointerOffset, }, { name: "multiple type 0 with room", @@ -589,7 +633,7 @@ func TestIPv4Sanity(t *testing.T) { expectErrorICMP: true, ICMPType: header.ICMPv4ParamProblem, ICMPCode: header.ICMPv4UnusedCode, - paramProblemPointer: header.IPv4MinimumSize + 2, + paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptTSPointerOffset, }, { name: "valid timestamp pointer", @@ -624,7 +668,7 @@ func TestIPv4Sanity(t *testing.T) { expectErrorICMP: true, ICMPType: header.ICMPv4ParamProblem, ICMPCode: header.ICMPv4UnusedCode, - paramProblemPointer: header.IPv4MinimumSize + 2, + paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptTSPointerOffset, }, // End of option list with illegal option after it, which should be ignored. { @@ -636,24 +680,31 @@ func TestIPv4Sanity(t *testing.T) { 68, 12, 13, 0x11, 192, 168, 1, 12, 1, 2, 3, 4, - 0, 10, 3, 99, + 0, 10, 3, 99, // EOL followed by junk }, replyOptions: header.IPv4Options{ 68, 12, 13, 0x21, 192, 168, 1, 12, 1, 2, 3, 4, - 0, 0, 0, 0, // 3 bytes unknown option - }, // ^ End of options hides following bytes. + 0, // End of Options hides following bytes. + 0, 0, 0, // 3 bytes unknown option removed. + }, }, { - // Timestamp with a size too small. + // Timestamp with a size much too small. name: "timestamp truncated", maxTotalLength: ipv4.MaxTotalSize, transportProtocol: uint8(header.ICMPv4ProtocolNumber), TTL: ttl, - options: header.IPv4Options{68, 1, 0, 0}, - // ^ Smallest possible is 8. - shouldFail: true, + options: header.IPv4Options{ + 68, 1, 0, 0, + // ^ Smallest possible is 8. Linux points at the 68. + }, + shouldFail: true, + expectErrorICMP: true, + ICMPType: header.ICMPv4ParamProblem, + ICMPCode: header.ICMPv4UnusedCode, + paramProblemPointer: header.IPv4MinimumSize + pointerOffsetForInvalidLength, }, { name: "single record route with room", @@ -751,7 +802,7 @@ func TestIPv4Sanity(t *testing.T) { expectErrorICMP: true, ICMPType: header.ICMPv4ParamProblem, ICMPCode: header.ICMPv4UnusedCode, - paramProblemPointer: header.IPv4MinimumSize + 2, + paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptRRPointerOffset, }, { // Pointer must be 4 or more as it must point past the 3 byte header @@ -769,7 +820,7 @@ func TestIPv4Sanity(t *testing.T) { expectErrorICMP: true, ICMPType: header.ICMPv4ParamProblem, ICMPCode: header.ICMPv4UnusedCode, - paramProblemPointer: header.IPv4MinimumSize + 2, + paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptRRPointerOffset, }, { // Pointer must be 4 or more as it must point past the 3 byte header @@ -808,8 +859,7 @@ func TestIPv4Sanity(t *testing.T) { expectErrorICMP: true, ICMPType: header.ICMPv4ParamProblem, ICMPCode: header.ICMPv4UnusedCode, - paramProblemPointer: header.IPv4MinimumSize + 2, - replyOptions: header.IPv4Options{}, + paramProblemPointer: header.IPv4MinimumSize + header.IPv4OptRRPointerOffset, }, { name: "duplicate record route", @@ -828,7 +878,6 @@ func TestIPv4Sanity(t *testing.T) { ICMPType: header.ICMPv4ParamProblem, ICMPCode: header.ICMPv4UnusedCode, paramProblemPointer: header.IPv4MinimumSize + 7, - replyOptions: header.IPv4Options{}, }, } @@ -884,7 +933,6 @@ func TestIPv4Sanity(t *testing.T) { if test.maxTotalLength < totalLen { totalLen = test.maxTotalLength } - ip.Encode(&header.IPv4Fields{ TotalLength: totalLen, Protocol: test.transportProtocol, |