1582343 - Soft token MAC verification not constant time

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Deian Stefan]

A bunch of the MAC verification functions use PORT_Memcmp (=memcmp). It's not clear if this is actually a problem, but worth creating a bug:

• sftk_SSLMACVerify()
• NSC_VerifyFinal()
• sftk_TLSPRFVerify
• Anything using HMACContext's::verify()

Comment 1

[Martin Thomson [:mt:]]

The risk here is that an attacker can attempt to progressively find a MAC value (or other verifier) for a known input by observing the timing side channel. It does not endanger inputs in any way.

Our TLS stack is not affected (TLS 1.3, TLS <=1.2), and nor is WebCrypto. But this is still serious.

When fixing this, we should try to be more rigorous and find all uses of memcmp and friends. Inconsistent use of the PORT_Memcmp variant means that we'll have to look for all the different variants, maybe by using #define memcmp(_, _) PR_STATIC_ASSERT(false). I don't know if we'd have much success with dudect or similar, but we might even try that.

Comment 2

[Deian Stefan]

Attachment: 0001-Bug-1582343-Use-NSS_SecureMemcmp-instead-of-PORT_Mem.patch

Comment 3

[Deian Stefan]

@Martin: Yeah that sounds reasonable. Though memcmp is used in some cases safely in, say, the pkcs11c.c file.

RE dudect: This sounds great. Though if we know what's secret (we'd need this for dudect anyway), we can also throw Fraser's checkers at this (shes working on some CT checkers), and a new constant-time verification tool (faster and simpler than ct-verif) we've been building.

Comment 4

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9094007 [details] [diff] [review]
0001-Bug-1582343-Use-NSS_SecureMemcmp-instead-of-PORT_Mem.patch

This looks good to me. I don't see any other interesting uses of `memcmp` in softoken.

Comment 5

[Martin Thomson [:mt:]]

As we are coming up on the 3.47 release, we can land this any time.

Comment 6

[J.C. Jones [:jcj] (he/him)]

I'll land this in 3.47 later this week

Comment 7

[J.C. Jones [:jcj] (he/him)]

https://hg.mozilla.org/projects/nss/rev/86ef6ba1f1d791364c391d4746c47feedd01c288

Comment 8

[Tom Ritter [:tjr]]

No advisory, given we don't know if this is triggerable. ni if disagree