From de73486d2155f549cd7ae76eb275b48bbe05492f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=96=E7=95=8C?= Date: Tue, 24 Mar 2026 22:26:20 +0800 Subject: [PATCH] Fix DNS pre-match CIDR fail-closed semantics --- adapter/inbound.go | 11 +- dns/_repro_test.go | 121 ---------------------- dns/repro_test.go | 142 ++++++++++++++++++++++++++ route/rule/rule_abstract.go | 8 +- route/rule/rule_dns.go | 1 - route/rule/rule_set_semantics_test.go | 49 ++++++++- 6 files changed, 194 insertions(+), 138 deletions(-) delete mode 100644 dns/_repro_test.go create mode 100644 dns/repro_test.go diff --git a/adapter/inbound.go b/adapter/inbound.go index 048699f6d..5bc147436 100644 --- a/adapter/inbound.go +++ b/adapter/inbound.go @@ -99,12 +99,11 @@ type InboundContext struct { IPCIDRMatchSource bool IPCIDRAcceptEmpty bool - SourceAddressMatch bool - SourcePortMatch bool - DestinationAddressMatch bool - DestinationPortMatch bool - DidMatch bool - IgnoreDestinationIPCIDRMatch bool + SourceAddressMatch bool + SourcePortMatch bool + DestinationAddressMatch bool + DestinationPortMatch bool + DidMatch bool } func (c *InboundContext) ResetRuleCache() { diff --git a/dns/_repro_test.go b/dns/_repro_test.go deleted file mode 100644 index 467d0cb24..000000000 --- a/dns/_repro_test.go +++ /dev/null @@ -1,121 +0,0 @@ -package dns - -import ( - "context" - "errors" - "net/netip" - "testing" - - "github.com/sagernet/sing-box/adapter" - C "github.com/sagernet/sing-box/constant" - "github.com/sagernet/sing-box/option" - "github.com/sagernet/sing/common/json/badoption" - mDNS "github.com/miekg/dns" - "github.com/stretchr/testify/require" -) - -func TestReproLookupWithRulesIgnoresRouteStrategy(t *testing.T) { - defaultTransport := &fakeDNSTransport{tag: "default", transportType: C.DNSTypeUDP} - router := newTestRouter(t, []option.DNSRule{ - { - Type: C.RuleTypeDefault, - DefaultOptions: option.DefaultDNSRule{ - RawDefaultDNSRule: option.RawDefaultDNSRule{ - Domain: badoption.Listable[string]{"example.com"}, - }, - DNSRuleAction: option.DNSRuleAction{ - Action: C.RuleActionTypeEvaluate, - RouteOptions: option.DNSRouteActionOptions{Server: "default"}, - }, - }, - }, - { - Type: C.RuleTypeDefault, - DefaultOptions: option.DefaultDNSRule{ - RawDefaultDNSRule: option.RawDefaultDNSRule{ - MatchResponse: true, - }, - DNSRuleAction: option.DNSRuleAction{ - Action: C.RuleActionTypeRoute, - RouteOptions: option.DNSRouteActionOptions{Server: "selected", Strategy: C.DomainStrategyIPv4Only}, - }, - }, - }, - }, &fakeDNSTransportManager{ - defaultTransport: defaultTransport, - transports: map[string]adapter.DNSTransport{ - "default": defaultTransport, - "selected": &fakeDNSTransport{tag: "selected", transportType: C.DNSTypeUDP}, - }, - }, &fakeDNSClient{ - exchange: func(transport adapter.DNSTransport, message *mDNS.Msg) (*mDNS.Msg, error) { - if transport.Tag() == "default" { - return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("1.1.1.1")}, 60), nil - } - switch message.Question[0].Qtype { - case mDNS.TypeA: - return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("2.2.2.2")}, 60), nil - case mDNS.TypeAAAA: - return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("2001:db8::1")}, 60), nil - default: - return nil, errors.New("unexpected qtype") - } - }, - }) - - addrs, err := router.Lookup(context.Background(), "example.com", adapter.DNSQueryOptions{}) - require.NoError(t, err) - require.Equal(t, []netip.Addr{netip.MustParseAddr("2.2.2.2")}, addrs) -} - -func TestReproLogicalMatchResponseIPCIDR(t *testing.T) { - transportManager := &fakeDNSTransportManager{ - defaultTransport: &fakeDNSTransport{tag: "default", transportType: C.DNSTypeUDP}, - transports: map[string]adapter.DNSTransport{ - "upstream": &fakeDNSTransport{tag: "upstream", transportType: C.DNSTypeUDP}, - "selected": &fakeDNSTransport{tag: "selected", transportType: C.DNSTypeUDP}, - "default": &fakeDNSTransport{tag: "default", transportType: C.DNSTypeUDP}, - }, - } - client := &fakeDNSClient{ - exchange: func(transport adapter.DNSTransport, message *mDNS.Msg) (*mDNS.Msg, error) { - switch transport.Tag() { - case "upstream": - return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("1.1.1.1")}, 60), nil - case "selected": - return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("8.8.8.8")}, 60), nil - default: - return nil, errors.New("unexpected transport") - } - }, - } - rules := []option.DNSRule{ - { - Type: C.RuleTypeDefault, - DefaultOptions: option.DefaultDNSRule{ - RawDefaultDNSRule: option.RawDefaultDNSRule{Domain: badoption.Listable[string]{"example.com"}}, - DNSRuleAction: option.DNSRuleAction{Action: C.RuleActionTypeEvaluate, RouteOptions: option.DNSRouteActionOptions{Server: "upstream"}}, - }, - }, - { - Type: C.RuleTypeLogical, - LogicalOptions: option.LogicalDNSRule{ - RawLogicalDNSRule: option.RawLogicalDNSRule{ - Mode: C.LogicalTypeOr, - Rules: []option.DNSRule{{ - Type: C.RuleTypeDefault, - DefaultOptions: option.DefaultDNSRule{ - RawDefaultDNSRule: option.RawDefaultDNSRule{MatchResponse: true, IPCIDR: badoption.Listable[string]{"1.1.1.0/24"}}, - }, - }}, - }, - DNSRuleAction: option.DNSRuleAction{Action: C.RuleActionTypeRoute, RouteOptions: option.DNSRouteActionOptions{Server: "selected"}}, - }, - }, - } - router := newTestRouter(t, rules, transportManager, client) - - response, err := router.Exchange(context.Background(), &mDNS.Msg{Question: []mDNS.Question{fixedQuestion("example.com", mDNS.TypeA)}}, adapter.DNSQueryOptions{}) - require.NoError(t, err) - require.Equal(t, []netip.Addr{netip.MustParseAddr("8.8.8.8")}, MessageToAddresses(response)) -} diff --git a/dns/repro_test.go b/dns/repro_test.go new file mode 100644 index 000000000..2569a58d7 --- /dev/null +++ b/dns/repro_test.go @@ -0,0 +1,142 @@ +package dns + +import ( + "context" + "errors" + "net/netip" + "testing" + + "github.com/sagernet/sing-box/adapter" + C "github.com/sagernet/sing-box/constant" + "github.com/sagernet/sing-box/option" + "github.com/sagernet/sing/common/json/badoption" + + mDNS "github.com/miekg/dns" + "github.com/stretchr/testify/require" +) + +func TestReproLookupWithRulesIgnoresRouteStrategy(t *testing.T) { + t.Parallel() + + defaultTransport := &fakeDNSTransport{tag: "default", transportType: C.DNSTypeUDP} + router := newTestRouter(t, []option.DNSRule{ + { + Type: C.RuleTypeDefault, + DefaultOptions: option.DefaultDNSRule{ + RawDefaultDNSRule: option.RawDefaultDNSRule{ + Domain: badoption.Listable[string]{"example.com"}, + }, + DNSRuleAction: option.DNSRuleAction{ + Action: C.RuleActionTypeEvaluate, + RouteOptions: option.DNSRouteActionOptions{Server: "default"}, + }, + }, + }, + { + Type: C.RuleTypeDefault, + DefaultOptions: option.DefaultDNSRule{ + RawDefaultDNSRule: option.RawDefaultDNSRule{ + MatchResponse: true, + }, + DNSRuleAction: option.DNSRuleAction{ + Action: C.RuleActionTypeRoute, + RouteOptions: option.DNSRouteActionOptions{ + Server: "selected", + Strategy: option.DomainStrategy(C.DomainStrategyIPv4Only), + }, + }, + }, + }, + }, &fakeDNSTransportManager{ + defaultTransport: defaultTransport, + transports: map[string]adapter.DNSTransport{ + "default": defaultTransport, + "selected": &fakeDNSTransport{tag: "selected", transportType: C.DNSTypeUDP}, + }, + }, &fakeDNSClient{ + exchange: func(transport adapter.DNSTransport, message *mDNS.Msg) (*mDNS.Msg, error) { + if transport.Tag() == "default" { + return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("1.1.1.1")}, 60), nil + } + switch message.Question[0].Qtype { + case mDNS.TypeA: + return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("2.2.2.2")}, 60), nil + case mDNS.TypeAAAA: + return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("2001:db8::1")}, 60), nil + default: + return nil, errors.New("unexpected qtype") + } + }, + }) + + addrs, err := router.Lookup(context.Background(), "example.com", adapter.DNSQueryOptions{}) + require.NoError(t, err) + require.Equal(t, []netip.Addr{netip.MustParseAddr("2.2.2.2")}, addrs) +} + +func TestReproLogicalMatchResponseIPCIDR(t *testing.T) { + t.Parallel() + + transportManager := &fakeDNSTransportManager{ + defaultTransport: &fakeDNSTransport{tag: "default", transportType: C.DNSTypeUDP}, + transports: map[string]adapter.DNSTransport{ + "upstream": &fakeDNSTransport{tag: "upstream", transportType: C.DNSTypeUDP}, + "selected": &fakeDNSTransport{tag: "selected", transportType: C.DNSTypeUDP}, + "default": &fakeDNSTransport{tag: "default", transportType: C.DNSTypeUDP}, + }, + } + client := &fakeDNSClient{ + exchange: func(transport adapter.DNSTransport, message *mDNS.Msg) (*mDNS.Msg, error) { + switch transport.Tag() { + case "upstream": + return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("1.1.1.1")}, 60), nil + case "selected": + return FixedResponse(0, message.Question[0], []netip.Addr{netip.MustParseAddr("8.8.8.8")}, 60), nil + default: + return nil, errors.New("unexpected transport") + } + }, + } + rules := []option.DNSRule{ + { + Type: C.RuleTypeDefault, + DefaultOptions: option.DefaultDNSRule{ + RawDefaultDNSRule: option.RawDefaultDNSRule{ + Domain: badoption.Listable[string]{"example.com"}, + }, + DNSRuleAction: option.DNSRuleAction{ + Action: C.RuleActionTypeEvaluate, + RouteOptions: option.DNSRouteActionOptions{Server: "upstream"}, + }, + }, + }, + { + Type: C.RuleTypeLogical, + LogicalOptions: option.LogicalDNSRule{ + RawLogicalDNSRule: option.RawLogicalDNSRule{ + Mode: C.LogicalTypeOr, + Rules: []option.DNSRule{{ + Type: C.RuleTypeDefault, + DefaultOptions: option.DefaultDNSRule{ + RawDefaultDNSRule: option.RawDefaultDNSRule{ + MatchResponse: true, + IPCIDR: badoption.Listable[string]{"1.1.1.0/24"}, + }, + }, + }}, + }, + DNSRuleAction: option.DNSRuleAction{ + Action: C.RuleActionTypeRoute, + RouteOptions: option.DNSRouteActionOptions{Server: "selected"}, + }, + }, + }, + } + router := newTestRouter(t, rules, transportManager, client) + + response, err := router.Exchange(context.Background(), &mDNS.Msg{ + Question: []mDNS.Question{fixedQuestion("example.com", mDNS.TypeA)}, + }, adapter.DNSQueryOptions{}) + require.NoError(t, err) + require.Equal(t, []netip.Addr{netip.MustParseAddr("8.8.8.8")}, MessageToAddresses(response)) +} diff --git a/route/rule/rule_abstract.go b/route/rule/rule_abstract.go index 8a95fa6d2..8ec57aac3 100644 --- a/route/rule/rule_abstract.go +++ b/route/rule/rule_abstract.go @@ -56,11 +56,11 @@ func (r *abstractDefaultRule) Match(metadata *adapter.InboundContext) bool { } func (r *abstractDefaultRule) destinationIPCIDRMatchesSource(metadata *adapter.InboundContext) bool { - return !metadata.IgnoreDestinationIPCIDRMatch && metadata.IPCIDRMatchSource && len(r.destinationIPCIDRItems) > 0 + return metadata.IPCIDRMatchSource && len(r.destinationIPCIDRItems) > 0 } func (r *abstractDefaultRule) destinationIPCIDRMatchesDestination(metadata *adapter.InboundContext) bool { - return !metadata.IgnoreDestinationIPCIDRMatch && !metadata.IPCIDRMatchSource && len(r.destinationIPCIDRItems) > 0 + return !metadata.IPCIDRMatchSource && len(r.destinationIPCIDRItems) > 0 } func (r *abstractDefaultRule) requiresSourceAddressMatch(metadata *adapter.InboundContext) bool { @@ -156,10 +156,6 @@ func (r *abstractDefaultRule) matchStatesWithBase(metadata *adapter.InboundConte return r.invertedFailure(inheritedBase) } if r.invert { - // DNS pre-lookup defers destination address-limit checks until the response phase. - if metadata.IgnoreDestinationIPCIDRMatch && stateSet == emptyRuleMatchState() && !metadata.DidMatch && len(r.destinationIPCIDRItems) > 0 { - return emptyRuleMatchState().withBase(inheritedBase) - } return 0 } return stateSet diff --git a/route/rule/rule_dns.go b/route/rule/rule_dns.go index 545d88f60..f535844a7 100644 --- a/route/rule/rule_dns.go +++ b/route/rule/rule_dns.go @@ -365,7 +365,6 @@ func (r *DefaultDNSRule) matchStatesForMatch(metadata *adapter.InboundContext) r return r.abstractDefaultRule.matchStates(&matchMetadata) } matchMetadata := *metadata - matchMetadata.IgnoreDestinationIPCIDRMatch = true return r.abstractDefaultRule.matchStates(&matchMetadata) } diff --git a/route/rule/rule_set_semantics_test.go b/route/rule/rule_set_semantics_test.go index 553c2bf3e..d2a865bb3 100644 --- a/route/rule/rule_set_semantics_test.go +++ b/route/rule/rule_set_semantics_test.go @@ -616,6 +616,47 @@ func TestDNSRuleSetSemantics(t *testing.T) { require.False(t, metadata.IPCIDRMatchSource) require.False(t, metadata.IPCIDRAcceptEmpty) }) + t.Run("pre lookup ruleset only deferred fields fail closed", func(t *testing.T) { + t.Parallel() + metadata := testMetadata("lookup.example") + ruleSet := newLocalRuleSetForTest("dns-prelookup-ipcidr", headlessDefaultRule(t, func(rule *abstractDefaultRule) { + addDestinationIPCIDRItem(t, rule, []string{"203.0.113.0/24"}) + })) + rule := dnsRuleForTest(func(rule *abstractDefaultRule) { + addRuleSetItem(rule, &RuleSetItem{setList: []adapter.RuleSet{ruleSet}}) + }) + require.False(t, rule.Match(&metadata)) + }) + t.Run("pre lookup ruleset destination cidr does not fall back to other predicates", func(t *testing.T) { + t.Parallel() + metadata := testMetadata("lookup.example") + ruleSet := newLocalRuleSetForTest("dns-prelookup-network-and-ipcidr", headlessDefaultRule(t, func(rule *abstractDefaultRule) { + addOtherItem(rule, NewNetworkItem([]string{N.NetworkTCP})) + addDestinationIPCIDRItem(t, rule, []string{"203.0.113.0/24"}) + })) + rule := dnsRuleForTest(func(rule *abstractDefaultRule) { + addRuleSetItem(rule, &RuleSetItem{setList: []adapter.RuleSet{ruleSet}}) + }) + require.False(t, rule.Match(&metadata)) + }) + t.Run("pre lookup mixed ruleset still matches non response branch", func(t *testing.T) { + t.Parallel() + metadata := testMetadata("www.example.com") + ruleSet := newLocalRuleSetForTest( + "dns-prelookup-mixed", + headlessDefaultRule(t, func(rule *abstractDefaultRule) { + addOtherItem(rule, NewNetworkItem([]string{N.NetworkTCP})) + addDestinationIPCIDRItem(t, rule, []string{"203.0.113.0/24"}) + }), + headlessDefaultRule(t, func(rule *abstractDefaultRule) { + addDestinationAddressItem(t, rule, nil, []string{"example.com"}) + }), + ) + rule := dnsRuleForTest(func(rule *abstractDefaultRule) { + addRuleSetItem(rule, &RuleSetItem{setList: []adapter.RuleSet{ruleSet}}) + }) + require.True(t, rule.Match(&metadata)) + }) } func TestDNSMatchResponseRuleSetDestinationCIDRUsesDNSResponse(t *testing.T) { @@ -751,7 +792,7 @@ func TestDNSInvertAddressLimitPreLookupRegression(t *testing.T) { require.True(t, rule.MatchAddressLimit(&unmatchedMetadata, dnsResponseForTest(testCase.unmatchedAddrs...))) }) } - t.Run("mixed resolved and deferred fields keep old pre lookup false", func(t *testing.T) { + t.Run("mixed resolved and deferred fields invert matches pre lookup", func(t *testing.T) { t.Parallel() metadata := testMetadata("lookup.example") rule := dnsRuleForTest(func(rule *abstractDefaultRule) { @@ -759,9 +800,9 @@ func TestDNSInvertAddressLimitPreLookupRegression(t *testing.T) { addOtherItem(rule, NewNetworkItem([]string{N.NetworkTCP})) addDestinationIPCIDRItem(t, rule, []string{"203.0.113.0/24"}) }) - require.False(t, rule.Match(&metadata)) + require.True(t, rule.Match(&metadata)) }) - t.Run("ruleset only deferred fields keep old pre lookup false", func(t *testing.T) { + t.Run("ruleset only deferred fields invert matches pre lookup", func(t *testing.T) { t.Parallel() metadata := testMetadata("lookup.example") ruleSet := newLocalRuleSetForTest("dns-ruleset-ipcidr", headlessDefaultRule(t, func(rule *abstractDefaultRule) { @@ -771,7 +812,7 @@ func TestDNSInvertAddressLimitPreLookupRegression(t *testing.T) { rule.invert = true addRuleSetItem(rule, &RuleSetItem{setList: []adapter.RuleSet{ruleSet}}) }) - require.False(t, rule.Match(&metadata)) + require.True(t, rule.Match(&metadata)) }) }