Closed Bug 1582343 Opened 4 years ago Closed 4 years ago

Soft token MAC verification not constant time


(NSS :: Libraries, defect, P2)



(firefox-esr6870+ fixed, firefox67 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70+ fixed, firefox71+ fixed)

Tracking Status
firefox-esr68 70+ fixed
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 + fixed
firefox71 + fixed


(Reporter: deian, Assigned: deian)



(Keywords: csectype-other, sec-audit, Whiteboard: [adv-main70-][adv-esr68.2-][post-critsmash-triage])


(1 file)

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:

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.

Priority: -- → P3
Target Milestone: --- → 3.47
Priority: P3 → P2
Assignee: nobody → deian

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

Attachment #9094007 - Flags: review?(kjacobs.bugzilla)
Comment on attachment 9094007 [details] [diff] [review]

This looks good to me. I don't see any other interesting uses of `memcmp` in softoken.
Attachment #9094007 - Flags: review?(kjacobs.bugzilla) → review+

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

I'll land this in 3.47 later this week

Keywords: checkin-needed
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Group: crypto-core-security → core-security-release

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

Whiteboard: [adv-main70-]
Whiteboard: [adv-main70-] → [adv-main70-][adv-esr68.2-]
Flags: qe-verify-
Whiteboard: [adv-main70-][adv-esr68.2-] → [adv-main70-][adv-esr68.2-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.