Skip to content

Commit c64af5e

Browse files
bradfitzbradfitz
committed
wgengine/netstack: clear TCP ECN bits before giving to gvisor
Updates tailscale#2642 Change-Id: Ic219442a2656dd9dc99ae1dd91e907fd3d924987 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
1 parent de4696d commit c64af5e

File tree

3 files changed

+134
-10
lines changed

3 files changed

+134
-10
lines changed

‎net/packet/packet.go

Lines changed: 77 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,16 @@ const minFrag = 60 + 20 // max IPv4 header + basic TCP header
2222
type TCPFlag uint8
2323

2424
const (
25-
TCPFin TCPFlag = 0x01
26-
TCPSyn TCPFlag = 0x02
27-
TCPRst TCPFlag = 0x04
28-
TCPPsh TCPFlag = 0x08
29-
TCPAck TCPFlag = 0x10
30-
TCPSynAck TCPFlag = TCPSyn | TCPAck
25+
TCPFin TCPFlag = 0x01
26+
TCPSyn TCPFlag = 0x02
27+
TCPRst TCPFlag = 0x04
28+
TCPPsh TCPFlag = 0x08
29+
TCPAck TCPFlag = 0x10
30+
TCPUrg TCPFlag = 0x20
31+
TCPECNEcho TCPFlag = 0x40
32+
TCPCWR TCPFlag = 0x80
33+
TCPSynAck TCPFlag = TCPSyn | TCPAck
34+
TCPECNBits TCPFlag = TCPECNEcho | TCPCWR
3135
)
3236

3337
// Parsed is a minimal decoding of a packet suitable for use in filters.
@@ -180,7 +184,7 @@ func (q *Parsed) decode4(b []byte) {
180184
}
181185
q.Src = q.Src.WithPort(binary.BigEndian.Uint16(sub[0:2]))
182186
q.Dst = q.Dst.WithPort(binary.BigEndian.Uint16(sub[2:4]))
183-
q.TCPFlags = TCPFlag(sub[13]) & 0x3F
187+
q.TCPFlags = TCPFlag(sub[13])
184188
headerLength := (sub[12] & 0xF0) >> 2
185189
q.dataofs = q.subofs + int(headerLength)
186190
return
@@ -282,7 +286,7 @@ func (q *Parsed) decode6(b []byte) {
282286
}
283287
q.Src = q.Src.WithPort(binary.BigEndian.Uint16(sub[0:2]))
284288
q.Dst = q.Dst.WithPort(binary.BigEndian.Uint16(sub[2:4]))
285-
q.TCPFlags = TCPFlag(sub[13]) & 0x3F
289+
q.TCPFlags = TCPFlag(sub[13])
286290
headerLength := (sub[12] & 0xF0) >> 2
287291
q.dataofs = q.subofs + int(headerLength)
288292
return
@@ -374,8 +378,14 @@ func (q *Parsed) Payload() []byte {
374378
return q.b[q.dataofs:q.length]
375379
}
376380

377-
// IsTCPSyn reports whether q is a TCP SYN packet
378-
// (i.e. the first packet in a new connection).
381+
// Transport returns the transport header and payload (IP subprotocol, such as TCP or UDP).
382+
// This is a read-only view; that is, p retains the ownership of the buffer.
383+
func (p *Parsed) Transport() []byte {
384+
return p.b[p.subofs:]
385+
}
386+
387+
// IsTCPSyn reports whether q is a TCP SYN packet,
388+
// without ACK set. (i.e. the first packet in a new connection)
379389
func (q *Parsed) IsTCPSyn() bool {
380390
return (q.TCPFlags & TCPSynAck) == TCPSyn
381391
}
@@ -424,6 +434,40 @@ func (q *Parsed) IsEchoResponse() bool {
424434
}
425435
}
426436

437+
// RemoveECNBits modifies p and its underlying memory buffer to remove
438+
// ECN bits, if any. It reports whether it did so.
439+
//
440+
// It currently only does the TCP flags.
441+
func (p *Parsed) RemoveECNBits() bool {
442+
if p.IPVersion == 0 {
443+
return false
444+
}
445+
if p.IPProto != ipproto.TCP {
446+
// TODO(bradfitz): handle non-TCP too? for now only trying to
447+
// fix the Issue 2642 problem.
448+
return false
449+
}
450+
if p.TCPFlags&TCPECNBits == 0 {
451+
// Nothing to do.
452+
return false
453+
}
454+
455+
// Clear flags.
456+
457+
// First in the parsed output.
458+
p.TCPFlags = p.TCPFlags & ^TCPECNBits
459+
460+
// Then in the underlying memory.
461+
tcp := p.Transport()
462+
old := binary.BigEndian.Uint16(tcp[12:14])
463+
tcp[13] = byte(p.TCPFlags)
464+
new := binary.BigEndian.Uint16(tcp[12:14])
465+
oldSum := binary.BigEndian.Uint16(tcp[16:18])
466+
newSum := ^checksumUpdate2ByteAlignedUint16(^oldSum, old, new)
467+
binary.BigEndian.PutUint16(tcp[16:18], newSum)
468+
return true
469+
}
470+
427471
func Hexdump(b []byte) string {
428472
out := new(strings.Builder)
429473
for i := 0; i < len(b); i += 16 {
@@ -455,3 +499,26 @@ func Hexdump(b []byte) string {
455499
}
456500
return out.String()
457501
}
502+
503+
// From gVisor's unexported API:
504+
505+
// checksumUpdate2ByteAlignedUint16 updates a uint16 value in a calculated
506+
// checksum.
507+
//
508+
// The value MUST begin at a 2-byte boundary in the original buffer.
509+
func checksumUpdate2ByteAlignedUint16(xsum, old, new uint16) uint16 {
510+
// As per RFC 1071 page 4,
511+
//(4) Incremental Update
512+
//
513+
// ...
514+
//
515+
// To update the checksum, simply add the differences of the
516+
// sixteen bit integers that have been changed. To see why this
517+
// works, observe that every 16-bit integer has an additive inverse
518+
// and that addition is associative. From this it follows that
519+
// given the original value m, the new value m', and the old
520+
// checksum C, the new checksum C' is:
521+
//
522+
// C' = C + (-m) + m' = C + (m' - m)
523+
return checksumCombine(xsum, checksumCombine(new, ^old))
524+
}

‎net/packet/packet_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ package packet
66

77
import (
88
"bytes"
9+
"encoding/hex"
910
"reflect"
11+
"regexp"
1012
"testing"
1113

1214
"inet.af/netaddr"
@@ -561,3 +563,57 @@ func BenchmarkString(b *testing.B) {
561563
})
562564
}
563565
}
566+
567+
func TestRemoveECNBits(t *testing.T) {
568+
// withECNHex is a TCP SYN packet with ECN bits set in the TCP
569+
// header as captured by Wireshark on macOS against the
570+
// Tailscale interface. In this packet (because it's a SYN
571+
// control packet), the ECN bits are not set in the IP header.
572+
const withECNHex = `45 00 00 40 00 00 40 00
573+
40 06 0c 66 64 7b 65 28 64 7f 00 30 f1 ab 00 16
574+
5a 7a 63 e8 00 00 00 00 b0 c2 ff ff 97 76 00 00
575+
02 04 04 d8 01 03 03 06 01 01 08 0a 03 e1 bd 49
576+
00 00 00 00 04 02 00 00`
577+
578+
// Generated by hand-editing a pcap file in hexl-mode to set
579+
// the TCP flags to just SYN (0x02), then loading that pcap
580+
// file in wireshark to get the expected checksum value, then
581+
// putting that checksum value (0x9836) in the file.
582+
const wantStrippedHex = `45 00 00 40 00 00 40 00
583+
40 06 0c 66 64 7b 65 28 64 7f 00 30 f1 ab 00 16
584+
5a 7a 63 e8 00 00 00 00 b0 02 ff ff 98 36 00 00
585+
02 04 04 d8 01 03 03 06 01 01 08 0a 03 e1 bd 49
586+
00 00 00 00 04 02 00 00`
587+
588+
var p Parsed
589+
pktBuf := bytesOfHex(withECNHex)
590+
p.Decode(pktBuf)
591+
if want := TCPCWR | TCPECNEcho | TCPSyn; p.TCPFlags != want {
592+
t.Fatalf("pre flags = %v; want %v", p.TCPFlags, want)
593+
}
594+
595+
if !p.RemoveECNBits() {
596+
t.Fatal("didn't remove bits")
597+
}
598+
if want := TCPSyn; p.TCPFlags != want {
599+
t.Fatalf("post flags = %v; want %v", p.TCPFlags, want)
600+
}
601+
wantPkt := bytesOfHex(wantStrippedHex)
602+
if !bytes.Equal(pktBuf, wantPkt) {
603+
t.Fatalf("wrong result.\n got: % 2x\nwant: % 2x\n", pktBuf, wantPkt)
604+
}
605+
606+
if p.RemoveECNBits() {
607+
t.Fatal("unexpected true return value on second call")
608+
}
609+
}
610+
611+
var nonHex = regexp.MustCompile(`[^0-9a-fA-F]+`)
612+
613+
func bytesOfHex(s string) []byte {
614+
b, err := hex.DecodeString(nonHex.ReplaceAllString(s, ""))
615+
if err != nil {
616+
panic(err)
617+
}
618+
return b
619+
}

‎wgengine/netstack/netstack.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -512,6 +512,7 @@ func (ns *Impl) injectInbound(p *packet.Parsed, t *tstun.Wrapper) filter.Respons
512512
case 6:
513513
pn = header.IPv6ProtocolNumber
514514
}
515+
p.RemoveECNBits() // Issue 2642
515516
if debugPackets {
516517
ns.logf("[v2] packet in (from %v): % x", p.Src, p.Buffer())
517518
}

0 commit comments

Comments
 (0)