Page MenuHomePhabricator

[services-lib] Use constant-time-eq for token verification
ClosedPublic

Authored by bartek on Sep 20 2023, 2:50 AM.
Tags
None
Referenced Files
F2902985: D9242.id31306.diff
Sat, Oct 5, 7:49 PM
F2902822: D9242.id31589.diff
Sat, Oct 5, 7:12 PM
Unknown Object (File)
Wed, Sep 25, 1:55 PM
Unknown Object (File)
Mon, Sep 16, 8:33 AM
Unknown Object (File)
Sep 4 2024, 10:39 PM
Unknown Object (File)
Sep 3 2024, 3:17 PM
Unknown Object (File)
Sep 3 2024, 3:17 PM
Unknown Object (File)
Sep 3 2024, 3:16 PM
Subscribers

Details

Summary

verify_access_token in Identity Service does this so probably we should do it here too.

Depends on D9241

Test Plan

I did some simple comparisons and it seems to work fine.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Sep 20 2023, 3:02 AM

I'm not really familiar with this stuff, so I might be completely wrong but:

From the contant_time_eq docs:

// Not equal-sized, so won't take constant time.
assert!(!constant_time_eq(b"foo", b""));

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

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