813857 - Certificate trust flags are not thread safe

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Ryan Sleevi]

As documented in lib/certdb/certt.h, the "CERTCertTrust *trust" member of CERTCertificateStr is not thread-safe, and is subject to change/be recomputed as tokens are added/removed, potentially affecting how cert trust settings are computed or causing data races.

Comment 1

[Ryan Sleevi]

Attachment: Initial patch

This is an initial patch that adds no new tests, and just ensures that any reads from CERTCertificate->trust are updated to instead of CERT_GetCertTrust. All writes are guarded by CERT_LockCertTrust / CERT_UnlockCertTrust.

Currently, CERT_LockCertTrust / CERT_UnlockCertTrust use a Big Global Lock, but they exist as functions as it was anticipated this can move into a per-cert lock. That will be for further investigation.

Comment 2

[Robert Relyea]

I think the patch is incomplete. There is also a STAN_ChangeCertTrust() function which is sometimes protected, and sometimes not. When it's not, it can change the trust flags without the lock. The corner case is __CERT_AddTempToPerm.

It's probably best to move the locks to inside STAN_ChangeCertTrust().

bob

Comment 3

[Robert Relyea]

Comment on attachment 683885 [details] [diff] [review]
Initial patch

r- I guess my previous comment means the patch deserves an r-, but I believe the rest of the patch is correct. A new patch which corrects the issue or a comment why it's not a problem will get a r+ from me.

bob

Comment 4

[Ryan Sleevi]

Attachment: Patch 2 - Locking review feedback

Also handles the __CERT_AddTempToPerm case.

Comment 5

[Robert Relyea]

Comment on attachment 697748 [details] [diff] [review]
Patch 2 - Locking review feedback

r+ rrelyea

Comment 6

[Ryan Sleevi]

Checking in mozilla/security/nss/cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.166; previous revision: 1.165
done
Checking in mozilla/security/nss/cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.126; previous revision: 1.125
done
Checking in mozilla/security/nss/cmd/multinit/multinit.c;
/cvsroot/mozilla/security/nss/cmd/multinit/multinit.c,v  <--  multinit.c
new revision: 1.4; previous revision: 1.3
done
Checking in mozilla/security/nss/lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.124; previous revision: 1.123
done
Checking in mozilla/security/nss/lib/certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.93; previous revision: 1.92
done
Checking in mozilla/security/nss/lib/certhigh/certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.48; previous revision: 1.47
done
Checking in mozilla/security/nss/lib/certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.79; previous revision: 1.78
done
Checking in mozilla/security/nss/lib/pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.184; previous revision: 1.183
done
Checking in mozilla/security/nss/lib/pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.111; previous revision: 1.110
done
Checking in mozilla/security/nss/lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.196; previous revision: 1.195
done