From 7921c0f6f355994bc43cc499a9523157c0c250c8 Mon Sep 17 00:00:00 2001 From: Wade Simmons Date: Mon, 21 Aug 2023 11:03:50 -0400 Subject: [PATCH 1/3] firewall reject packets: cleanup error cases We found a few types of packets that could cause a panic here, live if a non-TCP packet has TCP header options set. These change should shore up any edge cases and ensure we reserve the maximum amount of buffer space we need to create the reject packet. --- header/header.go | 13 ++++++++ inside.go | 22 ++++++++++--- iputil/packet.go | 39 ++++++++++++++++++++--- iputil/packet_test.go | 73 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 9 deletions(-) create mode 100644 iputil/packet_test.go diff --git a/header/header.go b/header/header.go index 50b7d62b2..fb4d2a52a 100644 --- a/header/header.go +++ b/header/header.go @@ -24,6 +24,19 @@ type m map[string]interface{} const ( Version uint8 = 1 Len = 16 + + // The total Nebula packet overhead is 60 bytes: + // - HeaderLen bytes for the Nebula header. + // - 16 bytes for the encryption cipher's AEAD 128-bit tag. + // NOTE: both AESGCM and ChaChaPoly have a 16 byte tag, but if we add other + // ciphers in the future we could calculate this based on the cipher, + // returned by (cipher.AEAD).Overhead(). + // - 20 bytes for our IPv4 header. + // (max is 60 bytes, but we don't use IPv4 options) + // TODO: Could routers along the path inject a larger IPv4 header? If so, + // we may need to increase this. + // - 8 bytes for our UDP header. + NebulaOverhead = Len + 16 + 20 + 8 ) type MessageType uint8 diff --git a/inside.go b/inside.go index 0fac833a6..2e256e689 100644 --- a/inside.go +++ b/inside.go @@ -102,10 +102,24 @@ func (f *Interface) rejectOutside(packet []byte, ci *ConnectionState, hostinfo * } // Use some out buffer space to build the packet before encryption - // Need 40 bytes for the reject packet (20 byte ipv4 header, 20 byte tcp rst packet) - // Leave 100 bytes for the encrypted packet (60 byte Nebula header, 40 byte reject packet) - out = out[:140] - outPacket := iputil.CreateRejectPacket(packet, out[100:]) + const maxOutLen = iputil.MaxRejectPacketSize + header.NebulaOverhead + outPacket := iputil.CreateRejectPacket(packet, out[maxOutLen:maxOutLen+iputil.MaxRejectPacketSize]) + out = out[:maxOutLen] + + if len(outPacket) == 0 { + return + } + + if len(outPacket) > iputil.MaxRejectPacketSize { + if f.l.GetLevel() >= logrus.DebugLevel { + f.l. + WithField("packet", packet). + WithField("outPacket", outPacket). + Debug("rejectOutside: packet too big, not sending") + } + return + } + f.sendNoMetrics(header.Message, 0, ci, hostinfo, nil, outPacket, nb, out, q) } diff --git a/iputil/packet.go b/iputil/packet.go index 74ae37f09..34e669d0f 100644 --- a/iputil/packet.go +++ b/iputil/packet.go @@ -6,8 +6,19 @@ import ( "golang.org/x/net/ipv4" ) +const ( + // Need 96 bytes for the largest reject packet: + // - 20 byte ipv4 header + // - 8 byte icmpv4 header + // - 68 byte body (60 byte max orig ipv4 header + 8 byte orig icmpv4 header) + MaxRejectPacketSize = ipv4.HeaderLen + 8 + 60 + 8 +) + func CreateRejectPacket(packet []byte, out []byte) []byte { - // TODO ipv4 only, need to fix when inside supports ipv6 + if len(packet) < ipv4.HeaderLen || int(packet[0]>>4) != ipv4.Version { + return nil + } + switch packet[9] { case 6: // tcp return ipv4CreateRejectTCPPacket(packet, out) @@ -19,7 +30,16 @@ func CreateRejectPacket(packet []byte, out []byte) []byte { func ipv4CreateRejectICMPPacket(packet []byte, out []byte) []byte { ihl := int(packet[0]&0x0f) << 2 - // ICMP reply includes header and first 8 bytes of the packet + if ihl > 60 { + // invalid + return nil + } + if len(packet) < ihl { + // We need at least this many bytes for this to be a valid packet + return nil + } + + // ICMP reply includes original header and first 8 bytes of the packet packetLen := len(packet) if packetLen > ihl+8 { packetLen = ihl + 8 @@ -30,9 +50,9 @@ func ipv4CreateRejectICMPPacket(packet []byte, out []byte) []byte { out = out[:(outLen)] ipHdr := out[0:ipv4.HeaderLen] - ipHdr[0] = ipv4.Version<<4 | (ipv4.HeaderLen >> 2) // version, ihl - ipHdr[1] = 0 // DSCP, ECN - binary.BigEndian.PutUint16(ipHdr[2:], uint16(ipv4.HeaderLen+8+packetLen)) // Total Length + ipHdr[0] = ipv4.Version<<4 | (ipv4.HeaderLen >> 2) // version, ihl + ipHdr[1] = 0 // DSCP, ECN + binary.BigEndian.PutUint16(ipHdr[2:], uint16(outLen)) // Total Length ipHdr[4] = 0 // id ipHdr[5] = 0 // . @@ -76,6 +96,15 @@ func ipv4CreateRejectTCPPacket(packet []byte, out []byte) []byte { ihl := int(packet[0]&0x0f) << 2 outLen := ipv4.HeaderLen + tcpLen + if ihl > 60 { + // invalid + return nil + } + if len(packet) < ihl+tcpLen { + // We need at least this many bytes for this to be a valid packet + return nil + } + out = out[:(outLen)] ipHdr := out[0:ipv4.HeaderLen] diff --git a/iputil/packet_test.go b/iputil/packet_test.go new file mode 100644 index 000000000..e1d0d95d8 --- /dev/null +++ b/iputil/packet_test.go @@ -0,0 +1,73 @@ +package iputil + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" + "golang.org/x/net/ipv4" +) + +func Test_CreateRejectPacket(t *testing.T) { + h := ipv4.Header{ + Len: 20, + Src: net.IPv4(10, 0, 0, 1), + Dst: net.IPv4(10, 0, 0, 2), + Protocol: 1, // ICMP + } + + b, err := h.Marshal() + if err != nil { + t.Fatalf("h.Marhshal: %v", err) + } + b = append(b, []byte{0, 3, 0, 4}...) + + expectedLen := ipv4.HeaderLen + 8 + h.Len + 4 + out := make([]byte, expectedLen) + rejectPacket := CreateRejectPacket(b, out) + assert.NotNil(t, rejectPacket) + assert.Len(t, rejectPacket, expectedLen) + + // ICMP with max header len + h = ipv4.Header{ + Len: 60, + Src: net.IPv4(10, 0, 0, 1), + Dst: net.IPv4(10, 0, 0, 2), + Protocol: 1, // ICMP + Options: make([]byte, 40), + } + + b, err = h.Marshal() + if err != nil { + t.Fatalf("h.Marhshal: %v", err) + } + b = append(b, []byte{0, 3, 0, 4, 0, 0, 0, 0}...) + + expectedLen = MaxRejectPacketSize + out = make([]byte, MaxRejectPacketSize) + rejectPacket = CreateRejectPacket(b, out) + assert.NotNil(t, rejectPacket) + assert.Len(t, rejectPacket, expectedLen) + + // TCP with max header len + h = ipv4.Header{ + Len: 60, + Src: net.IPv4(10, 0, 0, 1), + Dst: net.IPv4(10, 0, 0, 2), + Protocol: 6, // TCP + Options: make([]byte, 40), + } + + b, err = h.Marshal() + if err != nil { + t.Fatalf("h.Marhshal: %v", err) + } + b = append(b, []byte{0, 3, 0, 4}...) + b = append(b, make([]byte, 16)...) + + expectedLen = ipv4.HeaderLen + 20 + out = make([]byte, expectedLen) + rejectPacket = CreateRejectPacket(b, out) + assert.NotNil(t, rejectPacket) + assert.Len(t, rejectPacket, expectedLen) +} From 7b874fc30aefebe3021e20c5a6b7ddb41b164cb0 Mon Sep 17 00:00:00 2001 From: Wade Simmons Date: Mon, 21 Aug 2023 12:24:37 -0400 Subject: [PATCH 2/3] cleanup, use correct maxOutLen --- header/header.go | 13 ------------- inside.go | 3 ++- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/header/header.go b/header/header.go index fb4d2a52a..50b7d62b2 100644 --- a/header/header.go +++ b/header/header.go @@ -24,19 +24,6 @@ type m map[string]interface{} const ( Version uint8 = 1 Len = 16 - - // The total Nebula packet overhead is 60 bytes: - // - HeaderLen bytes for the Nebula header. - // - 16 bytes for the encryption cipher's AEAD 128-bit tag. - // NOTE: both AESGCM and ChaChaPoly have a 16 byte tag, but if we add other - // ciphers in the future we could calculate this based on the cipher, - // returned by (cipher.AEAD).Overhead(). - // - 20 bytes for our IPv4 header. - // (max is 60 bytes, but we don't use IPv4 options) - // TODO: Could routers along the path inject a larger IPv4 header? If so, - // we may need to increase this. - // - 8 bytes for our UDP header. - NebulaOverhead = Len + 16 + 20 + 8 ) type MessageType uint8 diff --git a/inside.go b/inside.go index 2e256e689..a7829d69b 100644 --- a/inside.go +++ b/inside.go @@ -102,7 +102,8 @@ func (f *Interface) rejectOutside(packet []byte, ci *ConnectionState, hostinfo * } // Use some out buffer space to build the packet before encryption - const maxOutLen = iputil.MaxRejectPacketSize + header.NebulaOverhead + const aeadOverhead = 16 + const maxOutLen = iputil.MaxRejectPacketSize + header.Len + aeadOverhead outPacket := iputil.CreateRejectPacket(packet, out[maxOutLen:maxOutLen+iputil.MaxRejectPacketSize]) out = out[:maxOutLen] From 1739d8c20bba810ce1e6fd08a46d5f497fd34e5a Mon Sep 17 00:00:00 2001 From: Wade Simmons Date: Fri, 3 Nov 2023 15:05:57 -0400 Subject: [PATCH 3/3] cleanup checks and re-use packet as out buffer --- inside.go | 23 +++++++++++------------ iputil/packet.go | 18 ++++++++---------- outside.go | 4 +++- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/inside.go b/inside.go index a7829d69b..30db93952 100644 --- a/inside.go +++ b/inside.go @@ -90,6 +90,10 @@ func (f *Interface) rejectInside(packet []byte, out []byte, q int) { } out = iputil.CreateRejectPacket(packet, out) + if len(out) == 0 { + return + } + _, err := f.readers[q].Write(out) if err != nil { f.l.WithError(err).Error("Failed to write to tun") @@ -101,27 +105,22 @@ func (f *Interface) rejectOutside(packet []byte, ci *ConnectionState, hostinfo * return } - // Use some out buffer space to build the packet before encryption - const aeadOverhead = 16 - const maxOutLen = iputil.MaxRejectPacketSize + header.Len + aeadOverhead - outPacket := iputil.CreateRejectPacket(packet, out[maxOutLen:maxOutLen+iputil.MaxRejectPacketSize]) - out = out[:maxOutLen] - - if len(outPacket) == 0 { + out = iputil.CreateRejectPacket(packet, out) + if len(out) == 0 { return } - if len(outPacket) > iputil.MaxRejectPacketSize { - if f.l.GetLevel() >= logrus.DebugLevel { + if len(out) > iputil.MaxRejectPacketSize { + if f.l.GetLevel() >= logrus.InfoLevel { f.l. WithField("packet", packet). - WithField("outPacket", outPacket). - Debug("rejectOutside: packet too big, not sending") + WithField("outPacket", out). + Info("rejectOutside: packet too big, not sending") } return } - f.sendNoMetrics(header.Message, 0, ci, hostinfo, nil, outPacket, nb, out, q) + f.sendNoMetrics(header.Message, 0, ci, hostinfo, nil, out, nb, packet, q) } func (f *Interface) Handshake(vpnIp iputil.VpnIp) { diff --git a/iputil/packet.go b/iputil/packet.go index 34e669d0f..b18e52447 100644 --- a/iputil/packet.go +++ b/iputil/packet.go @@ -30,10 +30,6 @@ func CreateRejectPacket(packet []byte, out []byte) []byte { func ipv4CreateRejectICMPPacket(packet []byte, out []byte) []byte { ihl := int(packet[0]&0x0f) << 2 - if ihl > 60 { - // invalid - return nil - } if len(packet) < ihl { // We need at least this many bytes for this to be a valid packet return nil @@ -46,8 +42,11 @@ func ipv4CreateRejectICMPPacket(packet []byte, out []byte) []byte { } outLen := ipv4.HeaderLen + 8 + packetLen + if outLen > cap(out) { + return nil + } - out = out[:(outLen)] + out = out[:outLen] ipHdr := out[0:ipv4.HeaderLen] ipHdr[0] = ipv4.Version<<4 | (ipv4.HeaderLen >> 2) // version, ihl @@ -96,16 +95,15 @@ func ipv4CreateRejectTCPPacket(packet []byte, out []byte) []byte { ihl := int(packet[0]&0x0f) << 2 outLen := ipv4.HeaderLen + tcpLen - if ihl > 60 { - // invalid - return nil - } if len(packet) < ihl+tcpLen { // We need at least this many bytes for this to be a valid packet return nil } + if outLen > cap(out) { + return nil + } - out = out[:(outLen)] + out = out[:outLen] ipHdr := out[0:ipv4.HeaderLen] ipHdr[0] = ipv4.Version<<4 | (ipv4.HeaderLen >> 2) // version, ihl diff --git a/outside.go b/outside.go index a9dcdc860..1cd25afd4 100644 --- a/outside.go +++ b/outside.go @@ -406,7 +406,9 @@ func (f *Interface) decryptToTun(hostinfo *HostInfo, messageCounter uint64, out dropReason := f.firewall.Drop(out, *fwPacket, true, hostinfo, f.pki.GetCAPool(), localCache) if dropReason != nil { - f.rejectOutside(out, hostinfo.ConnectionState, hostinfo, nb, out, q) + // NOTE: We give `packet` as the `out` here since we already decrypted from it and we don't need it anymore + // This gives us a buffer to build the reject packet in + f.rejectOutside(out, hostinfo.ConnectionState, hostinfo, nb, packet, q) if f.l.Level >= logrus.DebugLevel { hostinfo.logger(f.l).WithField("fwPacket", fwPacket). WithField("reason", dropReason).