Paths

Table of Contentst

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

Authored by varun on Aug 7 2024, 4:09 PM.

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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

@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 ↗	(On Diff #43243)	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

add debug logs

varun added inline comments.Aug 8 2024, 12:17 AM

services/tunnelbroker/src/websockets/session.rs
310–319 ↗	(On Diff #43243)	ah, just saw this. will address this feedback before landing

Harbormaster failed remote builds in B30998: Diff 43244!Aug 8 2024, 12:39 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.

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).

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

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

address feedback

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

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

I don't think this is a good solution, the client is unable to receive notifs
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….