Page MenuHomePhabricator
Differential D13022

[tunnelbroker] disallow windows device tokens that don't contain the correct domain
ClosedPublic
Actions

Authored by varun on Aug 7 2024, 4:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 7:43 PM
Unknown Object (File)
Sat, Nov 9, 7:32 PM
Unknown Object (File)
Sat, Nov 9, 7:26 PM
Unknown Object (File)
Sat, Nov 9, 4:15 PM
Unknown Object (File)
Sat, Nov 2, 5:39 AM
Unknown Object (File)
Sat, Nov 2, 5:39 AM
Unknown Object (File)
Sat, Nov 2, 5:38 AM
Unknown Object (File)
Sat, Nov 2, 5:38 AM
View All 67 Files
Subscribers
ashoat
tomek

Details

Reviewers
bartek
kamil
Commits
rCOMM7001e487e341: [tunnelbroker] disallow windows device tokens that don't contain the correct…
Summary

the device token should be a valid URL and should contain the correct domain. per the docs:

It is important that the cloud service always ensures that the channel URI uses the domain "notify.windows.com". The service should never push notifications to a channel on any other domain. If the callback for your app is ever compromised, a malicious attacker could submit a channel URI to spoof WNS. Without inspecting the domain, your cloud service could potentially disclose information to this attacker unknowingly. The subdomain of the channel URI is subject to change and should not be considered when validating the channel URI.

Depends on D13020

Test Plan

tried to send an invalid windows device token and it was rejected, verified that it wasn't in DDB

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Aug 7 2024, 4:09 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 7 2024, 4:09 PM
varun published this revision for review.Aug 7 2024, 4:09 PM
Harbormaster failed remote builds in B30997: Diff 43243!Aug 7 2024, 4:32 PM
varun added a comment.Aug 7 2024, 10:55 PM

@kamil I wonder if we should do something similar for other platforms, too

bartek accepted this revision.Aug 8 2024, 12:06 AM
bartek added inline comments.
services/tunnelbroker/src/websockets/session.rs
310–319

Nit, not sure if less verbose / more readable

Alternatively

Url::parse(&token_with_platform.device_token)
  .ok()
  .filter(|url| {
    url
      .domain()
      .is_some_and(|domain| domain.ends_with("notify.windows.com"))
  })
  .ok_or(SessionError::InvalidDeviceToken)?;
This revision is now accepted and ready to land.Aug 8 2024, 12:06 AM
varun updated this revision to Diff 43244.Aug 8 2024, 12:15 AM

add debug logs

varun added inline comments.Aug 8 2024, 12:17 AM
services/tunnelbroker/src/websockets/session.rs
310–319

ah, just saw this. will address this feedback before landing

Harbormaster failed remote builds in B30998: Diff 43244!Aug 8 2024, 12:39 AM
kamil added a comment.Aug 8 2024, 12:48 AM
In D13022#368148, @varun wrote:

@kamil I wonder if we should do something similar for other platforms, too

For FCM and APNs we have a hardcoded domain in Rust, the device token is used as the route/header in the request (URI is not part of the device token).

For the Web Push endpoint is part of the device token (device token is JSON stringified Subsctpion Info), however, there is a different URL for each browser, and AFAIK can change it, e.g. for Firefox this is something like https://updates.push.services.mozilla.com and for Safari https://web.push.apple.com and as long as there isn't anything explicit about validating endpoint we shouldn't do it.

But since this is explicit in WNS docs we probably should make sure this is done on the keyserver too and at least create a follow-up task?

kamil accepted this revision.Aug 8 2024, 12:52 AM
kamil added inline comments.
services/tunnelbroker/src/websockets/session.rs
317 ↗(On Diff #43244)

I would prefer to use a different error here - InvalidDeviceToken was introduced to specifically inform the client (when sending notif) that the recipient device token was invalidated by the Push Service and a new one should be uploaded, here is a bit different case - we failed to upload it.

kamil added a comment.Aug 8 2024, 12:57 AM

Additionally, when Tunnelbroker informs us about the fact the token is not correct we should reset the localToken, otherwise client will continuously try to upload the token without refreshing this using electron API.

To achieve that, you should also handle setTBDeviceTokenActionTypes.failed in here and set localToken: null but only when the error is the specific error here (InvalidDeviceToken or different if you address my comment).

kamil requested changes to this revision.Aug 8 2024, 12:58 AM

Requesting to address the above comment, sorry for blocking but this is an important one.

This revision now requires changes to proceed.Aug 8 2024, 12:58 AM
varun added a comment.Aug 8 2024, 8:34 AM

But since this is explicit in WNS docs we probably should make sure this is done on the keyserver too and at least create a follow-up task?

It's actually already done on keyserver here: https://github.com/CommE2E/comm/blob/master/keyserver/src/push/utils.js#L366-L374

varun updated this revision to Diff 43268.Aug 8 2024, 4:41 PM

address feedback

varun added a comment.Aug 8 2024, 4:48 PM
In D13022#368163, @kamil wrote:

Additionally, when Tunnelbroker informs us about the fact the token is not correct we should reset the localToken, otherwise client will continuously try to upload the token without refreshing this using electron API.

i didn't observe the client continuously retrying until i modified the reducer to set localToken to null

To achieve that, you should also handle setTBDeviceTokenActionTypes.failed in here and set localToken: null but only when the error is the specific error here (InvalidDeviceToken or different if you address my comment).

this was a little more complicated because registerForNotifications() returns the same channel URL every time unless the channel is closed. i honestly don't love my solution, but the alternatives are 1) ignoring the setTBDeviceTokenActionTypes.failed action type and just not having a device token for the device in tunnelbroker or 2) spam tunnelbroker by trying to upload the same token

Harbormaster completed remote builds in B31018: Diff 43268.Aug 8 2024, 4:58 PM
kamil accepted this revision.Aug 12 2024, 8:00 AM
In D13022#368426, @varun wrote:

To achieve that, you should also handle setTBDeviceTokenActionTypes.failed in here and set localToken: null but only when the error is the specific error here (InvalidDeviceToken or different if you address my comment).

this was a little more complicated because registerForNotifications() returns the same channel URL every time unless the channel is closed. i honestly don't love my solution, but the alternatives are 1) ignoring the setTBDeviceTokenActionTypes.failed action type and just not having a device token for the device in tunnelbroker or 2) spam tunnelbroker by trying to upload the same token

  1. I don't think this is a good solution, the client is unable to receive notifs
  2. This won't work, we trying to upload the token only if localToken !== tunnelbrokerToken

I think your solution is okay

This revision is now accepted and ready to land.Aug 12 2024, 8:00 AM
Closed by commit rCOMM7001e487e341: [tunnelbroker] disallow windows device tokens that don't contain the correct… (authored by varun). · Explain WhyAug 12 2024, 8:12 AM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMM7001e487e341: [tunnelbroker] disallow windows device tokens that don't contain the correct….

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
src/
websockets/
14 lines

Diff 43243

View Options

services/tunnelbroker/src/websockets/session.rs

Loading...