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

Avoid CNAME uncloaking if a proxy is configured #8957

Merged
merged 5 commits into from
Jun 11, 2021
Merged

Avoid CNAME uncloaking if a proxy is configured #8957

merged 5 commits into from
Jun 11, 2021

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented May 28, 2021 •
edited
Loading

Resolves brave/brave-browser#16011

Caveats about the previous implementation (since updated)

Disclaimer: this doesn't seem like the right solution, but I'm opening this now to get discussion going on how it should best be done, and possibly to get it merged as a short-term fix. In particular:

  • The semantics of ProxyConfigDictionary are not quite what we're looking for. It'd be preferable to learn whether or not the proxy is in SingleProxy mode, but that's a private part of UrlRequestContext which I don't even see how to get here.
  • This is difficult to test appropriately. I've only been able to test it so far by manually adding LOG statements and checking whether or not the correct if-branch is selected when the NordVPN extension is installed (on Linux).

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Install the NordVPN extension.
  • Sign in to the extension via the menu in the plugin area at the top-right of the browser (NordVPN credentials are available in 1Password).
  • In the extension UI, connect to the VPN. Use a location that is distinctly different from your actual location.
  • Visit http://browserleaks.com/dns and begin the test.
  • Once the test is complete, verify that the "Your DNS Servers" section does not show your actual location.

@antonok-edm antonok-edm self-assigned this May 28, 2021
diracdeltas
diracdeltas approved these changes May 28, 2021
View reviewed changes
Copy link
Member

@diracdeltas diracdeltas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not an expert here but this general approach sgtm

@antonok-edm antonok-edm force-pushed the proxy-disable-cname-uncloaking branch from fec927b to 801cadc Compare June 3, 2021 17:39
@antonok-edm antonok-edm requested a review from darkdh June 3, 2021 17:40
@antonok-edm antonok-edm force-pushed the proxy-disable-cname-uncloaking branch from 801cadc to 7803660 Compare June 3, 2021 18:07
iefremov
iefremov reviewed Jun 4, 2021
View reviewed changes
browser/net/brave_ad_block_tp_network_delegate_helper.cc Outdated
@@ -245,6 +252,41 @@ void OnBeforeURLRequestAdBlockTP(const ResponseCallback& next_callback,
brave_shields::features::kBraveAdblockCnameUncloaking) &&
ctx->browser_context && !ctx->browser_context->IsTor();

// Also, skip CNAME uncloaking if there is currently a configured proxy.
if (ctx->browser_context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would extract this block to a separate function

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

@antonok-edm
Copy link
Collaborator Author

AdBlockServiceTest.CnameCloakedRequestsGetBlocked and AdBlockServiceTest.CnameCloakedRequestsCanBeExcepted are failing on Windows with this approach; something must be causing ProxySettingsAllowUncloaking to return false (possibly the CONFIG_PENDING branch?). I'm pushing some extra logging statements to investigate further since I don't have a Windows machine to test on locally.

@antonok-edm antonok-edm added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jun 4, 2021
antonok-edm
antonok-edm commented Jun 7, 2021
View reviewed changes
browser/net/brave_ad_block_tp_network_delegate_helper.cc Outdated
Comment on lines 268 to 274
} else if (availability ==
net::ProxyConfigService::ConfigAvailability::CONFIG_PENDING) {
// Fallback to not CNAME uncloaking if the proxy configuration cannot be
// determined.
LOG(ERROR) << "CONFIG_PENDING";
can_uncloak = false;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to CI, this gets triggered on Windows without any additional configuration. It's unclear whether or not that's the case every time the proxy service is created, in which case we'll never have CNAME uncloaking on Windows with this approach, or if it will permanently be resolved after a short time on browser startup, in which case this is probably an acceptable fix.

Any thoughts @iefremov @darkdh?

@iefremov iefremov self-requested a review June 9, 2021 10:40
@antonok-edm antonok-edm merged commit a26a583 into master Jun 11, 2021
@antonok-edm antonok-edm deleted the proxy-disable-cname-uncloaking branch June 11, 2021 15:22
@antonok-edm antonok-edm mentioned this pull request Jun 11, 2021
Merged
7 tasks
@antonok-edm antonok-edm modified the milestone: 1.27.x - Nightly Jun 11, 2021
@antonok-edm antonok-edm added this to the 1.26.x - Beta milestone Jun 15, 2021
@GeetaSarvadnya GeetaSarvadnya mentioned this pull request Jun 21, 2021
Closed
@diracdeltas diracdeltas mentioned this pull request Jul 6, 2021
Closed
@antonok-edm antonok-edm mentioned this pull request Jul 28, 2022
Closed
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh approved these changes

@diracdeltas diracdeltas diracdeltas approved these changes

@iefremov iefremov Awaiting requested review from iefremov iefremov is a code owner

Assignees

@antonok-edm antonok-edm

Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
Milestone
1.26.x - Release
Development

Successfully merging this pull request may close these issues.

[Security] [hackerone] adblocking DNS issue
4 participants
@antonok-edm @diracdeltas @iefremov @darkdh