From 53bb97af3920729caac5937a26125993baa7c612 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 30 Jun 2020 14:57:11 +0200 Subject: [PATCH] whatsapp: implement remaining checks As mentioned in the commit, we see a 400 Bad Request error from WhatsApp when using the User-Agent we use for measurements along with the standard Golang's ClientHello fingerprint. This looks like MITM detection like https://mitm.watch to me. A fix for this issue could be to find out a combination of User-Agent and ClientHello that does not trigger 400 and keep the test as it should according to the spec. Yet, if there is MITM detection, it may change. This will likely cause future false positives, and we already have a bunch of such false positives for the IM tests. Also, it currently seems safe to assume that, if we can perform a TLS handshake with a certificate pool we trust, then we are talking with WhatsApp. Therefore, the status code and the returned web page matter much less than they did when we wrote the initial implementation of the WhatsApp experiment. What's more, because the HTTP request only redirects us, we should probably also simplify that check, to avoid asserting anything on the returned web page _if_ we're correctly redirected. How to properly do this will be researched in the next sprint as part of https://github.com/ooni/probe-engine/issues/740. Further investigating this issue should also be fun. This work is part of https://github.com/ooni/probe-engine/issues/55. --- experiment/whatsapp/whatsapp.go | 72 ++++++++++++++++++++-------- experiment/whatsapp/whatsapp_test.go | 62 ++++++++++++++++++++++-- 2 files changed, 111 insertions(+), 23 deletions(-) diff --git a/experiment/whatsapp/whatsapp.go b/experiment/whatsapp/whatsapp.go index 13de1d87..06efa497 100644 --- a/experiment/whatsapp/whatsapp.go +++ b/experiment/whatsapp/whatsapp.go @@ -6,6 +6,13 @@ // // This implementation does not currently perform the CIDR check, which is // know to be broken. We shall fix this issue at the spec level first. +// +// This implemention currently triggers what looks like MITM blocking by +// WhatsApp, where the combination of User-Agent header and TLS ClientHello +// we use causes a `400 Bad Request` error. We have experimentally seen we +// can avoid such error by using `miniooni/0.1.0-dev` as User-Agent. We may +// want to find out a better implementation in the future. But doing that +// is tricky as it may cause subsequent false positives down the line. package whatsapp import ( @@ -73,13 +80,13 @@ func NewTestKeys() *TestKeys { // Update updates the TestKeys using the given MultiOutput result. func (tk *TestKeys) Update(v urlgetter.MultiOutput) { - // update the easy to update entries first + // Update the easy to update entries first tk.NetworkEvents = append(tk.NetworkEvents, v.TestKeys.NetworkEvents...) tk.Queries = append(tk.Queries, v.TestKeys.Queries...) tk.Requests = append(tk.Requests, v.TestKeys.Requests...) tk.TCPConnect = append(tk.TCPConnect, v.TestKeys.TCPConnect...) tk.TLSHandshakes = append(tk.TLSHandshakes, v.TestKeys.TLSHandshakes...) - // set the status of WhatsApp endpoints + // Set the status of WhatsApp endpoints if endpointPattern.MatchString(v.Input.Target) { if v.TestKeys.Failure != nil { endpoint := strings.ReplaceAll(v.Input.Target, "tcpconnect://", "") @@ -89,23 +96,29 @@ func (tk *TestKeys) Update(v urlgetter.MultiOutput) { tk.WhatsappEndpointsStatus = "ok" return } - // set the status of the registration service + // Set the status of the registration service if v.Input.Target == RegistrationServiceURL { - // TODO(bassosimone): here we should check the HTTP status code tk.RegistrationServerFailure = v.TestKeys.Failure if v.TestKeys.Failure == nil { tk.RegistrationServerStatus = "ok" } return } - // track result of accessing the web interface - // TODO(bassosimone): here we should check the HTTP status code - // as well as the webpage contains "WhatsApp Web". + // Track result of accessing the web interface. + // + // We treat HTTPS differently. A comment above describing what looks + // like MITM detection should be enough to understand this code. switch v.Input.Target { case WebHTTPSURL: tk.WhatsappHTTPSFailure = v.TestKeys.Failure case WebHTTPURL: - tk.WhatsappHTTPFailure = v.TestKeys.Failure + failure := v.TestKeys.Failure + title := `WhatsApp Web` + if failure == nil && strings.Contains(v.TestKeys.HTTPResponseBody, title) == false { + failureString := "whatsapp_missing_title_error" + failure = &failureString + } + tk.WhatsappHTTPFailure = failure } } @@ -124,19 +137,28 @@ func (tk *TestKeys) ComputeWebStatus() { tk.WhatsappWebFailure = tk.WhatsappHTTPFailure } -type measurer struct { - config Config +// Measurer performs the measurement +type Measurer struct { + // Config contains the experiment settings. If empty we + // will be using default settings. + Config Config + + // Getter is an optional getter to be used for testing. + Getter urlgetter.MultiGetter } -func (m measurer) ExperimentName() string { +// ExperimentName implements ExperimentMeasurer.ExperimentName +func (m Measurer) ExperimentName() string { return testName } -func (m measurer) ExperimentVersion() string { +// ExperimentVersion implements ExperimentMeasurer.ExperimentVersion +func (m Measurer) ExperimentVersion() string { return testVersion } -func (m measurer) Run( +// Run implements ExperimentMeasurer.Run +func (m Measurer) Run( ctx context.Context, sess model.ExperimentSession, measurement *model.Measurement, callbacks model.ExperimentCallbacks, ) error { @@ -156,14 +178,26 @@ func (m measurer) Run( rnd.Shuffle(len(inputs), func(i, j int) { inputs[i], inputs[j] = inputs[j], inputs[i] }) - if m.config.AllEndpoints == false { + if m.Config.AllEndpoints == false { inputs = inputs[0:1] } - inputs = append(inputs, urlgetter.MultiInput{Target: RegistrationServiceURL}) - inputs = append(inputs, urlgetter.MultiInput{Target: WebHTTPSURL}) - inputs = append(inputs, urlgetter.MultiInput{Target: WebHTTPURL}) + inputs = append(inputs, urlgetter.MultiInput{ + Config: urlgetter.Config{FailOnHTTPError: true}, + Target: RegistrationServiceURL, + }) + inputs = append(inputs, urlgetter.MultiInput{ + // See the above comment regarding what seems MITM detection to + // understand why we're not forcing FailOnHTTPError here. + Target: WebHTTPSURL, + }) + inputs = append(inputs, urlgetter.MultiInput{ + // We may eventually start seeing HTTP 400 errors here. See the + // above comment on what seems MITM detection. + Config: urlgetter.Config{FailOnHTTPError: true}, + Target: WebHTTPURL, + }) // measure in parallel - multi := urlgetter.Multi{Begin: time.Now(), Session: sess} + multi := urlgetter.Multi{Begin: time.Now(), Getter: m.Getter, Session: sess} testkeys := NewTestKeys() testkeys.Agent = "redirect" measurement.TestKeys = testkeys @@ -176,5 +210,5 @@ func (m measurer) Run( // NewExperimentMeasurer creates a new ExperimentMeasurer. func NewExperimentMeasurer(config Config) model.ExperimentMeasurer { - return measurer{config: config} + return Measurer{Config: config} } diff --git a/experiment/whatsapp/whatsapp_test.go b/experiment/whatsapp/whatsapp_test.go index 1e58ec30..c4a61de1 100644 --- a/experiment/whatsapp/whatsapp_test.go +++ b/experiment/whatsapp/whatsapp_test.go @@ -8,6 +8,7 @@ import ( "github.com/apex/log" "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-engine/atomicx" "github.com/ooni/probe-engine/experiment/handler" "github.com/ooni/probe-engine/experiment/urlgetter" "github.com/ooni/probe-engine/experiment/whatsapp" @@ -231,8 +232,10 @@ func TestTestKeysOnlyEndpointsFailure(t *testing.T) { TestKeys: urlgetter.TestKeys{}, }) tk.Update(urlgetter.MultiOutput{ - Input: urlgetter.MultiInput{Target: whatsapp.WebHTTPURL}, - TestKeys: urlgetter.TestKeys{}, + Input: urlgetter.MultiInput{Target: whatsapp.WebHTTPURL}, + TestKeys: urlgetter.TestKeys{ + HTTPResponseBody: "WhatsApp Web", + }, }) tk.ComputeWebStatus() if tk.RegistrationServerFailure != nil { @@ -274,8 +277,10 @@ func TestTestKeysOnlyRegistrationServerFailure(t *testing.T) { TestKeys: urlgetter.TestKeys{}, }) tk.Update(urlgetter.MultiOutput{ - Input: urlgetter.MultiInput{Target: whatsapp.WebHTTPURL}, - TestKeys: urlgetter.TestKeys{}, + Input: urlgetter.MultiInput{Target: whatsapp.WebHTTPURL}, + TestKeys: urlgetter.TestKeys{ + HTTPResponseBody: "WhatsApp Web", + }, }) tk.ComputeWebStatus() if *tk.RegistrationServerFailure != failure { @@ -343,3 +348,52 @@ func TestTestKeysOnlyWebFailure(t *testing.T) { t.Fatal("invalid WhatsappWebStatus") } } + +func TestWeConfigureWebChecksToFailOnHTTPError(t *testing.T) { + called := atomicx.NewInt64() + failOnErrorWebHTTPS := atomicx.NewInt64() + failOnErrorWebHTTP := atomicx.NewInt64() + failOnErrorRegistrationService := atomicx.NewInt64() + measurer := whatsapp.Measurer{ + Config: whatsapp.Config{}, + Getter: func(ctx context.Context, g urlgetter.Getter) (urlgetter.TestKeys, error) { + called.Add(1) + switch g.Target { + case whatsapp.WebHTTPSURL: + if g.Config.FailOnHTTPError { + failOnErrorWebHTTPS.Add(1) + } + case whatsapp.WebHTTPURL: + if g.Config.FailOnHTTPError { + failOnErrorWebHTTP.Add(1) + } + case whatsapp.RegistrationServiceURL: + if g.Config.FailOnHTTPError { + failOnErrorRegistrationService.Add(1) + } + } + return urlgetter.DefaultMultiGetter(ctx, g) + }, + } + ctx := context.Background() + sess := &mockable.ExperimentSession{ + MockableLogger: log.Log, + } + measurement := new(model.Measurement) + callbacks := handler.NewPrinterCallbacks(log.Log) + if err := measurer.Run(ctx, sess, measurement, callbacks); err != nil { + t.Fatal(err) + } + if called.Load() < 1 { + t.Fatal("not called") + } + if failOnErrorWebHTTPS.Load() != 0 { + t.Fatal("configured fail on error for Web HTTPS") + } + if failOnErrorWebHTTP.Load() != 1 { + t.Fatal("not configured fail on error for Web HTTP") + } + if failOnErrorRegistrationService.Load() != 1 { + t.Fatal("not configured fail on error for registragtion service") + } +}