1742205 - Possible incorrect locking in nsNSSCallbacks.cpp

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

PreliminaryHandshakeDone(fd) in nsNSSCallbacks.cpp accesses several fields that I think are supposed to be protected by the mMutex in nsNSSSocketInfo. In particular:

mCipherSuite, mProtocolVersion, mKeaGroup, and mSignatureSchemeName

I set a breakpoint and verified we do not hold the mutex at this point. Several of them appear to only be accessed with a locked mutex in TransportSecurityInfo.cpp, such as in GetSecretKeyLength() (mCipherSuite).

At minimum, even if none of these require locking, what's supposed to require locking should be documented in TransportSecurityInfo.h.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

It would be nice if we could start using DataMutex instead. That would make it less likely to get the locking wrong when this code changes in the future.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: Bug 1742205 - reorganize setting TLS handshake information r?jschanck

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Based on https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html this needs a sec-approval to land.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #3)

Based on https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html this needs a sec-approval to land.

Given there's no sec rating yet, I concur. dveditz?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

These fields are for display purposes or tests, so this doesn't seem like a huge concern from a security perspective.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

reorganize setting TLS handshake information r=jschanck
https://hg.mozilla.org/integration/autoland/rev/7d346ded011aad5812aee428a25cc43e768d9ea6
https://hg.mozilla.org/mozilla-central/rev/7d346ded011a