1028781 - PKCS#12 import probably doesn't work so well after we removed all the configuration for CERT_VerfiyCert since it uses CERT_VerifyCert

Status: NEW

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

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #1028643 +++

nsIX509CertDB.importPKCS12File
  nsNSSCertificateDB::ImportPKCS12File
    nsPKCS12Blob::ImportFromFile
      nsPKCS12Blob::ImportFromFileHelper
        SEC_PKCS12DecoderVerify
          SEC_PKCS7VerifySignature
            sec_pkcs7_verify_signature
              CERT_VerifyCert

Basically, we need to do something similar to what I did for bug 1028643. I invite somebody else to take on this, as I won't have time to do it myself.

Probably the main regression is that OCSP-related stuff isn't quite right any mor. For example, OCSP requests won't respect the Necko proxy configuration, and the prefs for OCSP fetching will be ignored. Perhaps no OCSP will be done at all?
It's not even clear to me that this code should be verifying the certificate chain anyway, and it probably shouldn't be caring about OCSP. Also, almost nobody is importing PKCS#12 files into Firefox, and when people do, it will probably still work. Consequently, it doesn't seem like we have a very urgent need to fix the regression here. Thus, I'm not marking this as blocking bug 975229.

HOWEVER, the work to reduce the size of NSS in Firefox from bug 611781 DOES depend on doing this, because we can't avoid linking CERT_VerifyCert without resolving this bug. Note that CERT_VerifyCert indirectly calls CERT_PKIXVerifyCert which means it will cause all of libpkix to get linked in too.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: cut-out-nsPKCS12Blob.patch

Here is a patch that building or using PKCS12Blob when MOZ_DISABLE_CRYPTOLEGACY is set, as is the case for B2G and Android builds. This should allow you, if you rebase your linker scripts on top of this patch + the patch for bug 1028643 (which just landed in inbound), to get a substantial size savings. I am curious how big the savings actually is.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8444817 [details] [diff] [review]
cut-out-nsPKCS12Blob.patch

Review of attachment 8444817 [details] [diff] [review]:
-----------------------------------------------------------------

Once I fixed the compile errors, the actual number of symbols libxul uses on an Android build was changed only a little (most, but not all of the SEC_PKCS12* functions, and a couple others were removed).  Does nsNSSComponent.cpp:InitializeCipherSuite need to still setup everything for PKCS12 in that build scenario?

By and large, though, it looks like we can't remove those symbols anyway, because we build pk12util, which uses all of the PKCS12 symbols.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: disable PKCS#12 for MOZ_DISABLE_CRYPTOLEGACY

This patch compiles for me on Android

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Once I fixed the compile errors, the actual number of symbols libxul uses on
> an Android build was changed only a little (most, but not all of the
> SEC_PKCS12* functions, and a couple others were removed).  Does
> nsNSSComponent.cpp:InitializeCipherSuite need to still setup everything for
> PKCS12 in that build scenario?
> 
> By and large, though, it looks like we can't remove those symbols anyway,
> because we build pk12util, which uses all of the PKCS12 symbols.

Sorry, I should have been more clear. The point isn't to see how many symbols are reduced, but to measure whether removing, SEC_PKCS12DecoderVerify,  SEC_PKCS7VerifySignature, CERT_Verify*, and CERT_PKIXVerifyCert reduces the size of libnss3 by a substantial amount. It should be very significant. If it isn't, that means there are other indirect callers of CERT_VerifyCert/CERT_VerifyCertificate that need to be removed. To do this experiment, you'd have to temporarily disable the building of all the command-line utilities in order to get a good measurement.

Comment 5

[Nathan Froyd [:froydnj]]

SEC_PKCS7VerifySignature and CERT_VerifyCert are already not exported in the patch for bug 1025998.

Removing SEC_PKCS12DecoderVerify, CERT_VerifyCertificate, and CERT_PKIXVerifyCert does virtually nothing to the size of libnss3.so (~150 bytes or so) on Android.  CERT_VerifyCertName is still required, FWIW.

Applying the patch in this bug on top of bug 1025998 and then relinking libnss3.so with *just* the symbols required for libxul on Android cuts another 15K or so.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: minimal libxul-only nss.def

Here's a minimal approximation of what libxul needs on Android to link successfully.  Do you see any obvious fat to trim?

Comment 7

[Nathan Froyd [:froydnj]]

For reasons that I can't understand, the linker doesn't seem to be eliminating functions like CERT_PKIXVerifyCert that are never called, but visible outside their compilation unit.  I wonder if the interaction between --gc-sections and --version-script is not as complete as we thought it was...but that doesn't explain how adding --version-script to Android managed to trim several hundred K off if it didn't help --gc-sections do its job.  And I don't see any obvious calls to, say, CERT_PKIXVerifyCert within NSS...

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Nathan Froyd (:froydnj) from comment #7)
> For reasons that I can't understand, the linker doesn't seem to be
> eliminating functions like CERT_PKIXVerifyCert that are never called, but
> visible outside their compilation unit.  I wonder if the interaction between
> --gc-sections and --version-script is not as complete as we thought it
> was...but that doesn't explain how adding --version-script to Android
> managed to trim several hundred K off if it didn't help --gc-sections do its
> job.  And I don't see any obvious calls to, say, CERT_PKIXVerifyCert within
> NSS...

I don't have much more time to spend on this, but I will give you a couple of ideas for finding out why CERT_PKIXVerifyCert is being called.

First, note this call stack:

    CERT_VerifyCert
      cert_VerifyCertWithFlags
        CERT_VerifyCertChain
          cert_VerifyCertChain
            cert_VerifyCertChainPkix  

cert_VerifyCertChainPkix is basically a copy of CERT_PKIXVerifyCert. So, you need to eliminate all callers of CERT_Verify* because they all result in similar call stacks. Perhaps one way of doing this is to temporarily hack the NSS source code to delete the definitions (but not declarations) of cert_VerifyCertChainPkix and CERT_PKIXVerifyCert, then relink libnss3, and see what callers cause the linking to fail, and then transitively find those callers.

If the linking succeeds but CERT_PKIXVerifyCert is still being linked in, then I suggest that you experiment by settings CSRCS = $(NULL) in all of the libpkix manifest.mn files (there are tons, because of the many nested subdirectories).

Incidentally, I noticed libpkix's manifest.mn says "export ALLOW_OPT_CODE_SIZE = 1". Perhaps this ends up unintentionally and ironically undoing your optimizations?

Also, look at SSL_AuthCertificate. It isn't called by Gecko, and it isn't called at all, but libssl initializes a pointer to it in ssl_NewSocket:

    ss->authCertificate    = SSL_AuthCertificate;

Try #ifdefing the call to CERT_VerifyCert in SSL_AuthCertificate out. (In the long run, you'll need to write a patch to NSS to allow a compile-time flag to control this.)

That's really all I know off the top of my head. Sorry I can't help more.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8446530 [details]
minimal libxul-only nss.def

I don't see anything that is likely to have anything close to the impact of removing libpkix. It would be good to remove our uses of CERT_FindCertIssuer, CERT_CertChainFromCert, and CERT_GetCertChainFromCert, which might result in a non-trivial win from removing the classic (non-libpkix) cert chain building code, but I suspect that we'd have to deal with internal uses of them within NSS too. The main thing to do is to figure out why libpkix is being linked in. And/or experiment with making sure libpkix never even gets compiled to verify that it is actually a big enough win to matter.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #8)
> (In reply to Nathan Froyd (:froydnj) from comment #7)
> > For reasons that I can't understand, the linker doesn't seem to be
> > eliminating functions like CERT_PKIXVerifyCert that are never called, but
> > visible outside their compilation unit.  I wonder if the interaction between
> > --gc-sections and --version-script is not as complete as we thought it
> > was...but that doesn't explain how adding --version-script to Android
> > managed to trim several hundred K off if it didn't help --gc-sections do its
> > job.  And I don't see any obvious calls to, say, CERT_PKIXVerifyCert within
> > NSS...
> 
> I don't have much more time to spend on this, but I will give you a couple
> of ideas for finding out why CERT_PKIXVerifyCert is being called.

Thanks for the ideas.  It's not being called at all; one can comment out its definition and relink libnss3.so with no problems.  And the linkers for different Android toolchains eliminate different amounts of symbols with --gc-sections, so there's clearly something going wrong there.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #9)
> I don't see anything that is likely to have anything close to the impact of
> removing libpkix. It would be good to remove our uses of
> CERT_FindCertIssuer, CERT_CertChainFromCert, and CERT_GetCertChainFromCert,
> which might result in a non-trivial win from removing the classic
> (non-libpkix) cert chain building code, but I suspect that we'd have to deal
> with internal uses of them within NSS too.

Thanks for the once-over.  Looks like the CERT_* functions you suggest getting rid of stem from nsNSSCertificate and the nsIX509Cert* interfaces we expose to addons.

libpkix clocks in at around 300k of ARM code, or ~25% of the size of libnss3.so on Android.  Seems worth removing, but trying to go through and flush out all the (indirect) security/ uses of it, plus all the NSS build gymnastics, seems quite difficult.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> libpkix clocks in at around 300k of ARM code, or ~25% of the size of
> libnss3.so on Android.  Seems worth removing, but trying to go through and
> flush out all the (indirect) security/ uses of it, plus all the NSS build
> gymnastics, seems quite difficult.

The thing is, the linker script you posted should remove all of them already.

Are you using -ffunction-section? It shouldn't matter if you fix the SSL_AuthCertificate issue, which I think may be the only remaining caller of CERT_VerifyCert that you are linking in.

Also, see https://sourceware.org/bugzilla/show_bug.cgi?id=12975.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #12)
> Are you using -ffunction-section? It shouldn't matter if you fix the
> SSL_AuthCertificate issue, which I think may be the only remaining caller of
> CERT_VerifyCert that you are linking in.
> 
> Also, see https://sourceware.org/bugzilla/show_bug.cgi?id=12975.

Note that lack of -ffunction-section + SSL_AuthCertificate calling CERT_VerifyCert + CERT_PKIXVerifyCert being in the same file as cert_VerifyCertChainPkix would explain why CERT_PKIXVerifyCert remains.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #12)
> (In reply to Nathan Froyd (:froydnj) from comment #11)
> > libpkix clocks in at around 300k of ARM code, or ~25% of the size of
> > libnss3.so on Android.  Seems worth removing, but trying to go through and
> > flush out all the (indirect) security/ uses of it, plus all the NSS build
> > gymnastics, seems quite difficult.
> 
> The thing is, the linker script you posted should remove all of them already.
> 
> Are you using -ffunction-section? It shouldn't matter if you fix the
> SSL_AuthCertificate issue, which I think may be the only remaining caller of
> CERT_VerifyCert that you are linking in.

Yes: nss/lib/certhigh/certvfypkix.o has multiple .text sections.

> Also, see https://sourceware.org/bugzilla/show_bug.cgi?id=12975.

Ah, thanks for that links.  That would explain why the 4.7 toolchain's ld (2.22.90.20120727) does a better job than the 4.6 toolchain's ld (2.21), but it still doesn't explain why CERT_PKIXVerifyCert is still there.  Maybe the fix for that PR didn't go far enough.