Skip to content

Commit 95d98b1

Browse files
JackDoanJackDoan
authored
firewall: move conntrack check after cert+IP verification (#1779)
1 parent 6afca0f commit 95d98b1

2 files changed

Lines changed: 158 additions & 5 deletions

File tree

‎firewall.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -423,11 +423,6 @@ var ErrNoMatchingRule = errors.New("no matching rule in firewall table")
423423
// Drop returns an error if the packet should be dropped, explaining why. It
424424
// returns nil if the packet should not be dropped.
425425
func (f *Firewall) Drop(fp firewall.Packet, incoming bool, h *HostInfo, caPool *cert.CAPool, localCache firewall.ConntrackCache) error {
426-
// Check if we spoke to this tuple, if we did then allow this packet
427-
if f.inConns(fp, h, caPool, localCache) {
428-
return nil
429-
}
430-
431426
// Make sure remote address matches nebula certificate, and determine how to treat it
432427
if h.networks == nil {
433428
// Simple case: Certificate has one address and no unsafe networks
@@ -461,6 +456,11 @@ func (f *Firewall) Drop(fp firewall.Packet, incoming bool, h *HostInfo, caPool *
461456
return ErrInvalidLocalIP
462457
}
463458

459+
// Check if we spoke to this tuple, if we did then allow this packet
460+
if f.inConns(fp, h, caPool, localCache) {
461+
return nil
462+
}
463+
464464
table := f.OutRules
465465
if incoming {
466466
table = f.InRules

‎firewall_test.go‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -916,6 +916,159 @@ func TestFirewall_DropIPSpoofing(t *testing.T) {
916916
assert.Equal(t, fw.Drop(p, true, &h1, cp, nil), ErrInvalidRemoteIP)
917917
}
918918

919+
func TestFirewall_ConntrackSourceSpoofingAcrossPeers(t *testing.T) {
920+
l := test.NewLoggerWithOutput(&bytes.Buffer{})
921+
922+
myVpnNetworksTable := new(bart.Lite)
923+
myVpnNetworksTable.Insert(netip.MustParsePrefix("192.0.2.1/24"))
924+
925+
owner := &dummyCert{
926+
name: "owner",
927+
networks: []netip.Prefix{netip.MustParsePrefix("192.0.2.1/24")},
928+
}
929+
930+
victim := &cert.CachedCertificate{
931+
Certificate: &dummyCert{
932+
name: "victim",
933+
networks: []netip.Prefix{netip.MustParsePrefix("192.0.2.2/24")},
934+
},
935+
}
936+
victimHI := HostInfo{
937+
ConnectionState: &ConnectionState{peerCert: victim},
938+
vpnAddrs: []netip.Addr{netip.MustParseAddr("192.0.2.2")},
939+
}
940+
victimHI.buildNetworks(myVpnNetworksTable, victim.Certificate)
941+
942+
attacker := &cert.CachedCertificate{
943+
Certificate: &dummyCert{
944+
name: "attacker",
945+
networks: []netip.Prefix{netip.MustParsePrefix("192.0.2.3/24")},
946+
},
947+
}
948+
attackerHI := HostInfo{
949+
ConnectionState: &ConnectionState{peerCert: attacker},
950+
vpnAddrs: []netip.Addr{netip.MustParseAddr("192.0.2.3")},
951+
}
952+
attackerHI.buildNetworks(myVpnNetworksTable, attacker.Certificate)
953+
954+
fw := NewFirewall(l, time.Second, time.Minute, time.Hour, owner)
955+
// Allow any inbound traffic that passes the cert / source-IP checks.
956+
require.NoError(t, fw.AddRule(true, firewall.ProtoAny, 0, 0, []string{"any"}, "", "", "", "", ""))
957+
cp := cert.NewCAPool()
958+
959+
flow := firewall.Packet{
960+
LocalAddr: netip.MustParseAddr("192.0.2.1"),
961+
RemoteAddr: netip.MustParseAddr("192.0.2.2"),
962+
LocalPort: 443,
963+
RemotePort: 55000,
964+
Protocol: firewall.ProtoUDP,
965+
}
966+
967+
require.NoError(t, fw.Drop(flow, true, &victimHI, cp, nil),
968+
"victim's own traffic from its own overlay IP must be allowed")
969+
970+
unseen := flow
971+
unseen.RemotePort = 55001
972+
assert.Equal(t, ErrInvalidRemoteIP, fw.Drop(unseen, true, &attackerHI, cp, nil),
973+
"sanity: attacker forging victim's source IP must be rejected when no conntrack entry exists")
974+
975+
got := fw.Drop(flow, true, &attackerHI, cp, nil)
976+
t.Logf("attacker replaying victim's 4-tuple: Drop returned %v (nil == packet ALLOWED == spoof succeeded)", got)
977+
assert.Equal(t, ErrInvalidRemoteIP, got,
978+
"SECURITY: attacker spoofed victim's overlay source IP (192.0.2.2) by reusing an existing conntrack 4-tuple; Drop returned %v instead of rejecting", got)
979+
}
980+
981+
// BenchmarkFirewallDropConntrackHit measures Drop on an already-established flow
982+
// (a conntrack hit). This is the fast path that the source-IP<->cert binding
983+
// reordering adds work to, so it quantifies the cost of moving the address checks
984+
// ahead of the conntrack lookup. Cases:
985+
// - simple: peer cert has one address, no unsafe networks (h.networks == nil),
986+
// so the remote-address check is a single netip.Addr compare.
987+
// - complex: peer cert has unsafe networks (h.networks populated), so the
988+
// remote-address check is a BART lookup.
989+
// - noCache/localCache: whether a per-batch ConntrackCache is supplied, which in
990+
// the original code let the fast path skip straight past the address checks.
991+
func BenchmarkFirewallDropConntrackHit(b *testing.B) {
992+
l := test.NewLoggerWithOutput(&bytes.Buffer{})
993+
994+
myVpnNetworksTable := new(bart.Lite)
995+
myVpnNetworksTable.Insert(netip.MustParsePrefix("192.0.2.1/24"))
996+
997+
owner := &dummyCert{
998+
name: "owner",
999+
networks: []netip.Prefix{netip.MustParsePrefix("192.0.2.1/24")},
1000+
}
1001+
1002+
simpleCert := &cert.CachedCertificate{
1003+
Certificate: &dummyCert{
1004+
name: "simple",
1005+
networks: []netip.Prefix{netip.MustParsePrefix("192.0.2.2/24")},
1006+
},
1007+
}
1008+
simpleHost := &HostInfo{
1009+
ConnectionState: &ConnectionState{peerCert: simpleCert},
1010+
vpnAddrs: []netip.Addr{netip.MustParseAddr("192.0.2.2")},
1011+
}
1012+
simpleHost.buildNetworks(myVpnNetworksTable, simpleCert.Certificate)
1013+
1014+
complexCert := &cert.CachedCertificate{
1015+
Certificate: &dummyCert{
1016+
name: "complex",
1017+
networks: []netip.Prefix{netip.MustParsePrefix("192.0.2.2/24")},
1018+
unsafeNetworks: []netip.Prefix{netip.MustParsePrefix("198.51.100.0/24")},
1019+
},
1020+
}
1021+
complexHost := &HostInfo{
1022+
ConnectionState: &ConnectionState{peerCert: complexCert},
1023+
vpnAddrs: []netip.Addr{netip.MustParseAddr("192.0.2.2")},
1024+
}
1025+
complexHost.buildNetworks(myVpnNetworksTable, complexCert.Certificate)
1026+
1027+
cp := cert.NewCAPool()
1028+
1029+
flow := firewall.Packet{
1030+
LocalAddr: netip.MustParseAddr("192.0.2.1"),
1031+
RemoteAddr: netip.MustParseAddr("192.0.2.2"),
1032+
LocalPort: 443,
1033+
RemotePort: 55000,
1034+
Protocol: firewall.ProtoUDP,
1035+
}
1036+
1037+
cases := []struct {
1038+
name string
1039+
host *HostInfo
1040+
useCache bool
1041+
}{
1042+
{"simple/noCache", simpleHost, false},
1043+
{"simple/localCache", simpleHost, true},
1044+
{"complex/noCache", complexHost, false},
1045+
{"complex/localCache", complexHost, true},
1046+
}
1047+
1048+
for _, tc := range cases {
1049+
b.Run(tc.name, func(b *testing.B) {
1050+
fw := NewFirewall(l, time.Second, time.Minute, time.Hour, owner)
1051+
require.NoError(b, fw.AddRule(true, firewall.ProtoAny, 0, 0, []string{"any"}, "", "", "", "", ""))
1052+
1053+
// Establish the conntrack entry so every benchmarked Drop is a hit.
1054+
require.NoError(b, fw.Drop(flow, true, tc.host, cp, nil))
1055+
1056+
var cache firewall.ConntrackCache
1057+
if tc.useCache {
1058+
cache = firewall.ConntrackCache{}
1059+
}
1060+
1061+
b.ReportAllocs()
1062+
b.ResetTimer()
1063+
for i := 0; i < b.N; i++ {
1064+
if err := fw.Drop(flow, true, tc.host, cp, cache); err != nil {
1065+
b.Fatal(err)
1066+
}
1067+
}
1068+
})
1069+
}
1070+
}
1071+
9191072
func BenchmarkLookup(b *testing.B) {
9201073
ml := func(m map[string]struct{}, a [][]string) {
9211074
for n := 0; n < b.N; n++ {

0 commit comments

Comments
 (0)