Skip to content

Commit

Permalink
fix tricky case with order of DNS processing
Browse files Browse the repository at this point in the history
(I am thankful there's a ~comprehensive test suite.)
  • Loading branch information
bassosimone committed Nov 27, 2023
1 parent c6a49ff commit 8d25a65
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 12 deletions.
12 changes: 6 additions & 6 deletions internal/experiment/webconnectivityqa/dnshijacking.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,17 @@ func dnsHijackingToProxyWithHTTPURL() *TestCase {
},
ExpectErr: false,
ExpectTestKeys: &testKeys{
DNSConsistency: "inconsistent",
DNSConsistency: "consistent",
BodyLengthMatch: true,
BodyProportion: 1,
StatusCodeMatch: true,
HeadersMatch: true,
TitleMatch: true,
XStatus: 2, // StatusSuccessCleartext
XDNSFlags: 4, // AnalysisDNSUnexpectedAddrs
XBlockingFlags: 33, // AnalysisFlagDNSBlocking | analysisFlagSuccess
Accessible: false, // FIXME: this is probably incorrect!
Blocking: "dns",
XStatus: 2, // StatusSuccessCleartext
XDNSFlags: 0,
XBlockingFlags: 32, // analysisFlagSuccess
Accessible: true,
Blocking: false,
},
}
}
Expand Down
18 changes: 16 additions & 2 deletions internal/minipipeline/analysis.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,13 +203,12 @@ func (wa *WebAnalysis) ComputeDNSPossiblyInvalidAddrs(c *WebObservationsContaine

state := make(map[string]bool)

// pass 1: insert candidates into the state map
for _, obs := range c.KnownTCPEndpoints {
addr := obs.IPAddress.Unwrap()

// if we have a succesful TLS handshake for this addr, we're good
if obs.TLSHandshakeFailure.UnwrapOr("unknown_failure") == "" {
// just in case another transaction succeded, clear the address from the state
delete(state, addr)
continue
}

Expand All @@ -227,6 +226,21 @@ func (wa *WebAnalysis) ComputeDNSPossiblyInvalidAddrs(c *WebObservationsContaine
state[addr] = true
}

// pass 2: remove IP addresses we could validate using TLS handshakes
//
// we need to perform this second step because the order with which we walk
// through c.KnownTCPEndpoints is not fixed _and_ in any case, there is no
// guarantee that we'll observe 80/tcp entries _before_ 443/tcp ones. So, by
// applying this algorithm as a second step, we ensure that we're always
// able to remove TLS-validate addresses from the "bad" set.
for _, obs := range c.KnownTCPEndpoints {
addr := obs.IPAddress.Unwrap()
if obs.TLSHandshakeFailure.UnwrapOr("") != "" {
continue
}
delete(state, addr)
}

wa.DNSPossiblyInvalidAddrs = optional.Some(state)
}

Expand Down
18 changes: 14 additions & 4 deletions internal/minipipeline/observation.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,15 +368,25 @@ func (c *WebObservationsContainer) controlMatchDNSLookupResults(inputDomain stri

// walk through the list of known observations
for _, obs := range c.KnownTCPEndpoints {
// skip entries using a different domain than the one used by the TH
// obtain the domain from which we obtained the endpoint's address
domain := obs.DNSDomain.UnwrapOr("")
if domain == "" || domain != inputDomain {
continue
}

// obtain the IP address
addr := obs.IPAddress.Unwrap()

// handle the case from which the IP address has been provided by the control, which
// is a case where the domain is empty and the IP address is in thAddrMap
if domain == "" && thAddrMap[addr] {
obs.MatchWithControlIPAddress = optional.Some(true)
obs.MatchWithControlIPAddressASN = optional.Some(true)
continue
}

// skip entries using a different domain than the one used by the TH
if domain == "" || domain != inputDomain {
continue
}

// compute whether also the TH observed this addr
obs.MatchWithControlIPAddress = optional.Some(thAddrMap[addr])

Expand Down

0 comments on commit 8d25a65

Please sign in to comment.