Add WebSockets connection pool limit when shields are enabled. by goodov · Pull Request #11609 · brave/brave-core

goodov · 2021-12-15T14:32:02Z

Resolves brave/brave-browser#19990

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
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, 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:

iefremov · 2021-12-17T10:41:40Z

Most probably we don't need a formal sec review, but I'd double-check with someone from the sec team. Maybe @fmarier you could take a quick look?

iefremov · 2021-12-17T10:46:07Z

perhaps we can enable by default, the change doesn't look very dangerous?

👌 I'll make it enabled by default and will add a brave://flag to be safe.

iefremov · 2021-12-17T10:46:41Z

is almost 2022! (in all files)

iefremov · 2021-12-17T12:48:29Z

perhaps a browser test?

goodov · 2021-12-17T12:53:56Z

perhaps a browser test?

of course. currently working on it.

fmarier

Most probably we don't need a formal sec review, but I'd double-check with someone from the sec team.

The high-level approach sounds good to me. It's not adding (or really modifying) any new networks requests, so I don't think we need to do a full sec review.

I assume @pes10k has confirmed that this mitigation is effective against the pool party attack?

pes10k · 2021-12-17T19:42:42Z

Yep! Limiting each eTLD+1 to a maximum of 10 websockets would effectively defeat the attack (an attacker could hypothetically still carry out the attack by controlling ~20 domains at the same time, and coordinating between them, but this approach makes the attack sufficiently difficult to carry out that I think we can consider it solved (at least when carried out through the WebSocket pool, which is the most concerning example).

Just to confirm @goodov, this PR does

Partition: impose a per-site (as determined by top-level frame eTLD+1) cap of how many websockets can be open. Initial suggestion is 10, but any cap thats << the global cap would work

but not

De-limit: remove, or greatly expand, the global cap, to increase the number of eTLD+1s an attack has to control to carry out the attack

Is that correct?

goodov · 2021-12-20T14:20:56Z

Just to confirm @goodov, this PR does

Partition: impose a per-site (as determined by top-level frame eTLD+1) cap of how many websockets can be open. Initial suggestion is 10, but any cap thats << the global cap would work

but not

De-limit: remove, or greatly expand, the global cap, to increase the number of eTLD+1s an attack has to control to carry out the attack

Is that correct?

yes, that is correct!

goodov · 2021-12-20T17:38:36Z

+      if (auto* top_frame_security_context =
+              frame->Top()->GetSecurityContext()) {
+        return top_frame_security_context->GetSecurityOrigin()
+            ->GetOriginOrPrecursorOriginIfOpaque();


see SandboxedFramesAreLimited test.

goodov · 2021-12-30T09:34:45Z

@bridiver chromium_src, patches review ping, other reviewers are currently on PTO.

iefremov · 2022-01-05T12:39:59Z

@mkarolin @bridiver PTAL, thanks!

mkarolin · 2022-01-05T16:00:00Z

+
+namespace blink {
+
+class MODULES_EXPORT WebSocketChannelImpl


Should we restore finality here?

mkarolin · 2022-01-05T16:04:05Z

+    const String& resource_id_in_use() const { return resource_id_in_use_; }
+
+   private:
+    String resource_id_in_use_;


Nit: seems like this and the getter could just be called resource_id_/resource_id()?

mkarolin · 2022-01-05T16:06:40Z

+
+  MutexLocker locker(resources_in_use_lock_);
+  // `insert` doesn't change the value if it already exists.
+  int& resource_in_use_value =


Nit: resource_in_use_count might be a bit more clear.

mkarolin · 2022-01-05T16:08:29Z

+int GetResourceLimit(ResourceType resource_type) {
+  switch (resource_type) {
+    case ResourceType::kWebSocket:
+      return 10;


Should this be hard-coded? Would it make sense to make it a feature param, for example, and control it via Griffin?

iefremov · 2022-01-05T17:38:32Z

@mkarolin turned out we want to merge it asap, would you mind approving it and then we can do a follow-up?

mkarolin

++ with a couple of nits and questions that can be address in a followup.

mkarolin · 2022-01-05T18:00:31Z

@mkarolin turned out we want to merge it asap, would you mind approving it and then we can do a follow-up?

No problem.

This reverts commit b217cd4, reversing changes made to 8f34773.

goodov requested review from a team as code owners December 15, 2021 14:32

goodov self-assigned this Dec 15, 2021

github-actions bot added the CI/run-network-audit Run network-audit label Dec 15, 2021

goodov force-pushed the issues/19990 branch 2 times, most recently from e75a1a3 to a5cddac Compare December 16, 2021 05:30

iefremov requested a review from fmarier December 17, 2021 10:41

iefremov reviewed Dec 17, 2021

View reviewed changes

goodov force-pushed the issues/19990 branch 2 times, most recently from 00a5a0f to 849b8d1 Compare December 17, 2021 16:20

fmarier approved these changes Dec 17, 2021

View reviewed changes

goodov force-pushed the issues/19990 branch from 849b8d1 to 8dbee3d Compare December 20, 2021 14:13

goodov force-pushed the issues/19990 branch from 8dbee3d to 20893a4 Compare December 20, 2021 16:59

goodov commented Dec 20, 2021

View reviewed changes

goodov force-pushed the issues/19990 branch 3 times, most recently from 56dc1ab to 68176d5 Compare December 27, 2021 09:17

iefremov approved these changes Dec 28, 2021

View reviewed changes

goodov added 5 commits December 30, 2021 11:20

Add WebSockets connection pool limit when shields are enabled.

69b9006

Update copyright year.

2294d2d

Enable feature by default, add brave://flags entry.

d341741

Add browser tests for in-page WebSockets.

3bbbb0e

Use top frame origin if possible for resource pool keying.

ced9ecd

goodov added 2 commits December 30, 2021 11:20

Add ServiceWorker tests.

3b7488d

Simplify tests.

5e45353

goodov force-pushed the issues/19990 branch from 68176d5 to 5e45353 Compare December 30, 2021 04:20

goodov requested a review from bridiver December 30, 2021 09:32

mkarolin reviewed Jan 5, 2022

View reviewed changes

mkarolin approved these changes Jan 5, 2022

View reviewed changes

iefremov merged commit b217cd4 into master Jan 6, 2022

iefremov deleted the issues/19990 branch January 6, 2022 05:17

bbondy added a commit that referenced this pull request Jan 6, 2022

Revert "Merge pull request #11609 from /issues/19990"

90cbdda

This reverts commit b217cd4, reversing changes made to 8f34773.

goodov added this to the 1.36.x - Nightly milestone Jan 10, 2022

goodov mentioned this pull request Mar 15, 2022

Don't impose WebSockets limits on Extensions #12597

Merged

25 tasks

Conversation

goodov commented Dec 15, 2021 • edited by iefremov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Uh oh!

iefremov commented Dec 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iefremov commented Dec 17, 2021

Uh oh!

goodov commented Dec 17, 2021

Uh oh!

fmarier left a comment

Choose a reason for hiding this comment

Uh oh!

pes10k commented Dec 17, 2021

Uh oh!

goodov commented Dec 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodov commented Dec 30, 2021

Uh oh!

iefremov commented Jan 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iefremov commented Jan 5, 2022

Uh oh!

mkarolin left a comment

Choose a reason for hiding this comment

Uh oh!

mkarolin commented Jan 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

goodov commented Dec 15, 2021 •

edited by iefremov

Loading