We don't check the length anywhere so couldn't the attacker start by sending a one-byte token, and after they find a match send a two-byte token, etc...?

For context the comment from https://phab.comm.dev/D4199?id=13456#inline-26218:

[without using the constant_time_eq] This introduces a timing side channel attack where the attacker can extract information about how much of the request token is right by the amount of time the comparison takes

after they find a match

i'm not sure what you mean by this. an attacker could learn the length of the token, but that's not a big deal - there'd still be enough entropy for a proper token

varun accepted this revision.Sep 20 2023, 11:22 PM

This revision is now accepted and ready to land.Sep 20 2023, 11:22 PM

Ok, seems like constant_time_eq checks the lengths inside, from looking at the code

bartek mentioned this in D9279: [services-lib] Add fn to verify services token.Sep 25 2023, 11:26 AM

Closed by commit rCOMM863cdaaa62b7: [services-lib] Use constant-time-eq for token verification (authored by bartek). · Explain WhyOct 2 2023, 6:27 AM

This revision was automatically updated to reflect the committed changes.

bartek added a commit: rCOMM863cdaaa62b7: [services-lib] Use constant-time-eq for token verification.

Revision Contents
Changeset List

Path

Size

services/

backup/

blob/

comm-services-lib/

1 line

src/

auth.rs

9 lines

commtest/

Cargo.lock

feature-flags/

Cargo.lock

reports/

Cargo.lock

Diff 31589

View Options

services/backup/Cargo.lock

Show All 39 Lines
import { fetchUserInfos } from '../fetchers/user-fetchers';		import { fetchUserInfos } from '../fetchers/user-fetchers';
import type { Viewer } from '../session/viewer';		import type { Viewer } from '../session/viewer';
import { getAPNsNotificationTopic } from './providers';		import { getAPNsNotificationTopic } from './providers';
import {		import {
apnPush,		apnPush,
fcmPush,		fcmPush,
getUnreadCounts,		getUnreadCounts,
apnMaxNotificationPayloadByteSize,		apnMaxNotificationPayloadByteSize,
		fcmMaxNotificationPayloadByteSize,
} from './utils';		} from './utils';

type Device = {		type Device = {
+deviceType: DeviceType,		+deviceType: DeviceType,
+deviceToken: string,		+deviceToken: string,
+codeVersion: ?number,		+codeVersion: ?number,
};		};
type PushUserInfo = {		type PushUserInfo = {
▲ Show 20 Lines • Show All 469 Lines • ▼ Show 20 Lines	function prepareAndroidNotification(
dbID: string,		dbID: string,
codeVersion: number,		codeVersion: number,
): Object {		): Object {
const notifID = collapseKey ? collapseKey : dbID;		const notifID = collapseKey ? collapseKey : dbID;
const { merged, ...rest } = notifTextsForMessageInfo(		const { merged, ...rest } = notifTextsForMessageInfo(
allMessageInfos,		allMessageInfos,
threadInfo,		threadInfo,
);		);
const messageInfos = JSON.stringify(newRawMessageInfos);		const notification = {

if (badgeOnly && codeVersion < 69) {
// Older Android clients don't look at badgeOnly, so if we sent them the
// full payload they would treat it as a normal notif. Instead we will
// send them this payload that is missing an ID, which will prevent the
// system notif from being generated, but still allow for in-app notifs
// and badge updating.
return {
data: {		data: {
badge: unreadCount.toString(),		badge: unreadCount.toString(),
...rest,		...rest,
threadID: threadInfo.id,		threadID: threadInfo.id,
messageInfos,
},		},
};		};
}
return {		// The reason we only include `badgeOnly` for newer clients is because older
data: {		// clients don't know how to parse it. The reason we only include `id` for
badge: unreadCount.toString(),		// newer clients is that if the older clients see that field, they assume
...rest,		// the notif has a full payload, and then crash when trying to parse it.
		// By skipping `id` we allow old clients to still handle in-app notifs and
		// badge updating.
		if (!badgeOnly \|\| codeVersion >= 69) {
		notification.data = {
		...notification.data,
id: notifID,		id: notifID,
threadID: threadInfo.id,
messageInfos,
badgeOnly: badgeOnly ? '1' : '0',		badgeOnly: badgeOnly ? '1' : '0',
},
};		};
}		}

		const messageInfos = JSON.stringify(newRawMessageInfos);
		const copyWithMessageInfos = {
		...notification,
		data: { ...notification.data, messageInfos },
		};

		if (
		Buffer.byteLength(JSON.stringify(copyWithMessageInfos)) <=
		fcmMaxNotificationPayloadByteSize
		) {
		return copyWithMessageInfos;
		}
		return notification;
		}

type NotificationInfo =		type NotificationInfo =
\| {		\| {
+source: 'new_message',		+source: 'new_message',
+dbID: string,		+dbID: string,
+userID: string,		+userID: string,
+threadID: string,		+threadID: string,
+messageID: string,		+messageID: string,
+collapseKey: ?string,		+collapseKey: ?string,
▲ Show 20 Lines • Show All 243 Lines • Show Last 20 Lines

View Options

services/blob/Cargo.lock

Show All 39 Lines
import { fetchUserInfos } from '../fetchers/user-fetchers';		import { fetchUserInfos } from '../fetchers/user-fetchers';
import type { Viewer } from '../session/viewer';		import type { Viewer } from '../session/viewer';
import { getAPNsNotificationTopic } from './providers';		import { getAPNsNotificationTopic } from './providers';
import {		import {
apnPush,		apnPush,
fcmPush,		fcmPush,
getUnreadCounts,		getUnreadCounts,
apnMaxNotificationPayloadByteSize,		apnMaxNotificationPayloadByteSize,
		fcmMaxNotificationPayloadByteSize,
} from './utils';		} from './utils';

type Device = {		type Device = {
+deviceType: DeviceType,		+deviceType: DeviceType,
+deviceToken: string,		+deviceToken: string,
+codeVersion: ?number,		+codeVersion: ?number,
};		};
type PushUserInfo = {		type PushUserInfo = {
▲ Show 20 Lines • Show All 469 Lines • ▼ Show 20 Lines	function prepareAndroidNotification(
dbID: string,		dbID: string,
codeVersion: number,		codeVersion: number,
): Object {		): Object {
const notifID = collapseKey ? collapseKey : dbID;		const notifID = collapseKey ? collapseKey : dbID;
const { merged, ...rest } = notifTextsForMessageInfo(		const { merged, ...rest } = notifTextsForMessageInfo(
allMessageInfos,		allMessageInfos,
threadInfo,		threadInfo,
);		);
const messageInfos = JSON.stringify(newRawMessageInfos);		const notification = {

if (badgeOnly && codeVersion < 69) {
// Older Android clients don't look at badgeOnly, so if we sent them the
// full payload they would treat it as a normal notif. Instead we will
// send them this payload that is missing an ID, which will prevent the
// system notif from being generated, but still allow for in-app notifs
// and badge updating.
return {
data: {		data: {
badge: unreadCount.toString(),		badge: unreadCount.toString(),
...rest,		...rest,
threadID: threadInfo.id,		threadID: threadInfo.id,
messageInfos,
},		},
};		};
}
return {		// The reason we only include `badgeOnly` for newer clients is because older
data: {		// clients don't know how to parse it. The reason we only include `id` for
badge: unreadCount.toString(),		// newer clients is that if the older clients see that field, they assume
...rest,		// the notif has a full payload, and then crash when trying to parse it.
		// By skipping `id` we allow old clients to still handle in-app notifs and
		// badge updating.
		if (!badgeOnly \|\| codeVersion >= 69) {
		notification.data = {
		...notification.data,
id: notifID,		id: notifID,
threadID: threadInfo.id,
messageInfos,
badgeOnly: badgeOnly ? '1' : '0',		badgeOnly: badgeOnly ? '1' : '0',
},
};		};
}		}

		const messageInfos = JSON.stringify(newRawMessageInfos);
		const copyWithMessageInfos = {
		...notification,
		data: { ...notification.data, messageInfos },
		};

		if (
		Buffer.byteLength(JSON.stringify(copyWithMessageInfos)) <=
		fcmMaxNotificationPayloadByteSize
		) {
		return copyWithMessageInfos;
		}
		return notification;
		}

type NotificationInfo =		type NotificationInfo =
\| {		\| {
+source: 'new_message',		+source: 'new_message',
+dbID: string,		+dbID: string,
+userID: string,		+userID: string,
+threadID: string,		+threadID: string,
+messageID: string,		+messageID: string,
+collapseKey: ?string,		+collapseKey: ?string,
▲ Show 20 Lines • Show All 243 Lines • Show Last 20 Lines

View Options

services/comm-services-lib/Cargo.lock

Show All 39 Lines
import { fetchUserInfos } from '../fetchers/user-fetchers';		import { fetchUserInfos } from '../fetchers/user-fetchers';
import type { Viewer } from '../session/viewer';		import type { Viewer } from '../session/viewer';
import { getAPNsNotificationTopic } from './providers';		import { getAPNsNotificationTopic } from './providers';
import {		import {
apnPush,		apnPush,
fcmPush,		fcmPush,
getUnreadCounts,		getUnreadCounts,
apnMaxNotificationPayloadByteSize,		apnMaxNotificationPayloadByteSize,
		fcmMaxNotificationPayloadByteSize,
} from './utils';		} from './utils';

type Device = {		type Device = {
+deviceType: DeviceType,		+deviceType: DeviceType,
+deviceToken: string,		+deviceToken: string,
+codeVersion: ?number,		+codeVersion: ?number,
};		};
type PushUserInfo = {		type PushUserInfo = {
▲ Show 20 Lines • Show All 469 Lines • ▼ Show 20 Lines	function prepareAndroidNotification(
dbID: string,		dbID: string,
codeVersion: number,		codeVersion: number,
): Object {		): Object {
const notifID = collapseKey ? collapseKey : dbID;		const notifID = collapseKey ? collapseKey : dbID;
const { merged, ...rest } = notifTextsForMessageInfo(		const { merged, ...rest } = notifTextsForMessageInfo(
allMessageInfos,		allMessageInfos,
threadInfo,		threadInfo,
);		);
const messageInfos = JSON.stringify(newRawMessageInfos);		const notification = {

if (badgeOnly && codeVersion < 69) {
// Older Android clients don't look at badgeOnly, so if we sent them the
// full payload they would treat it as a normal notif. Instead we will
// send them this payload that is missing an ID, which will prevent the
// system notif from being generated, but still allow for in-app notifs
// and badge updating.
return {
data: {		data: {
badge: unreadCount.toString(),		badge: unreadCount.toString(),
...rest,		...rest,
threadID: threadInfo.id,		threadID: threadInfo.id,
messageInfos,
},		},
};		};
}
return {		// The reason we only include `badgeOnly` for newer clients is because older
data: {		// clients don't know how to parse it. The reason we only include `id` for
badge: unreadCount.toString(),		// newer clients is that if the older clients see that field, they assume
...rest,		// the notif has a full payload, and then crash when trying to parse it.
		// By skipping `id` we allow old clients to still handle in-app notifs and
		// badge updating.
		if (!badgeOnly \|\| codeVersion >= 69) {
		notification.data = {
		...notification.data,
id: notifID,		id: notifID,
threadID: threadInfo.id,
messageInfos,
badgeOnly: badgeOnly ? '1' : '0',		badgeOnly: badgeOnly ? '1' : '0',
},
};		};
}		}

		const messageInfos = JSON.stringify(newRawMessageInfos);
		const copyWithMessageInfos = {
		...notification,
		data: { ...notification.data, messageInfos },
		};

		if (
		Buffer.byteLength(JSON.stringify(copyWithMessageInfos)) <=
		fcmMaxNotificationPayloadByteSize
		) {
		return copyWithMessageInfos;
		}
		return notification;
		}

type NotificationInfo =		type NotificationInfo =
\| {		\| {
+source: 'new_message',		+source: 'new_message',
+dbID: string,		+dbID: string,
+userID: string,		+userID: string,
+threadID: string,		+threadID: string,
+messageID: string,		+messageID: string,
+collapseKey: ?string,		+collapseKey: ?string,
▲ Show 20 Lines • Show All 243 Lines • Show Last 20 Lines

View Options

services/comm-services-lib/Cargo.toml

Show All 39 Lines
import { fetchUserInfos } from '../fetchers/user-fetchers';		import { fetchUserInfos } from '../fetchers/user-fetchers';
import type { Viewer } from '../session/viewer';		import type { Viewer } from '../session/viewer';
import { getAPNsNotificationTopic } from './providers';		import { getAPNsNotificationTopic } from './providers';
import {		import {
apnPush,		apnPush,
fcmPush,		fcmPush,
getUnreadCounts,		getUnreadCounts,
apnMaxNotificationPayloadByteSize,		apnMaxNotificationPayloadByteSize,
		fcmMaxNotificationPayloadByteSize,
} from './utils';		} from './utils';

type Device = {		type Device = {
+deviceType: DeviceType,		+deviceType: DeviceType,
+deviceToken: string,		+deviceToken: string,
+codeVersion: ?number,		+codeVersion: ?number,
};		};
type PushUserInfo = {		type PushUserInfo = {
▲ Show 20 Lines • Show All 469 Lines • ▼ Show 20 Lines	function prepareAndroidNotification(
dbID: string,		dbID: string,
codeVersion: number,		codeVersion: number,
): Object {		): Object {
const notifID = collapseKey ? collapseKey : dbID;		const notifID = collapseKey ? collapseKey : dbID;
const { merged, ...rest } = notifTextsForMessageInfo(		const { merged, ...rest } = notifTextsForMessageInfo(
allMessageInfos,		allMessageInfos,
threadInfo,		threadInfo,
);		);
const messageInfos = JSON.stringify(newRawMessageInfos);		const notification = {

if (badgeOnly && codeVersion < 69) {
// Older Android clients don't look at badgeOnly, so if we sent them the
// full payload they would treat it as a normal notif. Instead we will
// send them this payload that is missing an ID, which will prevent the
// system notif from being generated, but still allow for in-app notifs
// and badge updating.
return {
data: {		data: {
badge: unreadCount.toString(),		badge: unreadCount.toString(),
...rest,		...rest,
threadID: threadInfo.id,		threadID: threadInfo.id,
messageInfos,
},		},
};		};
}
return {		// The reason we only include `badgeOnly` for newer clients is because older
data: {		// clients don't know how to parse it. The reason we only include `id` for
badge: unreadCount.toString(),		// newer clients is that if the older clients see that field, they assume
...rest,		// the notif has a full payload, and then crash when trying to parse it.
		// By skipping `id` we allow old clients to still handle in-app notifs and
		// badge updating.
		if (!badgeOnly \|\| codeVersion >= 69) {
		notification.data = {
		...notification.data,
id: notifID,		id: notifID,
threadID: threadInfo.id,
messageInfos,
badgeOnly: badgeOnly ? '1' : '0',		badgeOnly: badgeOnly ? '1' : '0',
},
};		};
}		}

		const messageInfos = JSON.stringify(newRawMessageInfos);
		const copyWithMessageInfos = {
		...notification,
		data: { ...notification.data, messageInfos },
		};

		if (
		Buffer.byteLength(JSON.stringify(copyWithMessageInfos)) <=
		fcmMaxNotificationPayloadByteSize
		) {
		return copyWithMessageInfos;
		}
		return notification;
		}

type NotificationInfo =		type NotificationInfo =
\| {		\| {
+source: 'new_message',		+source: 'new_message',
+dbID: string,		+dbID: string,
+userID: string,		+userID: string,
+threadID: string,		+threadID: string,
+messageID: string,		+messageID: string,
+collapseKey: ?string,		+collapseKey: ?string,
▲ Show 20 Lines • Show All 243 Lines • Show Last 20 Lines