1566191 - [meta] track not decoding certificate information in the content process(es)

Status: NEW

(Core :: Security: PSM, task, P3)

Description

[:Gijs (he/him)]

Per the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1559878#c17, it seems that we currently decode certificates in both the parent and the child.

Off-hand, it's not entirely clear to me (or, afaict, Dana or JC) why the child would care about this information (except maaaaybe error pages, but they could have their own mechanism when we actually want the data, most of which is normally behind an expander anyway). Although we still need NSS for webcrypto, not decoding certs would potentially let us avoid:

• loading NSS in the child until/unless there's an actual webcrypto call
• using memory to do cert bookkeeping in the child
• spending CPU decoding certs for every network connection

Where we do need decoded cert information, we could send for the information from the parent.

We don't currently have a good grasp of the potential memory/CPU savings here. Dana, on slack you said changing PSM to not create cert objects in the content process should be fairly straightforward; could you point to where such a change would want to be made?

Eric/Kris, is this something that you'd consider for memshrink efforts?

Comment 1

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

First step would be to update nsNSSCertificate and nsNSSCertList to have the default restriction of ParentProcessOnly here: https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/security/manager/ssl/nsNSSModule.cpp#134 and see what breaks. If we're really lucky, nothing will. If we're slightly less lucky, we might have to bring back child-process-specific implementations of those two classes that stub out all of the functionality and just implement ::Read and ::Write (where ::Read won't actually create a certificate - it'll read and save the bytes of the certificate until ::Write gets called). If we're even less lucky, we'll have to take a closer look at what needs to change.

Comment 2

[Eric Rahm [:erahm]]

Just from a binary perspective we're looking at something like 20K per process for unshared lib data (.data + .bss + .data.rel.ro) on Linux. That doesn't include anything that gets allocated during NSS initialization. We've also seen issues with loading the cert store (it's not clear to me if we'd avoid that here) that showed several megabytes of data being loaded by rkvstore (bug 1538093).

So yes, from a MemShrink perspective I'd give this a thumbs up.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Eric Rahm [:erahm] from comment #2)

Just from a binary perspective we're looking at something like 20K per process for unshared lib data (.data + .bss + .data.rel.ro) on Linux.

I doubt we're going to be able to improve that part. Even if we don't initialize NSS in every child process, we're still going to need to load it. We use way too many NSS symbols in way too many places to try to load it dynamically.

It would be good to know how much memory we can save by just not initializing it, though.

Comment 4

[Sean Feng [:sefeng]]

Thanks Dana for the help, these are some updates to the bug.

Simply changing nsNSSCertificate and nsNSSCertList to be parent process only won't work because some code in content processes require nsITransportSecurityInfo to exist to check some of the properties, for example here would use mState and here requires the entire certificate to be existed.

As the next step, what I am currently working is instead of decoding the certificate and doing all the validity checks in the content process, we keep the certificate encoded in raw bytes and make the parent process to check all the validity.

Comment 5

[Sean Feng [:sefeng]]

Attachment: Bug 1566191 - Remove nsIX509CertList and use nsTArray<nsIX509Cert> instead

Comment 6

[Sean Feng [:sefeng]]

Attachment: Bug 1566191 - Update test cases to adopt new certList format

Depends on D43267

Comment 7

[Sean Feng [:sefeng]]

Attachment: Bug 1566191 - Remove nsNSSCertList/nsIX509CertList

Comment 8

[Sean Feng [:sefeng]]

Attachment: Bug 1566191 - Update some APIs to adapt new certList format

Depends on D43494

Comment 9

[Sean Feng [:sefeng]]

Attachment: Bug 1566191 - Update certChains to use nsTArray<nsIX509Cert>

Depends on D43495

Comment 10

[Sean Feng [:sefeng]]

Attachment: Bug 1566191 - Update signatureInfo to Array of Array of Bytes

Depends on D43496

Comment 11

[Sean Feng [:sefeng]]

Attachment: Bug 1566191 - Remove nsIX509CertList from some ssl services

Depends on D43497

Comment 12

[Sean Feng [:sefeng]]

Attachment: Bug 1566191 - Update signatureInfo to Array of Array of Bytes r=keeler

The signatureInfo that has been used in ExternalHelperAppService and
ReputationService has been stored Array of nsIX509CertList, which
isn't necessary because only the raw bytes of the certs are required.
This patch intends to remove the usage of nsIX509CertList and store
the raw bytes directly.

Depends on D44242

Comment 13

[Phabricator Automation]

Comment on attachment 9089498 [details]
Bug 1566191 - Update signatureInfo to Array of Array of Bytes r=keeler

Revision D44243 was moved to bug 1577836. Setting attachment 9089498 [details] to obsolete.

Comment 14

[Vicky Chin [:vchin]]

Work in progress but targeting 72.

Comment 15

[Chris Peterson [:cpeterson]]

The Fission team would like to fix this bug before we ship Fission MVP. We want to avoid loading NSS in content processes because, IIUC, we shouldn't need to read certs in content process.

Comment 16

[Neha Kochar [:neha]]

I believe Randell said (during Fission bug triage today) that he would be able to do this himself.

Comment 17

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

BTW, the memory hit I see for NSS init in Content Processes (right after RecvConstructBrowser) is ~1.5MB -- a LOT in fission

Comment 18

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

Note that NSS is responsible for more than just certificates. For instance, hashing data generally requires NSS (except when using rust) and webcrypto heavily relies on NSS. It would be great to separate those parts of NSS and make them statically available, but that would be a significant amount of work.

Comment 19

[Chris Peterson [:cpeterson]]

This work doesn't need to block Fission MVP, so moving to Fission Milestone Future.