Validate endpoint and dns in WireGuard config #24100

bsclifton · 2024-06-10T18:57:50Z

Fixes brave/brave-browser#37960

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used GitHub auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
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 wiki
- npm run presubmit wiki, 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

See brave/brave-browser#37960

components/brave_vpn/common/wireguard/wireguard_utils.cc

bsclifton · 2024-06-10T21:49:13Z

Whoops - just noticed this:

Using std::regex adds unnecessary binary size to Chrome. Please use
re2::RE2 instead (crbug.com/755321)

Fixing now...

diracdeltas

can be bypassed with whitespace, see https://bravesoftware.slack.com/archives/C6R461GF4/p1718115100660649

bsclifton · 2024-06-11T21:14:50Z

Fixed and added tests for those cases. Thanks @diracdeltas

components/brave_vpn/common/wireguard/wireguard_utils.cc

diracdeltas

reject the entire config if it contains any unicode characters OR any # - see https://bravesoftware.slack.com/archives/C6R461GF4/p1718182338474479 and https://bravesoftware.slack.com/archives/C6R461GF4/p1718215096342729 for test cases

bsclifton · 2024-06-12T19:58:14Z

Updated to add these checks

components/brave_vpn/common/wireguard/wireguard_utils.cc

simonhong

++ 👍🏼

components/brave_vpn/common/wireguard/wireguard_utils_unittest.cc

Fixes brave/brave-browser#37960

bridiver · 2024-06-13T16:25:12Z

components/brave_vpn/common/wireguard/wireguard_utils.cc

+  // Match our expected `DNS` field.
+  absl::string_view input(config_str);
+  while (match_count < 2 &&
+         re2::RE2::FindAndConsume(&input, R"((?m)^\s*DNS = 1\.1\.1\.1$)")) {


seems a bit odd to me that we're using regex to validate things like ip address and hostnames since there are better tools for that in chromium. Wouldn't it be better to capture the field value and validate it using GURL, etc... ?

I think with GURL you still have to add a schema. There's also net::HostPortPair.

With our code, we're hardcoding 1.1.1.1 as the DNS. Since that's the case, I think it's fine to do a text match like this. Unless we change the default, there's no need to validate (it's either right or wrong).

Hostname is a bit more complex. We could validate with GURL; but we just want to make sure the two expected domains are found in the domain for Endpoint. Subdomain can have any value

bridiver · 2024-06-13T16:26:12Z

components/brave_vpn/common/wireguard/wireguard_utils.cc

+  // low ascii (excluding CR/LF), #, high ascii
+  if (re2::RE2::PartialMatch(
+          config_str,
+          R"([\x00-\x09]|[\x0b-\x0c]|[\x0d-\x1f]|[\x23]|[\x7f-\xFF])")) {


same as above, shouldn't we be capturing the value and using existing utilities to validate their contents?

By the way, there is a simple INI parser in chromium under chrome/common/ini_parser.h, although this one wouldn´t handle multi entry for keys.

With this, we just want to make sure none of these characters are in the config. It's not allowed

cdesouza-chromium · 2024-06-13T16:29:10Z

components/brave_vpn/common/wireguard/wireguard_utils.cc

@@ -82,6 +143,30 @@ WireguardKeyPair GenerateNewX25519Keypair() {
      EncodeBase64(std::vector<uint8_t>(privkey, privkey + 32)));
 }

+// See https://github.com/google/re2/wiki/Syntax for RE2 syntax
+bool IsValidConfig(const wchar_t* config) {


Don't you think it would be better to have base::wcstring_view config here?

We could - the COM interface that is calling this (IsValidConfig) is in browser/brave_vpn/win/brave_vpn_wireguard_service/service/brave_wireguard_manager.cc (see EnableVpn).

I didn't want to put any casting logic in the method, just passing the raw config value to the validation.

base::wcstring_view won't require any casting here though. It will act just like wchar_t*, but a little bit safer.

I tried and ran into some casting issues 😦

cdesouza-chromium · 2024-06-13T16:32:41Z

components/brave_vpn/common/wireguard/wireguard_utils.cc

+  // low ascii (excluding CR/LF), #, high ascii
+  if (re2::RE2::PartialMatch(
+          config_str,
+          R"([\x00-\x09]|[\x0b-\x0c]|[\x0d-\x1f]|[\x23]|[\x7f-\xFF])")) {


I wonder if this is the equivalent of base::IsStringASCII

Basically this is making sure something is not in ASCII range 0 - 9, 11, 12, 14-31, 35 (#), or 126+

10 and 13 allowed because config has CR (13) and LF (10)

I suspect that's what base::IsStringASCII checks for
https://source.chromium.org/chromium/chromium/src/+/main:base/strings/string_util_unittest.cc;l=595;drc=90cac1911508d3d682a67c97aa62483eb712f69a

Would need to see what the masks evaluate to:
https://source.chromium.org/chromium/chromium/src/+/main:base/strings/string_util_impl_helpers.h;l=164-167;drc=90cac1911508d3d682a67c97aa62483eb712f69a

I think what we have is OK - but can look further if it's a blocker

I think it's better to use a well tested existing utility than a regex if it has what we're looking for

cdesouza-chromium

LGTM % few comments

bsclifton · 2024-06-13T20:58:58Z

Created brave/brave-browser#39029 as a follow up

bsclifton · 2024-06-13T22:11:27Z

Talking with reviewers- a better solution would be to not pass a config to the COM interface.

Instead, only pass the required fields and then have the code generate the config (to avoid all these config parsing edge cases).

Closing PR

bsclifton requested review from diracdeltas and simonhong June 10, 2024 18:57

bsclifton self-assigned this Jun 10, 2024

bsclifton force-pushed the bsc-wg-endpoint-verify branch from c3193ab to f680371 Compare June 10, 2024 20:32

diracdeltas reviewed Jun 10, 2024

View reviewed changes

components/brave_vpn/common/wireguard/wireguard_utils.cc Outdated Show resolved Hide resolved

bsclifton requested a review from a team as a code owner June 11, 2024 07:50

bsclifton changed the title ~~Check hostname for WireGuard endpoint~~ Validate endpoint and dns in WireGuard config Jun 11, 2024

bsclifton force-pushed the bsc-wg-endpoint-verify branch 2 times, most recently from 6cc9717 to 3c8e4fb Compare June 11, 2024 16:31

diracdeltas requested changes Jun 11, 2024

View reviewed changes

bsclifton force-pushed the bsc-wg-endpoint-verify branch 2 times, most recently from dcee495 to 2e56d56 Compare June 11, 2024 21:12

diracdeltas reviewed Jun 11, 2024

View reviewed changes

components/brave_vpn/common/wireguard/wireguard_utils.cc Outdated Show resolved Hide resolved

diracdeltas reviewed Jun 11, 2024

View reviewed changes

components/brave_vpn/common/wireguard/wireguard_utils.cc Outdated Show resolved Hide resolved

bsclifton force-pushed the bsc-wg-endpoint-verify branch from 2e56d56 to c2418b0 Compare June 12, 2024 07:46

diracdeltas requested changes Jun 12, 2024

View reviewed changes

bsclifton force-pushed the bsc-wg-endpoint-verify branch from c2418b0 to 36fa9e0 Compare June 12, 2024 19:57

thypon reviewed Jun 12, 2024

View reviewed changes

components/brave_vpn/common/wireguard/wireguard_utils.cc Outdated Show resolved Hide resolved

bsclifton force-pushed the bsc-wg-endpoint-verify branch from 36fa9e0 to ee0afea Compare June 12, 2024 21:00

thypon approved these changes Jun 12, 2024

View reviewed changes

bsclifton force-pushed the bsc-wg-endpoint-verify branch from ee0afea to 6c651ee Compare June 12, 2024 21:12

diracdeltas self-requested a review June 12, 2024 21:19

bsclifton force-pushed the bsc-wg-endpoint-verify branch from 6c651ee to d2b85ad Compare June 13, 2024 06:46

simonhong approved these changes Jun 13, 2024

View reviewed changes

components/brave_vpn/common/wireguard/wireguard_utils_unittest.cc Outdated Show resolved Hide resolved

Check DNS and Endpoint fields in WireGuard config

2da00ce

Fixes brave/brave-browser#37960

bsclifton force-pushed the bsc-wg-endpoint-verify branch from d2b85ad to 2da00ce Compare June 13, 2024 07:51

diracdeltas approved these changes Jun 13, 2024

View reviewed changes

bsclifton enabled auto-merge June 13, 2024 16:21

bridiver reviewed Jun 13, 2024

View reviewed changes

cdesouza-chromium reviewed Jun 13, 2024

View reviewed changes

cdesouza-chromium approved these changes Jun 13, 2024

View reviewed changes

bsclifton closed this Jun 13, 2024

auto-merge was automatically disabled June 13, 2024 22:11
Pull request was closed

bsclifton deleted the bsc-wg-endpoint-verify branch June 13, 2024 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate endpoint and dns in WireGuard config #24100

Validate endpoint and dns in WireGuard config #24100

bsclifton commented Jun 10, 2024 •

edited

Loading

bsclifton commented Jun 10, 2024

diracdeltas left a comment

bsclifton commented Jun 11, 2024 •

edited

Loading

diracdeltas left a comment •

edited

Loading

bsclifton commented Jun 12, 2024

simonhong left a comment

bridiver Jun 13, 2024

cdesouza-chromium Jun 13, 2024 •

edited

Loading

bsclifton Jun 13, 2024 •

edited

Loading

bridiver Jun 13, 2024

cdesouza-chromium Jun 13, 2024

bsclifton Jun 13, 2024

cdesouza-chromium Jun 13, 2024 •

edited

Loading

bsclifton Jun 13, 2024 •

edited

Loading

cdesouza-chromium Jun 13, 2024 •

edited

Loading

bsclifton Jun 13, 2024

cdesouza-chromium Jun 13, 2024

bsclifton Jun 13, 2024 •

edited

Loading

cdesouza-chromium Jun 13, 2024

bsclifton Jun 13, 2024

bridiver Jun 13, 2024

cdesouza-chromium left a comment

bsclifton commented Jun 13, 2024

bsclifton commented Jun 13, 2024

Validate endpoint and dns in WireGuard config #24100

Validate endpoint and dns in WireGuard config #24100

Conversation

bsclifton commented Jun 10, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

bsclifton commented Jun 10, 2024

diracdeltas left a comment

Choose a reason for hiding this comment

bsclifton commented Jun 11, 2024 • edited Loading

diracdeltas left a comment • edited Loading

Choose a reason for hiding this comment

bsclifton commented Jun 12, 2024

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdesouza-chromium Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

bsclifton Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdesouza-chromium Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

bsclifton Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

cdesouza-chromium Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdesouza-chromium left a comment

Choose a reason for hiding this comment

bsclifton commented Jun 13, 2024

bsclifton commented Jun 13, 2024

bsclifton commented Jun 10, 2024 •

edited

Loading

bsclifton commented Jun 11, 2024 •

edited

Loading

diracdeltas left a comment •

edited

Loading

cdesouza-chromium Jun 13, 2024 •

edited

Loading

bsclifton Jun 13, 2024 •

edited

Loading

cdesouza-chromium Jun 13, 2024 •

edited

Loading

bsclifton Jun 13, 2024 •

edited

Loading

cdesouza-chromium Jun 13, 2024 •

edited

Loading

bsclifton Jun 13, 2024 •

edited

Loading