651523 - Remove SSL step-up code from libssl

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

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

The SSL step-up code uses CERT_VerifyCertNow instead of libpkix. Rather than rewriting the code to use libpkix and/or providing an option to do so, I propose that we just provide a way to disable the step-up support at build time (using #ifdefs), as a step towards removing it entirely.

Comment 1

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

See bug 476807. Like rrelyea said last Thursday, the trust bits for SSL step-up have been removed from all the built-in certs. Removing the step-up code would remove one of the two dependencies that libssl has on the non-libpkix certificate path building code (the other is client auth; see bug 658846 and the usage of CERT_Find* functions in authcert.c & cmpcert.c).

Comment 2

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

The step-up code appears to do a renegotiation irrespective of the setting of the secure renegotiation options and regardless of whether the server supports secure renegotiation.

Comment 3

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

Attachment: Remove SSL Step-up support.

This bug removes the step-up code, in particular the CERT_VerifyCert calls that are getting in the way of some of my other work.

Comment 4

[Robert Relyea]

Comment on attachment 546462 [details] [diff] [review]
Remove SSL Step-up support.

r+ rrelyea.

NOTE: this also makes setting Export and France policies no-ops. I think this is OK.

My one worry is the removal of the rehandshake field. It appears it's only needed for step up, which is why I r+'ed, but I'm definately depending on the fact that Brian compiled this code with  no errors.

What is this change OK and stepdown is not? The answer is we've long ago turned off the setup up bits. There is still the issue of servers running stepup (they don't need the bits. The only issue here could be the change to cert_ComputeCertType(), which would treat take a cert that only had the setup oid and not recognize it as an SSL cert. This may also affect the client, though my guess is that such a cert is highly unlikely. I would be OK with not including the certdb.c if we need to be more risk adverse.

bob

Comment 5

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

Attachment: Remove step-up code, v2: remove EXPORT_VERSION

Here is an updated patch that removes support for EXPORT_VERSION. If EXPORT_VERSION had been defined, then NSS_SetDomesticPolicy would call NSS_SetExportPolicy. But, my patch changes NSS_SetExportPolicy to call NSS_SetDomesticPolicy, so that would result in infinite recursion.

Comment 6

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

Attachment: Remove step-up code, v3: remove EXPORT_VERSION build option, unbitrotted

Comment 7

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

Attachment: Remove step-up code, v4: remove EXPORT_VERSION build option, unbitrotted (fixed typo)

Comment 8

[Robert Relyea]

Comment on attachment 570437 [details] [diff] [review]
Remove step-up code, v4: remove EXPORT_VERSION build option, unbitrotted (fixed typo)

r+ rrelyea

Comment 9

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

Checking in mozilla/security/nss/lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.117; previous revision: 1.116
done
Checking in mozilla/security/nss/lib/certhigh/certvfypkix.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfypkix.c,v  <--  certvfypkix.c
new revision: 1.48; previous revision: 1.47
done
Checking in mozilla/security/nss/lib/pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.103; previous revision: 1.102
done
Checking in mozilla/security/nss/lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.154; previous revision: 1.153
done
Checking in mozilla/security/nss/lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.86; previous revision: 1.85
done
Checking in mozilla/security/nss/lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.78; previous revision: 1.77
done
Checking in mozilla/security/nss/lib/util/pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.24; previous revision: 1.23
done

Comment 10

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

The patch was backed out today because I turned tinderbox red with another patch on 11/11/11. I have checked this patch back in, unmodified:

Checking in lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.120; previous revision: 1.119
done
Checking in lib/certhigh/certvfypkix.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfypkix.c,v  <--  certvfypkix.c
new revision: 1.50; previous revision: 1.49
done
Checking in lib/pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.105; previous revision: 1.104
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.157; previous revision: 1.156
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.89; previous revision: 1.88
done
Checking in lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.80; previous revision: 1.79
done
Checking in lib/util/pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.26; previous revision: 1.25
done

Comment 11

[Wan-Teh Chang]

For completeness, it seems that the use of CERTDB_GOVT_APPROVED_CA
in NSS should also be removed.  Or is CERTDB_GOVT_APPROVED_CA different
from a step-up cert?