435159 - nsNSSCertificateDB::DeleteCertificate has race conditions

Status: RESOLVED DUPLICATE of bug 1267861

(Core :: Security: PSM, defect)

Description

[Mike Perry]

I'm implementing SSL certificate isolation for Torbutton, where client, server, and CA SSL certificates for different Tor states are written out to disk and kept separate to minimize the ability of websites and exit nodes from probing to see if a client has a certificate for a certain corporation, website, or client certificate installed.

The code works like this:
1. Go through the nsICertTree tree views and record all the user-added ssl certs for each cert type.
2. Write these out to disk using the BER binary format to a file for this tor state.
3. call nsIX509CertDB::deleteCertificate() on each one
4. Import in certificates saved in the opposite tor state from that BER file on disk

I've managed to get it working in the general case, but it appears that there is a race condition on certificate deletion. It appears as if nsIX509Cert::deleteCertificate doesn't actually delete the certificates from the DB, but only lazily marks them for later deletion. This is actually fine, but the problem is that when the same certs are re-inserted before the deletion actually takes place (like for example, if the user toggles tor back and forth real fast, or if they have the same cert in both jars), then the whole process fails due to the duplicate cert. Worse still, this duplicate cert is then later deleted because it is still marked for deletion.

A fix for this would be a check during each of the nsNSSCertificateDB::Import* functions to see if the cert already exists but has the nsNSSCertificate::mPermDelete flag set. If so, these functions should merely clear the flag instead of complaining that the cert already exists.

Furthermore, iterations over all the current certs in a nsICertTree treeView should skip certs with this mPermDelete flag set (looks like this check should be done in nsCertTree::GetCertsByTypeFromCertList).

Comment 1

[Daniel Veditz [:dveditz]]

This isn't going to block one of our security/stability branch releases, but it is worth considering once it's fixed on the trunk

Comment 2

[shawn.sumin]

The Tor Project / Electronic Frontier Foundation is paying to have this bug fixed.

"If you know C++ and/or Firefox internals, we should be able to pay you for your time to address these issues and shepherd the relevant patches through Mozilla's review process."

Source: https://blog.torproject.org/blog/web-developers-and-firefox-hackers-help-us-firefox-4