Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WhatsApp false positives due to obsolete CIDR #341

Closed
bassosimone opened this issue Feb 17, 2020 · 11 comments
Closed

WhatsApp false positives due to obsolete CIDR #341

bassosimone opened this issue Feb 17, 2020 · 11 comments
Assignees
@bassosimone
Labels
bug Something isn't working discuss We need to have a conversation effort/XS Extra small effort priority/high High priority
Milestone
Sprint 17 - Θέτις

Comments

@bassosimone
Copy link
Contributor

See https://explorer.ooni.org/measurement/20200216T230231Z_AS11427_2QcLCBIJStNE89Fd8mRD0YNoRAxy5wDNHhUSVJjenmkJ0sEVGZ

Reported by @agrabeli and @lorenzoPrimi
@bassosimone bassosimone added the bug Something isn't working label Feb 17, 2020
@bassosimone bassosimone self-assigned this Feb 17, 2020
@hellais hellais added the priority/high High priority label Feb 19, 2020
@hellais
Copy link
Member

hellais commented Feb 19, 2020

Here is another sample measurement which shows the false positive: https://explorer.ooni.org/measurement/20200218T201823Z_AS24757_5zgq4DYv6ady7ZcZOq9JSngkhY49Tj6hIHd81WuM44U6GKB8L7.

It seems that in both these cases what is triggering the anomaly is whatsapp_endpoints_dns_inconsistent, which is happening because we can't attribute the resolved IP addresses for the whatsapp endpoints to WhatsApp itself.

This is most likely being caused by the fact that the CIDR list at this point is very stale: https://github.com/measurement-kit/measurement-kit/blob/62d477f085f767bc8235f04ad08dedd32a03afc6/src/libmeasurement_kit/ooni/whatsapp.cpp#L26 and WhatsApp has stopped updating it with fresh data.

I did a whois on the resolved addresses in both of those test results and I see that they both are mapping to AWS.

Maybe a viable hotfix for this problem could be that of checking if the resolved IPs match to AWS infrastructure.

Alternatively we could disable the DNS consistency check altogether and favour not showing false positives in the app and rather defer analysis to a post-processing stage.

@bassosimone
Copy link
Contributor Author

@hellais thanks for researching this. I think we should not release the mobile app until this is fixed in MK (cc: @lorenzoPrimi).

@bassosimone
Copy link
Contributor Author

Alternatively we could disable the DNS consistency check altogether and favour not showing false positives in the app and rather defer analysis to a post-processing stage.

This strikes me as a good hotfix. Will implement it as soon as I am in front of my computer again 🙄.

@bassosimone bassosimone added the interrupt Task not planned during planning label Feb 20, 2020
@bassosimone bassosimone changed the title Investigate WhatsApp false positives Investigate and remediate WhatsApp false positives Feb 20, 2020
@bassosimone bassosimone changed the title Investigate and remediate WhatsApp false positives WhatsApp false positives due to obsolete CIDR Feb 20, 2020
This was referenced Feb 20, 2020
Merged
Merged
FedericoCeratto pushed a commit to ooni/pipeline that referenced this issue Feb 20, 2020
FP 0.18 Implement WhatsApp parsing
1e73e33 
Parse raw report and ignore checks from probe due to
ooni/probe-engine#341
@FedericoCeratto FedericoCeratto mentioned this issue Feb 20, 2020
Merged
@bassosimone bassosimone mentioned this issue Feb 21, 2020
Closed
@bassosimone
Copy link
Contributor Author

So, I have implemented a workaround in MK as part of measurement-kit/measurement-kit#1915. Because this is annoying and may have data analysis implications, I have also documented it as part of the experiment spec in ooni/spec#178. There are more follow-up actions required from me to move forward this issue, including tagging a new release of Measurement Kit, rebuilding everything, and shipping it. I will reference this issue as the master issue for all these activities, since it's basically the interrupt to which I reacted.

FedericoCeratto pushed a commit to ooni/pipeline that referenced this issue Feb 21, 2020
FP 0.18 Implement WhatsApp parsing
18a3d6b 
Parse raw report and ignore checks from probe due to
ooni/probe-engine#341
@bassosimone bassosimone mentioned this issue Feb 24, 2020
Merged
@bassosimone
Copy link
Contributor Author

I'm proceeding to release MK v0.10.10 and to rebuild everything.

@bassosimone bassosimone added effort/L Large effort effort/XL Extra large effort and removed effort/L Large effort labels Feb 25, 2020
bassosimone added a commit to measurement-kit/script-build-linux that referenced this issue Feb 25, 2020
@bassosimone
Build MK v0.10.10
e757204 
Part of ooni/probe-engine#341
@bassosimone
Copy link
Contributor Author

Have rebuilt MK v0.10.10 for Catalina here: measurement-kit/homebrew-measurement-kit@e28d890

bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 26, 2020
@bassosimone
deps: latest upper.io/db and pin probe-engine
ecd916b 
We're specifically pinning a commit of probe-engine that is using
MK v0.10.11, so that we address the following issues:

1. we use db-ip.com for the country database as opposed to using
the increasingly stale MaxMind database (see ooni/probe-engine#334)

2. we're using an implementation of WhatsApp that does not suffer
from the super-old CIDR bug (see ooni/probe-engine#341)

3. we're not linking to libcurl anymore on Windows and Linux, thanks to
this new version of MK where we can optionally disable libcurl; we are
still linking to libcurl on macOS, but that has no impact on the binary
size since on macOS libcurl is part of the system

This should be enough, from my side to bless a new release of the
probe-cli (see ooni/probe#1028).
@bassosimone bassosimone mentioned this issue Feb 26, 2020
Merged
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 26, 2020
@bassosimone
deps: latest upper.io/db and pin probe-engine (#116)
f3b21f9 
We're specifically pinning a commit of probe-engine that is using
MK v0.10.11, so that we address the following issues:

1. we use db-ip.com for the country database as opposed to using
the increasingly stale MaxMind database (see ooni/probe-engine#334)

2. we're using an implementation of WhatsApp that does not suffer
from the super-old CIDR bug (see ooni/probe-engine#341)

3. we're not linking to libcurl anymore on Windows and Linux, thanks to
this new version of MK where we can optionally disable libcurl; we are
still linking to libcurl on macOS, but that has no impact on the binary
size since on macOS libcurl is part of the system

This should be enough, from my side to bless a new release of the
probe-cli (see ooni/probe#1028).
@bassosimone
Copy link
Contributor Author

Done for Android here: measurement-kit/android-libs@d2b58c8

@bassosimone
Copy link
Contributor Author

Done for iOS here measurement-kit/mkall-ios@32b7606 and here https://github.com/measurement-kit/mkall-ios/releases/tag/v0.8.0

@bassosimone
Copy link
Contributor Author

This issue should actually be open. We have implemented a workaround in MK but we have not fixed the real bug, i.e. that we're using an obsolete WhatsApp CIDR.

@bassosimone bassosimone reopened this Jun 22, 2020
@bassosimone bassosimone removed this from the Sprint 7 - Crown Jellyfish milestone Jun 22, 2020
@bassosimone bassosimone added this to the Sprint 17 - Θέτις milestone Jul 2, 2020
@bassosimone bassosimone added discuss We need to have a conversation effort/XS Extra small effort and removed interrupt Task not planned during planning effort/XL Extra large effort labels Jul 2, 2020
@bassosimone
Copy link
Contributor Author

This should also be discussed along with plans to improve the IM tests methodology.

@bassosimone
Copy link
Contributor Author

This is no longer an issue: the plan in #740 will fix this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bassosimone bassosimone

Labels
bug Something isn't working discuss We need to have a conversation effort/XS Extra small effort priority/high High priority
Projects
None yet
Milestone
Sprint 17 - Θέτις
Development

No branches or pull requests
2 participants
@bassosimone @hellais