|
Description
Kai Engert (:kaie) (inactive account)
2016-01-12 20:06:50 UTC
Karel asked for a list of changes in the newer NSS, here's a summary: Previously, we used NSS 3.19.x. The relevant changes that were added in version until 3.21 are: - CA certificates (list was updated twice, in 3.19.3 and 3.21) - Added support for DHE server side ciphersuites, disabled by default. (This was already included in RHEL 7 with local patches.) - disabled support for very old C compilers (pre C89) - support TLS extended master secret extension (RFC 7627), off by default - several other new APIs (that new application code could use) - upstream changed to build with ECC enabled by default - stricter build options, that cause most warnings to be treated as errors More details here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.19.3_release_notes https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.20_release_notes https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.21_release_notes Oops, wrong bug. Please see bug 1297888 comment 4 (attachment 1116177 [details]), which is the patch to keep the legacy CA certificates enabled. It should apply cleanly on top of NSS 3.21 (In reply to Kai Engert (:kaie) from comment #2) > - support TLS extended master secret extension (RFC 7627), off by default We cannot support this feature because of the older softokn that we're shipping with RHEL. We will force this feature to remain disabled. Created attachment 1123591 [details]
all changes for rebase V1 - work in progress
Posting of work in still in progress so I can discuss with Bob how to properly update the SHA384 PRF support patch to the state of the nss-3.21 sources. The tests suite doesn't pass yet.
Created attachment 1127046 [details]
all changes for rebase V2 - work in progress
This is difficult to review and it's sole purpose to make it easy to quickly to apply all changes via patch -p1 < allchangesForRhel72Rebase.patch. For the review I'll attach the individual patches. It's still a work in progress as some regression tests are failing.
Notes I took while studying the upstream changes made in going from NSS 3.19 To NSS 3.21 (nss/lib/ssl) which I may help in the review.
from sslimpl.h:
old extern SECStatus ssl3_SendECDHServerKeyExchange(sslSocket *ss,
const SSL3SignatureAndHashAlgorithm *sigAndHash);
new extern SECStatus ssl3_SendECDHServerKeyExchange(
sslSocket *ss, const *sigAndHash);
changes the type of the last parameter: SSL3SignatureAndHashAlgorithm -> SSLSignatureAndHashAlg
from ssl3prot.h:
old: typedef struct {
SECOidTag hashAlg;
TLSSignatureAlgorithm sigAlg;
} SSL3SignatureAndHashAlgorithm;
removed
int ->
The old typedef struct {
unsigned int len;
SECOidTag hashAlg; ------- enum defined in secoidt.h
> SEC_OID_UNKNOWN = 0, SEC_OID_MD2 = 1, SEC_OID_MD4 = 2, SEC_OID_MD5 = 3,
> SEC_OID_SHA1 = 4, ... SEC_OID_SHA256 = 191, SEC_OID_SHA384 = 192, SEC_OID_SHA512 = 193,
union {
PRUint8 raw[64];
SSL3HashesIndividually s;
} u;
} SSL3Hashes;
has changed to
typedef struct {
unsigned int len;
SSLHashType hashAlg; --------- an enum defined in sslt.h
> ssl_hash_none = 0, ssl_hash_md5 = 1, ssl_hash_sha1 = 2, ssl_hash_sha224 = 3,
> ssl_hash_sha256 = 4, ssl_hash_sha384 = 5, ssl_hash_sha512 = 6
union {
PRUint8 raw[64];
SSL3HashesIndividually s;
} u;
} SSL3Hashes;
Do we have conversion functions among SECOidTag <-> SSLHashType?
Do we need them?
ssl3_MasterKeyDeriveBypass renamed ssl3_MasterSecretDeriveBypass
ssl3_SendECDHServerKeyExchange changed the type of first argument
from SECOidTag hashAlg to SSLHashType hashAlg
Changed extern SECOidTag ssl3_TLSHashAlgorithmToOID(int hashFunc);
to extern SECOidTag ssl3_TLSHashAlgorithmToOID(SSLHashType hashFunc);
Added extern PRBool ssl3_IsSupportedSignatureAlgorithm(const SSLSignatureAndHashAlg *alg);
sslenum.c:
Added
TLS_DHE_DSS_WITH_AES_128_GCM_SHA256, TLS_DHE_DSS_WITH_AES_128_CBC_SHA256, and TLS_DHE_DSS_WITH_AES_256_CBC_SHA256 which we already had added downstream
and where we have different order than upstream.
ssl3con.s:
static int ssl3_OIDToTLSHashAlgorithm(SECOidTag oid); removed
Added the three ciphers above, first enabled the other two not enabled.
Added static const SSLSignatureAndHashAlg defaultSignatureAlgorithms[] array
Removed static const PRUint8 supported_signature_algorithms[] = {
on static const ssl3KEADef kea_defs[]
-------------------------------------------
This struct has been modified for RHEL-7.2 and is not yet upstream
ssl3CipherSuiteDef {
typedef struct ssl3CipherSuiteDefStr {
ssl3CipherSuite cipher_suite;
SSL3BulkCipher bulk_cipher_alg;
SSL3MACAlgorithm mac_alg;
SSL3KeyExchangeAlgorithm key_exchange_alg;
SSL3PRF pr_alg; ---------------------- new
} ssl3CipherSuiteDef;
and downstream we added this enum
/* The TLS PRF definition */
typedef enum {
prf_null = 0, /* use default prf */
prf_256 = CKM_SHA256,
prf_384 = CKM_SHA384
} SSL3PRF;
which I will keep.
Several internal functions no longer take a
intenal ssl3_GetPrfHashMechanismFromSocket(sslSocket *ss) - not yet upstream
esentially does
return (ss->ssl3.hs.suite_def->prf_alg == 0)
? CKM_SHA256 : ss->ssl3.hs.suite_def->prf_alg;
Problem is that we need to start with pointer to an sslSocket and several
functions that took such pointer no longer do so I would to modify them
to take such a parameter.
ssl3_SendCertificateRequest(sslSocket *ss) is a problematic function
The problem is that sigAls is now an array
PRUint8 sigAlgs[MAX_SIGNATURE_ALGORITHMS * 2];
instead of a PRUint8 * which required modifying one of the patches substantially.
Created attachment 1127076 [details]
changes to nss.spec - in patch format
Let's start with the spec file. Some explanations are in order.
modified: Bug-1001841-disable-sslv2-libssl.patch and
Bug-1001841-disable-sslv2-tests.patch
deleted: fix-disable-sslv2-libssl.patch and
fix-disable-sslv2-tests.patch
since I merged the corrections two onto the previous two patches they fix
deleted: additional-cipher-suites-enabled-by-default.patch
Not needed as dhe-sha384-dss-support.patch takes acre of this.
new file: client_auth_for_sha384_prf_support.patch
The changes dealing with client authentication are now in their own patch
as Bob suggested in an upstream review.
deleted: cve-2015-7575-minimal.patch
This is rendered obsolete with the rebase to nss-3.21.
modified: dhe-sha384-dss-support.patch
Substatially modifie for the rebase and some other patches merged into it.
new file: disable-extended-master-secret-with-old-softoken.patch
The same patch that Kai added for the rhel-6.7.z rebase work
modified: enable-ecdsa-ciphers-by-default.patch - due to rebase
modified: enable-fips-when-system-is-in-fips-mode.patch - due to rebase
modified: fix-min-library-version-in-SSLVersionRange.patch - due to rebase
modified: iquote.patch - due to rebase
modified: nss-539183.patch - due to rebase
modified: nss.spec - due to rebase
modified: p-ignore-setpolicy.patch
new file: pem-compile-with-Werror.patch
needed due to nss-3.21 where warnings are treated as errors
deleted: prfnonsha256.patch
merged into dhe-sha384-dss-support.patch
modified: reorder-cipher-suites.patch
It should be deleted as reordering is now part of dhe-sha384-dss-support.patch
deleted: sha384-client-verify.patch
merged into dhe-sha384-dss-support.patch
modified: ssl-server-min-key-sizes.patch
and also applied earlier
deleted: sslinfo-fix-info.patch
merged into dhe-sha384-dss-support.patch
Notice some reordering to move application of some of the easy patches before the more difficult ones.
Created attachment 1127077 [details]
Support TLS 1.2 PRF with SHA-384 as the hash function
This is the patch that was modified the most. Some changes moved to a separate patch.
Created attachment 1127081 [details] client authentication changes due to sha384 support This was for me the most difficult part where I'll need a lot of advise. It's worth having it as a patch on its own for ease of review and also for reasons Bob mentioned upstream in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=923089&attachment=8632467 Created attachment 1127082 [details] nss.spec after changes in attachment 1127076 [details] are applied Created attachment 1127326 [details]
disable ssl2 & export ciphers but allow the ..._RSA_WITH_NULL_... ones
corrections from old fix-disable-sslv2-libssl.patch merged into the patch
Created attachment 1127331 [details]
Skip ssl2 and export ciphers tests
corrections from fix-disable-sslv2-tests.patch merge into the patch
part, modification of two test data files, is done dynamically on nss.spec
Created attachment 1127342 [details]
disable extended master secret support with currently shiping softoken lacks support for
Essentially the same patch you made for rhel-6, I see #if 0 and wonder if #if (0) is preferable.
Created attachment 1127359 [details]
test failures
Result of 'grep FAILED results_nss/3.21.0/0.1.el7.test.1/build.log' and there is a pattern to the failures being that the client is using traditional ciphers.
Comment on attachment 1127326 [details]
disable ssl2 & export ciphers but allow the ..._RSA_WITH_NULL_... ones
It was decided that we must keep it possible to enable SSL v2 hellos. Please adjust the patch accordingly.
(In reply to Elio Maldonado Batiz from comment #15) > I see #if 0 and wonder if #if (0) is preferable. I prefer "#if 0" The NSS code already uses "#if 0" at many places. I've never seen code using "#if (0)" (In reply to Kai Engert (:kaie) from comment #17) > Comment on attachment 1127326 [details] > disable ssl2 & export ciphers but allow the ..._RSA_WITH_NULL_... ones > > It was decided that we must keep it possible to enable SSL v2 hellos. Please > adjust the patch accordingly. My comment doesn't apply to RHEL 7.x, sorry. RHEL 7.x always had SSL-v2 and v2-hellos disabled ALREADY since 7.0 I was confused. Your review led to me believe this patch is a new patch. Instead, you simply adjusted your patch. Hubert, what is your comment on Bob's/Elio's suggestion to disable export ciphersuites on RHEL 7.3 ? People, I'm a bit worried about the strategy you're using for this bug. I believe we have tight timing requirements for RHEL 7.2.z. Currently, we don't have a clone for 7.2.x yet. I think that the SSL-v2-disable-on-7.3 shouldn't block the rebase work for 7.2 I'd like to suggest that the SSL-v2-disable-stuff work gets moved to an independent bug. And given that SSL v2 is already disabled on RHEL 7.x, I don't understand why you are working on it right now. How can we get the rebase for 7.3 and the 7.2.z rebase bug cloning accelerated? Haven't we already disabled export ciphers in RHEL-7.3? But, yes, for RHEL-7.3 I'm ok with disabling export ciphers. I'm keeping things they way we had them earlier, per the last release on rhel-7.2, and the patches I have submitted for review are intended for rhel-7.2.z so we get this bug on modified and it gets cloned for rhel-7.2.z stream. Any changes for rhel-7.3 can be done at a later date. Comment on attachment 1127082 [details] nss.spec after changes in attachment 1127076 [details] are applied Please give me a diff from the previous .spec file (In reply to Bob Relyea from comment #23) > Comment on attachment 1127082 [details] > nss.spec after changes in attachment 1127076 [details] are applied > > Please give me a diff from the previous .spec file https://bugzilla.redhat.com/attachment.cgi?id=1127076&action=diff Comment on attachment 1127326 [details]
disable ssl2 & export ciphers but allow the ..._RSA_WITH_NULL_... ones
If possible, please reuse the same patch that you had used before, and don't make any new changes for this bug.
Is that possible?
Comment on attachment 1127331 [details]
Skip ssl2 and export ciphers tests
If possible, please reuse the same patch that you had used before, and don't make any new changes for this bug.
Is that possible?
(In reply to Kai Engert (:kaie) from comment #25) > Comment on attachment 1127326 [details] > disable ssl2 & export ciphers but allow the ..._RSA_WITH_NULL_... ones > > If possible, please reuse the same patch that you had used before, and don't > make any new changes for this bug. > Is that possible? I'm reusing it. It is still Patch52: Bug-1001841-disable-sslv2-libssl.patch I just modified with the corrections you gave me and and we had in # Fix Patch52 which caused NULL ciphers failures Patch105: fix-disable-sslv2-libssl.patch (In reply to Kai Engert (:kaie) from comment #26) > Comment on attachment 1127331 [details] > Skip ssl2 and export ciphers tests > > If possible, please reuse the same patch that you had used before, and don't > make any new changes for this bug. > Is that possible? It is possible and I'm reusing the patch Patch53: Bug-1001841-disable-sslv2-tests.patch modified by incorporating the needed corrections you had identified and were being applied vua the old # Fix flaws in Patch53 which caused needed tests to be skipped which could hide errors # on implementing the fix we already have a patch for. What is that fix? Patch104: fix-disable-sslv2-tests.patch (In reply to Elio Maldonado Batiz from comment #15) > Created attachment 1127342 [details] > disable extended master secret support with currently shiping softoken lacks > support for > > Essentially the same patch you made for rhel-6, I see #if 0 and wonder if > #if (0) is preferable. This patch seems wrong to me. On top of the rhel-6 patch, you added the following changes: (a) @@ -928,7 +932,11 @@ SSL_OptionGet(PRFileDesc *fd, PRInt32 wh case SSL_ENABLE_EXTENDED_MASTER_SECRET: - on = ss->opt.enableExtendedMS; break; +#if 0 +/* No-Op until we have a compatible softokn. */ + ssl_defaults.enableExtendedMS = on; +#endif + break; This doesn't make sense to me: - we don't need to modify the SSL_OptionGet function - instead of disabling the existing assignment code, you changed it to different assignment code, that you have obviously copied from a different function. - as a result of your change, the function won't assign a value to the output parameter, and the caller will obtain a random value. Please exclude this change from the patch. (b) @@ -1002,7 +1010,10 @@ SSL_OptionGetDefault(PRInt32 which, PRBo case SSL_ENABLE_EXTENDED_MASTER_SECRET: +#if 0 +/* No-Op until we have a compatible softokn. */ on = ssl_defaults.enableExtendedMS; +#endif break; This doesn't make sense to me: - we don't need to modify the SSL_OptionGetDefault function - as a result of your change, the function won't assign a value to the output parameter, and the caller will obtain a random value. Please exclude this change from the patch. Comment on attachment 1127326 [details] disable ssl2 & export ciphers but allow the ..._RSA_WITH_NULL_... ones (In reply to Elio Maldonado Batiz from comment #27) > (In reply to Kai Engert (:kaie) from comment #25) > > Comment on attachment 1127326 [details] > > disable ssl2 & export ciphers but allow the ..._RSA_WITH_NULL_... ones > > > > If possible, please reuse the same patch that you had used before, and don't > > make any new changes for this bug. > > Is that possible? > > I'm reusing it. It is still > Patch52: Bug-1001841-disable-sslv2-libssl.patch > I just modified with the corrections you gave me and and we had in > # Fix Patch52 which caused NULL ciphers failures > Patch105: fix-disable-sslv2-libssl.patch Thanks for clarifying. I've compared this new patch with the previous version of file Bug-1001841-disable-sslv2-libssl.patch With the old version, NSS_NO_SSL2 had the effect to disable all export ciphers. With the new version, NSS_NO_SSL2 has the effect to disable most export ciphers, but keeps the NULL ciphers enabled. So, I'm sorry for previously wondering why you're disabling export cipher suites. They had always been disabled on RHEL 7, and you're simply fixing what we disable. (I had been misled by the attachment name, sorry for that, I should have looked in detail, instead of making assumptions based on the attachment name. It would have been less confusing for me, if this attachment had been named something like "fix patch Bug-1001841-disable-sslv2-libssl.patch to no longer disable NULL export ciphersuites" ) Created attachment 1127740 [details] disable extended master secret support with currently shiping softoken - V2 Addresses Kai's review comments in Comment 29. Comment on attachment 1127359 [details]
test failures
I'm no longer seeing these failures.
Comment on attachment 1127740 [details]
disable extended master secret support with currently shiping softoken - V2
r+
Comment on attachment 1127331 [details] Skip ssl2 and export ciphers tests (In reply to Elio Maldonado Batiz from comment #28) > > It is possible and I'm reusing the patch > Patch53: Bug-1001841-disable-sslv2-tests.patch > modified by incorporating the needed corrections you had identified and were > being applied vua the old > # Fix flaws in Patch53 which caused needed tests to be skipped which could > hide errors > # on implementing the fix we already have a patch for. What is that fix? > Patch104: fix-disable-sslv2-tests.patch Ok. So you're asking me to review that you've correctly merged both patches into a single patch. I confirm that you have correctly merged fix-disable-sslv2-tests.patch into Bug-1001841-disable-sslv2-tests.patch However, in addition: (1) The old patch had removed SSLAUTH= In the new patch, you're keeping it. Was the previous removal a mistake, that you're fixing? Please confirm. (2) Optional. You're changing the script that starts selfserv during tests, by hardcoding the minimum version SSL3, by having added parameter -V ssl3: I don't mind doing it that way, since in RHEL 7, we're always disabling SSL2. However, it isn't really consistent with the rest of your script, which are dynamic, based on NSS_NO_SSL2. You could have used something like if [ "${NSS_NO_SSL2}" = "1" ]; then USE_MINVER="-V ssl3:" else USE_MINVER="" fi And in the places where you added "-V ssl3:", you could instead add: ${USE_MINVER} (3) Only an FYI, when I add the string "yyy" to a log output, I only ever do so while experimenting, so I can quickly find the results that are related to current experiments. I never intend to add "yyy" permanently anywhere. Created attachment 1128025 [details]
client auth. changes due to sha384 prf support
Created attachment 1128522 [details]
nss-prevent-abi-issue.patch
Elio, please apply this patch from Bob, as a workaround to the issue that Hubert had identified.
Created attachment 1128551 [details]
nss-prevent-abi-issue.patch
(In reply to Kai Engert (:kaie) from comment #37) > Created attachment 1128551 [details] > nss-prevent-abi-issue.patch this is the fixed patch Created attachment 1128692 [details]
all changes for rebase V3 - in patch form
Elio, the commit you made to rhel-7.3 doesn't contain the renaming of attribute "extendedMasterSecretUsed" from attachment 1128551 [details] - please add that.
(In reply to Kai Engert (:kaie) from comment #41) > Elio, the commit you made to rhel-7.3 doesn't contain the renaming of > attribute "extendedMasterSecretUsed" from attachment 1128551 [details] - > please add that. Yes, I noticed and I did pick it up for the bug #1310581 work whose build I'm testing and I'll synch. up this branch with 7.2.z as soon as the official 7.2 brew build are complete. Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory, and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://rhn.redhat.com/errata/RHBA-2016-2335.html |