fix(webconnectivitylte): handle measurements with loopback addrs (#1462)

This diff modifies webconnectivitylte and related packages to correctly
handle measurements containing loopback addresses. There are two cases:
(a) when both the probe and the TH only see loopback addresses at least
for the getaddrinfo lookups; (b) when only the probe sees loopback
addresses at least for the getaddrinfo lookups. In the former case, we
should mark the website as "down" (maybe a little bit of a stretch, but
if we mark down a website where the TLS is misconfigured, arguably we
can do the same when DNS is misconfigured). In the latter case, we mark
the measurement as censorship, since seeing a loopback address is not
what we expect.

Closes ooni/probe#1517.

internal/experiment/webconnectivitylte/analysisclassic.go

@@ -8,9 +8,12 @@ package webconnectivitylte
 //
 
 import (
+	"net"
+
 	"github.com/ooni/probe-cli/v3/internal/geoipx"
 	"github.com/ooni/probe-cli/v3/internal/minipipeline"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
 	"github.com/ooni/probe-cli/v3/internal/optional"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
 )
@@ -375,5 +378,52 @@ func analysisClassicComputeBlockingAccessible(woa *minipipeline.WebAnalysis, tk
 			tk.setHTTPExperimentFailure(entry.Failure)
 			return
 		}
+
+		// 6. handle the case of DNS success with the probe only seeing loopback
+		// addrs while the TH sees real addresses, which is a case where in the
+		// classic analysis (which is what we're doing) the probe does not attempt
+		// to connect to loopback addresses because it doesn't make sense.
+		if entry.Type == minipipeline.WebObservationTypeDNSLookup &&
+			!entry.Failure.IsNone() && entry.Failure.Unwrap() == "" &&
+			!entry.ControlDNSLookupFailure.IsNone() &&
+			entry.ControlDNSLookupFailure.Unwrap() == "" &&
+			!entry.DNSResolvedAddrs.IsNone() && !entry.ControlDNSResolvedAddrs.IsNone() &&
+			analysisContainsOnlyLoopbackAddrs(entry.DNSResolvedAddrs.Unwrap()) &&
+			!analysisContainsOnlyLoopbackAddrs(entry.ControlDNSResolvedAddrs.Unwrap()) {
+			tk.setBlockingString("dns")
+			return
+		}
+
+		// 7. handle the case of DNS success with loopback addrs, which is the case
+		// where neither the probe nor the TH attempt to measure endpoints.
+		if entry.Type == minipipeline.WebObservationTypeDNSLookup &&
+			!entry.Failure.IsNone() && entry.Failure.Unwrap() == "" &&
+			!entry.ControlDNSLookupFailure.IsNone() &&
+			entry.ControlDNSLookupFailure.Unwrap() == "" &&
+			!entry.DNSResolvedAddrs.IsNone() && !entry.ControlDNSResolvedAddrs.IsNone() &&
+			analysisContainsOnlyLoopbackAddrs(entry.DNSResolvedAddrs.Unwrap()) &&
+			analysisContainsOnlyLoopbackAddrs(entry.ControlDNSResolvedAddrs.Unwrap()) {
+			tk.setWebsiteDown()
+			return
+		}
+
+		// TODO(bassosimone): we should handle the case where a domain
+		// exists but there aren't IP addresses for it.
+	}
+}
+
+// analysisContainsOnlyLoopbackAddrs returns true iff the given set contains one or
+// more IP addresses and all these adresses are loopback addresses.
+func analysisContainsOnlyLoopbackAddrs(addrs minipipeline.Set[string]) bool {
+	var count int
+	for _, addr := range addrs.Keys() {
+		if net.ParseIP(addr) == nil {
+			continue
+		}
+		if !netxlite.IsLoopback(addr) {
+			return false
+		}
+		count++
 	}
+	return count > 0
 }

internal/experiment/webconnectivitylte/analysisclassic_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/apex/log"
+	"github.com/ooni/probe-cli/v3/internal/minipipeline"
 	"github.com/ooni/probe-cli/v3/internal/mocks"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
@@ -85,3 +86,46 @@ func TestTestKeys_analysisDNSToplevel(t *testing.T) {
 		})
 	}
 }
+
+func TestAnalysisClassicContainsOnlyLoopbackAddrs(t *testing.T) {
+	type testcase struct {
+		name   string
+		input  minipipeline.Set[string]
+		expect bool
+	}
+
+	cases := []testcase{{
+		name:   "with empty set",
+		input:  minipipeline.NewSet[string](),
+		expect: false,
+	}, {
+		name:   "with only loopback addrs",
+		input:  minipipeline.NewSet("127.0.0.1", "::1"),
+		expect: true,
+	}, {
+		name:   "with mixed addrs",
+		input:  minipipeline.NewSet("127.0.0.1", "130.192.91.211", "::1"),
+		expect: false,
+	}, {
+		name:   "make sure we skip non-addresses",
+		input:  minipipeline.NewSet("antani"),
+		expect: false,
+	}, {
+		name:   "make sure we say not loopback with non-addresses",
+		input:  minipipeline.NewSet("::1", "130.192.91.211", "antani", "127.0.0.1"),
+		expect: false,
+	}, {
+		name:   "make sure we say loopback with non-addresses",
+		input:  minipipeline.NewSet("::1", "antani", "127.0.0.1"),
+		expect: true,
+	}}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := analysisContainsOnlyLoopbackAddrs(tc.input)
+			if got != tc.expect {
+				t.Fatal("expected", tc.expect, "got", got)
+			}
+		})
+	}
+}

internal/experiment/webconnectivitylte/analysisext.go

@@ -64,7 +64,16 @@ func analysisExtDNS(tk *TestKeys, analysis *minipipeline.WebAnalysis, info io.Wr
 	// we're processing N >= 1 DNS lookups.
 
 	if failures := analysis.DNSLookupSuccessWithBogonAddresses; failures.Len() > 0 {
-		tk.BlockingFlags |= AnalysisBlockingFlagDNSBlocking
+		// Implementation note: uncommenting the following line IS WRONG.
+		//
+		//	tk.BlockingFlags |= AnalysisBlockingFlagDNSBlocking
+		//
+		// The reason why it is wrong is that DNS blocking depends on observing inconsistencies
+		// between the probe and the TH and it's possible to have a website misconfigured to return
+		// 127.0.0.1 to the probe, the TH, and possibly anyone else.
+		//
+		// See, for example, polito.it, which has addrs 192.168.59.6 and 192.168.40.1, as of
+		// 2024-01-24. Clearly a misconfiguration and bogons, but it can happen.
 		tk.DNSFlags |= AnalysisFlagDNSBogon
 		fmt.Fprintf(info, "- transactions with bogon IP addrs: %s\n", failures.String())
 	}

internal/experiment/webconnectivityqa/dnshijacking.go

@@ -78,3 +78,59 @@ func dnsHijackingToProxyWithHTTPSURL() *TestCase {
 		},
 	}
 }
+
+// dnsHijackingToLocalhostWithHTTP is the case where an ISP rule returns 127.0.0.1
+func dnsHijackingToLocalhostWithHTTP() *TestCase {
+	return &TestCase{
+		Name:  "dnsHijackingToLocalhostWithHTTP",
+		Flags: TestCaseFlagNoV04,
+		Input: "http://www.example.com/",
+		Configure: func(env *netemx.QAEnv) {
+
+			// add DPI rule to force all the cleartext DNS queries to
+			// point the client to use the loopback address.
+			env.DPIEngine().AddRule(&netem.DPISpoofDNSResponse{
+				Addresses: []string{"127.0.0.1"},
+				Logger:    env.Logger(),
+				Domain:    "www.example.com",
+			})
+
+		},
+		ExpectErr: false,
+		ExpectTestKeys: &testKeys{
+			DNSConsistency: "inconsistent",
+			XDNSFlags:      5,  // AnalysisFlagDNSBogon | AnalysisDNSFlagUnexpectedAddrs
+			XBlockingFlags: 33, // AnalysisBlockingFlagDNSBlocking | AnalysisBlockingFlagSuccess
+			Accessible:     false,
+			Blocking:       "dns",
+		},
+	}
+}
+
+// dnsHijackingToLocalhostWithHTTPS is the case where an ISP rule returns 127.0.0.1
+func dnsHijackingToLocalhostWithHTTPS() *TestCase {
+	return &TestCase{
+		Name:  "dnsHijackingToLocalhostWithHTTPS",
+		Flags: TestCaseFlagNoV04,
+		Input: "https://www.example.com/",
+		Configure: func(env *netemx.QAEnv) {
+
+			// add DPI rule to force all the cleartext DNS queries to
+			// point the client to use the loopback address.
+			env.DPIEngine().AddRule(&netem.DPISpoofDNSResponse{
+				Addresses: []string{"127.0.0.1"},
+				Logger:    env.Logger(),
+				Domain:    "www.example.com",
+			})
+
+		},
+		ExpectErr: false,
+		ExpectTestKeys: &testKeys{
+			DNSConsistency: "inconsistent",
+			XDNSFlags:      5,  // AnalysisFlagDNSBogon | AnalysisDNSFlagUnexpectedAddrs
+			XBlockingFlags: 33, // AnalysisBlockingFlagDNSBlocking | AnalysisBlockingFlagSuccess
+			Accessible:     false,
+			Blocking:       "dns",
+		},
+	}
+}

internal/experiment/webconnectivityqa/localhost.go

@@ -0,0 +1,49 @@
+package webconnectivityqa
+
+import "github.com/ooni/probe-cli/v3/internal/netemx"
+
+// localhostWithHTTP is the case where the website DNS is misconfigured and returns a loopback address.
+func localhostWithHTTP() *TestCase {
+	return &TestCase{
+		Name:  "localhostWithHTTP",
+		Flags: TestCaseFlagNoV04,
+		Input: "http://www.example.com/",
+		Configure: func(env *netemx.QAEnv) {
+
+			// make sure all resolvers think the correct answer is localhost
+			env.ISPResolverConfig().AddRecord("www.example.com", "", "127.0.0.1")
+			env.OtherResolversConfig().AddRecord("www.example.com", "", "127.0.0.1")
+
+		},
+		ExpectErr: false,
+		ExpectTestKeys: &testKeys{
+			DNSConsistency: "consistent",
+			XDNSFlags:      1, // AnalysisFlagDNSBogon
+			Accessible:     false,
+			Blocking:       false,
+		},
+	}
+}
+
+// localhostWithHTTPS is the case where the website DNS is misconfigured and returns a loopback address.
+func localhostWithHTTPS() *TestCase {
+	return &TestCase{
+		Name:  "localhostWithHTTPS",
+		Flags: TestCaseFlagNoV04,
+		Input: "https://www.example.com/",
+		Configure: func(env *netemx.QAEnv) {
+
+			// make sure all resolvers think the correct answer is localhost
+			env.ISPResolverConfig().AddRecord("www.example.com", "", "127.0.0.1")
+			env.OtherResolversConfig().AddRecord("www.example.com", "", "127.0.0.1")
+
+		},
+		ExpectErr: false,
+		ExpectTestKeys: &testKeys{
+			DNSConsistency: "consistent",
+			XDNSFlags:      1, // AnalysisFlagDNSBogon
+			Accessible:     false,
+			Blocking:       false,
+		},
+	}
+}

internal/experiment/webconnectivityqa/testcase.go

@@ -49,6 +49,8 @@ func AllTestCases() []*TestCase {
 		dnsBlockingBOGON(),
 		dnsBlockingNXDOMAIN(),
 
+		dnsHijackingToLocalhostWithHTTP(),
+		dnsHijackingToLocalhostWithHTTPS(),
 		dnsHijackingToProxyWithHTTPURL(),
 		dnsHijackingToProxyWithHTTPSURL(),
 
@@ -63,6 +65,9 @@ func AllTestCases() []*TestCase {
 		idnaWithoutCensorshipLowercase(),
 		idnaWithoutCensorshipWithFirstLetterUppercase(),
 
+		localhostWithHTTP(),
+		localhostWithHTTPS(),
+
 		redirectWithConsistentDNSAndThenConnectionRefusedForHTTP(),
 		redirectWithConsistentDNSAndThenConnectionRefusedForHTTPS(),
 		redirectWithConsistentDNSAndThenConnectionResetForHTTP(),

internal/minipipeline/analysis.go

@@ -285,11 +285,11 @@ func (wa *WebAnalysis) dnsComputeSuccessMetrics(
 		// determine whether the probe UNEXPECTEDLY resolved any address.
 		wa.DNSLookupSuccess.Add(obs.DNSTransactionID.Unwrap())
 
-		// if there's a bogon, mark as invalid
+		// if there's a bogon, register it and continue processing.
 		if !obs.IPAddressBogon.IsNone() && obs.IPAddressBogon.Unwrap() {
-			wa.DNSLookupSuccessWithInvalidAddresses.Add(obs.DNSTransactionID.Unwrap())
 			wa.DNSLookupSuccessWithBogonAddresses.Add(obs.DNSTransactionID.Unwrap())
-			continue
+			// fallthrough: bogons are legitimate if the website DNS is actually misconfigured
+			// so we determine bogons status and invalid addresses separately
 		}
 
 		// when there is no control info, we cannot say much

internal/minipipeline/set.go

@@ -74,12 +74,12 @@ func (sx *Set[T]) UnmarshalJSON(data []byte) error {
 }
 
 // Contains returns whether the set contains a key.
-func (sx *Set[T]) Contains(key T) bool {
+func (sx Set[T]) Contains(key T) bool {
 	_, found := sx.state[key]
 	return found
 }
 
 // String implements fmt.Stringer
-func (sx *Set[T]) String() string {
+func (sx Set[T]) String() string {
 	return fmt.Sprintf("%v", sx.Keys())
 }