Opened 8 years ago

Closed 3 years ago

#2949 closed defect (fixed)

Make Intermediate Cert Store Memory-Only for TorBrowser

Reported by: mikeperry Owned by: mikeperry
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: MikePerryIteration20110515
Cc: lunar@…, erinn Actual Points: 5
Parent ID: #2877 Points: 3
Reviewer: Sponsor:

Description

User stored certs as well as the intermediate certificate store should be memory-only by default in TorBrowser. This should be easy for user certs. No so sure about intermediate ones. Need to review the relevant code first.

Child Tickets

Attachments (2)

nocertdb.diff (8.9 KB) - added by mikeperry 8 years ago.
Patch against FF4.0
nocertdb.2.diff (11.5 KB) - added by mikeperry 8 years ago.
Updated patch against FF4.0. Implements observer for pref, but we still want a notify eventually.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by mikeperry

Component: - Select a componentTor Browser
Owner: set to mikeperry
Status: newaccepted

comment:2 Changed 8 years ago by lunar

Cc: lunar@… added

comment:3 Changed 8 years ago by mikeperry

Looks like the intermediate cert store is in cert8.db, which appears to be opened by https://mxr.mozilla.org/mozilla2.0/source/security/nss/lib/softoken/legacydb/lginit.c#360

It looks like we may be able to control the use of the db file via a parameter in nss_init:
https://mxr.mozilla.org/mozilla2.0/source/security/nss/lib/nss/nssinit.c#525

NSS_INIT_NOCERTDB seems to be the flag we want, and the NSS init appears to be called from nsNSSComponent::InitializeNSS(). It looks like we must hardcode this flag ourselves. But it also looks like a one-line patch for us (though adding an about:config option might make it a few lines).

It's not clear if this will explode everything or not. We'll need to test this and see what happens.

comment:4 Changed 8 years ago by mikeperry

Keywords: MikePerryIteration20110515 added
Points: 3

I imagine it is going to be a fun experience to try to build Firefox and learn to create a proper mercurial/git-formatted patch... otherwise this should be simple.

comment:5 Changed 8 years ago by mikeperry

Tested and this hack does in fact still allow NSS to cache intermediate certs in memory.

Can't seem to find a way to purge that cache yet, though.. Unlike #2950, it is not as simple as re-initializing NSS, sadly.

comment:6 Changed 8 years ago by mikeperry

Actual Points: 4
Resolution: fixed
Status: acceptedclosed

Ok, I banged on this for a while and can't seem to get it to work right. It is easy to make the intermediate cert store and other NSS dbs memory-only, but it does not appear easy to clear them at will.

It looks like Firefox's use of NSS is a crazy minefield of thread-safe and thread-unsafe accesses. I have some code that successfully re-initializes and clears NSS, but if any tabs are open with SSL pages in them, we get random segfaults and crashes.. I'm going to leave the code in the patch, but it is not run.

The pref needs to be set to the desired value at browser initialization, and cannot be toggled by Torbutton.

Changed 8 years ago by mikeperry

Attachment: nocertdb.diff added

Patch against FF4.0

comment:7 Changed 8 years ago by mikeperry

The pref name is 'security.nocertdb'. If set to true, no NSS data will get written to disk (including client certificates and PKCS tokens).

comment:8 Changed 8 years ago by mikeperry

Note that the code that causes crashes is essentially the same codepath that Firefox itself uses during the profile switching event series. It may be the case that we actually need all SSL tabs to be closed for us to be able to shutdown and re-initialize NSS. In which case, we can remove the early return from this patch and still use it to clear cached certs in our implementation of "New Identity" (which would close all tabs as part of its mechanisms). But we'll want to test extensively that to make sure the race still isn't possible.

comment:9 Changed 8 years ago by mikeperry

More notes to the future: It is possible that it is crashing in the cases where the profile change event would have been vetoed by the NSSComponent. I did not look into this as these cases are currently not logged. I actually altered the patch to avoid those codepaths entirely, because the prefchange is not vetoable like the profile change event is. If we could somehow create a blocking or cancel notification to our own new-identity observer event, this may be possible to do safely this way.

comment:10 Changed 8 years ago by mikeperry

Actual Points: 45

This kept bugging me so I banged on it some more to make the pref mimic an observer notification. I think I got it to stop crashing by checking to see if the component wanted to cancel shutdown, but the whole SSL system gets put into a broken state after a cancel, until such time as you try to shut it down again and then restart it. The cancel seems to be happening due to a long period of time between tab close and underlying SSL connection close.

So the user experience for a New-Identity based on this may not be good, unless we find a way to forcibly close any open SSL connections early.. But at least it seems to have stopped crashing.

Updating the patch.

Changed 8 years ago by mikeperry

Attachment: nocertdb.2.diff added

Updated patch against FF4.0. Implements observer for pref, but we still want a notify eventually.

comment:11 Changed 8 years ago by mikeperry

See #2739 for the bug to work this into something that can be used for "New Identity"

comment:12 Changed 8 years ago by erinn

Cc: erinn added

comment:13 Changed 4 years ago by cypherpunks

If this ticket is about certs then why patch disables all dbs intead only one cert.db ? Why need to init nss using NSS_NoDB_Init instead of NSS_Initialize with NSS_INIT_NOCERTDB flag?

comment:14 Changed 4 years ago by cypherpunks

Component: Firefox Patch IssuesTor Browser
Resolution: fixed
Severity: Normal
Status: closedreopened

If this ticket is about certs then why patch disables all dbs intead only one cert.db ? Why need to init nss using NSS_NoDB_Init instead of NSS_Initialize with NSS_INIT_NOCERTDB flag?

comment:15 Changed 3 years ago by bugzilla

Resolution: fixed
Status: reopenedclosed

Please, don't reopen tickets to ask a question.

Note: See TracTickets for help on using tickets.