As it is now, the system-wide crypto policy [0] in F21 is only enforced by openssl and gnutls. To harmonize crypto in Fedora, NSS should respect the settings of the system-wide crypto policy as well. There are few things to be done, before that is possible: 1. NSS should provide the required knobs to fine-tune the settings of the policy 2. NSS should provide the ability to read settings from a system-wide location which can be updated automatically (e.g, by a script). [0]. http://fedoraproject.org/wiki/Changes/CryptoPolicy
This bug appears to have been reported against 'rawhide' during the Fedora 22 development cycle. Changing version to '22'. More information and reason for this action is here: https://fedoraproject.org/wiki/Fedora_Program_Management/HouseKeeping/Fedora22
Elio has there been any update on that issue?
I haven't had the cycles to track why have the failures that I reported upstream at https://bugzilla.mozilla.org/show_bug.cgi?id=1009429#c8. There were essentially two types errors. ssl.sh: #1736: OCSP stapling, signed response, good status produced a returncode of 2, expected is 0 - FAILED and ssl.sh: #1727: TLS Server hello response with SNI produced a return code of 1, expected is 0 - FAILED On the first, I still need to extract one (or two) specific test(s) that we can manually invoke and do some debugging. On the second case I have the still unconfirmed suspicion that the failure may be related to one of the bugs listed here: https://bugzilla.mozilla.org/buglist.cgi?list_id=12441912&short_desc=SNI&resolution=---&classification=Components&query_format=advanced&short_desc_type=allwordssubstr&product=NSS in particular the second one listed. Another interesting fact is that once I applied to the upstream code along with your patches the patches that we have for disabling support for SSL2 and export cipher suites as we have had since RHEL-7 and in Fedora since f22 and may failures, if not all, went away. In that set up the SSL2 and EXPORT test are skipped. It was along time ago so details of the results have become blurred. I think it would be worth while trying your patches, suitably modified, on some Fedora scratch builds. F22 or higher would be required. I could make private (yet shared) branches for this investigation if you think it's worthwhile.
(In reply to Elio Maldonado Batiz from comment #3) > I haven't had the cycles to track why have the failures that I reported > upstream at https://bugzilla.mozilla.org/show_bug.cgi?id=1009429#c8. > There were essentially two types errors. > ssl.sh: #1736: OCSP stapling, signed response, good status produced a > returncode of 2, expected is 0 - FAILED At which of my patches this failure starts? > Another interesting fact is that once I applied to the upstream code along > with your patches the patches that we have for disabling support for SSL2 > and export cipher suites as we have had since RHEL-7 and in Fedora since f22 > and may failures, if not all, went away. In that set up the SSL2 and EXPORT > test are skipped. It was along time ago so details of the results have > become blurred. Most likely these patches intersect. > I think it would be worth while trying your patches, suitably modified, on > some Fedora scratch builds. F22 or higher would be required. I could make > private (yet shared) branches for this investigation if you think it's > worthwhile. I think the faster we act to merge the patch the better. The code bases have diverted so much it already needs too much additional work simply for rebasing. As it is now, I'm crippled as I cannot run the test suite, and only depend on you figuring the issue.
(In reply to Nikos Mavrogiannopoulos from comment #4) > (In reply to Elio Maldonado Batiz from comment #3) > > I haven't had the cycles to track why have the failures that I reported > > upstream at https://bugzilla.mozilla.org/show_bug.cgi?id=1009429#c8. > > There were essentially two types errors. > > ssl.sh: #1736: OCSP stapling, signed response, good status produced a > > returncode of 2, expected is 0 - FAILED > > At which of my patches this failure starts? In the very last patch. > > > Another interesting fact is that once I applied to the upstream code along > > with your patches the patches that we have for disabling support for SSL2 > > and export cipher suites as we have had since RHEL-7 and in Fedora since f22 > > and may failures, if not all, went away. In that set up the SSL2 and EXPORT > > test are skipped. It was along time ago so details of the results have > > become blurred. > > Most likely these patches intersect. > > > I think it would be worth while trying your patches, suitably modified, on > > some Fedora scratch builds. F22 or higher would be required. I could make > > private (yet shared) branches for this investigation if you think it's > > worthwhile. On further though, that idea won't work. Scratch builds won't fly because in Fedora we have things split into separate nss-util, nss-softokn, and nss packages which would have to be build in that order with each being in the the buildroot for the next one. The builds would have to be local ones. A private branch doesn't help much. > > I think the faster we act to merge the patch the better. The code bases have > diverted so much it already needs too much additional work simply for > rebasing. As it is now, I'm crippled as I cannot run the test suite, and > only depend on you figuring the issue. Yes, I'll revisit this again. I'll need setup a shared VM environment such as Beaker. I'll resume work on this as soon as current tasks on my plate ease off a bit
(In reply to Elio Maldonado Batiz from comment #5) > (In reply to Nikos Mavrogiannopoulos from comment #4) > > (In reply to Elio Maldonado Batiz from comment #3) > > > I haven't had the cycles to track why have the failures that I reported > > > upstream at https://bugzilla.mozilla.org/show_bug.cgi?id=1009429#c8. > > > There were essentially two types errors. > > > ssl.sh: #1736: OCSP stapling, signed response, good status produced a > > > returncode of 2, expected is 0 - FAILED > > > > At which of my patches this failure starts? > In the very last patch. The last patch applies the policy, but in that case it shouldn't have applied anything since no policy was set. I submitted a bug fix there with the comment "do not apply policies on empty policy string or when SEC_OID_APPLY_SSL_POLICY is not set". Is that applied? If yes, the bug would be really trivial to figure if you tell me how to reproduce that failure.
All the required changes are now upstream (see 1009429). The plan is to include them in F24 and enable them for all applications in F25. Elio please update this bug with a note when the NSS which has the required changes enters F24, and close it when we enable it for all applications.
(In reply to Nikos Mavrogiannopoulos from comment #7) The nss-3.22.0-3.fc24 build, part of the rebase to NSS 3.22, has the needed changes. This bug will remain open until F25 when we enable policy and have tested it.
This bug appears to have been reported against 'rawhide' during the Fedora 24 development cycle. Changing version to '24'. More information and reason for this action is here: https://fedoraproject.org/wiki/Fedora_Program_Management/HouseKeeping/Fedora24#Rawhide_Rebase
Created attachment 1137167 [details] Patch to force loading policy if /etc/pki/nssdb/policy.cfg exists. This patch is to NSS. The patch is written in an upstream friendly way. That is it isn't compiled in unless the POLICY_FILE variable is set. For it to function you need to set POLICY_FILE and POLICY_PATH. That allows us to set a platform specific path if upstream wants it on by default. For redhat, we simply set the environment variables in the .spec file. I'll attach a sample. This patch requires the nss-util patch.
Created attachment 1137194 [details] nss-util patch to enable check policy This patch basically allows us to rename pkcs11.txt. For historical reasons, we've overridden the secmod= line for sql databases. This patch allows someone to give us a flag that tells us not to do the override. The flag is forceSecmodChoice. The nss policy check patch uses this flag so that we call call the policy file 'policy.cfg'. Nothing else special about this patch. It should be applied upstream at the same time the nss patch is applied there.
Created attachment 1137208 [details] Environment variables to add to the nss.spec to turn on policy file checking Should be self explanatory.
Created attachment 1137235 [details] Sample policy.cfg Here's an actual policy.cfg I used to test. The critical thing here is the config= The rest is just to make the module loading code happy. You can also force loading other PKCS #11 modules (like trust stores) with this, but remember that this affects *ALL* nss programs (including programs that initialized without a database). for more info on what to put in the config= line see the extensive comments in nss/lib/pk11wrap/pk11pars.c NOTE: try out different configs before you deploy. It's possible to prevent firefox from working since firefox does an SSL phone home, and it it doesn't like that SSL parameters used in the negotiation, it will fail to start. (This config successfully moved the fedora page from aes128_gcm to aes128_cbc). bob
Created attachment 1141009 [details] force loading policy if it exists - patch for upstream Full patch, based in Bob's patches, more suitable for upstream review as soon as we file the upstream bug.
Created attachment 1146295 [details] Update for upstream documentation of options Note that the documentation text in pk11pars.c requires some update. The '/' option is not described, and I'm not sure what "disallow=AES128-GCM/ALL" could mean. Also if I have "disallow=all allow=tls-version-min=TLS1.0" does it mean that TLS 1.2 is enabled or do I explicitly need TLS-VERSION MAX? I've only updated the options that I can see they are changed. Please review and update so that we can use the upstream source as documentation for the options. https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11pars.c#141
> Note that the documentation text in pk11pars.c requires some update. > https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11pars.c#141 I knew I had a write up on this. I just need to copy it from the other location. you can find the other writeup in security/nss/tests/ssl/sslpolicy.txt > The '/' option is not described, and I'm not sure what "disallow=AES128-GCM/ALL" could mean. It means turn off AES128-GCM for all possible policies. It's basically the default if you don't specify any flags. Currently it's most useful for hashing functions where you may want to only turn them off for particular operations. > Also if I have "disallow=all allow=tls-version-min=TLS1.0" does it mean that > TLS 1.2 is enabled or do I explicitly need TLS-VERSION MAX? No, tls-verision-max will float with the NSS default. if you explicitly set TLS1.2, then when NSS support TLS1.3 and set's it as the default, you will still only get TLS1.2 (NOTE if you say disable=all, but don't turn on some specific key exchange and hash ciphers, then you won't get any TLS no matter what you set TLS-VERSION-{MAX,MIN} to. bob
Elio note that the policy file that will be used is not /etc/pki/nssdb/policy.cfg but /etc/crypto-policies/back-ends/nss.config. Please change the POLICY_PATH in the spec to "/etc/crypto-policies/back-ends"
and change the documentation appropriately
On more round. Elio the filename for the policy is _nss.config_ no nss.cfg. See #17 above. What needs to be changed is: export POLICY_FILE="nss.config" I was looking for nss.config in the strace log and that's why I didn't see it. Also something that I'm not sure it is correct. In nss-check-policy-file.patch there is the line: "parameters=\"configdir='sql:" POLICY_PATH "' " within the if (PR_Access(POLICY_PATH "/" POLICY_FILE, PR_ACCESS_READ_OK) == PR_SUCCESS ) block. What is this line purpose? The POLICY_PATH (/etc/crypto-policies/back-ends) will not have any nssdb. Should that be some default path for NSS?
I've recompiled with the correct file and retried, and still it seems not to open the file. What I notice may be related with the comment above. It tries to open pkcs11.txt instead in that same directory which it doesn't exist. If I link pkcs11.txt to nss.config everything is ok. access("/etc/crypto-policies/back-ends/nss.config", R_OK) = 0 open("/etc/crypto-policies/back-ends/pkcs11.txt", O_RDONLY) = -1 ENOENT (No such file or directory) access("/etc/crypto-policies/back-ends/nss.config", F_OK) = 0 ... stat("/etc/crypto-policies/back-ends/pkcs11.txt", 0x7ffdd7b865f0) = -1 ENOENT (No such file or directory) open("/etc/crypto-policies/back-ends/pkcs11.txt", O_RDONLY) = -1 ENOENT (No such file or directory)
(In reply to Nikos Mavrogiannopoulos from comment #20) > I've recompiled with the correct file and retried, and still it seems not to > open the file. What I notice may be related with the comment above. It tries > to open pkcs11.txt instead in that same directory which it doesn't exist. Er... if it's opening pkcs11.txt from there, does that mean this is a potential way of fixing bug 1173577? One could argue that loading the right PKCS#11 tokens according to the system (p11-kit) policy is quite closely related to this bug, and it lives in basically the same nss_InitModules() code path.
Created attachment 1158414 [details] add two missing exports to % - in patch format The same ones we have for nss.spec whose omission here may be the cause of the problems reported by Nikos.
Comment on attachment 1158414 [details] add two missing exports to % - in patch format Oops, you may want to hold off the review wait until I test it :-)
They aren't. The defines are only applicable to nssinit.c, which is in nss.def. There were no other conditional defines. The defines are to force loading the config file by default. The general code for the config file is not conditionally compiled. bob
Added system wide change proposal for F25: https://fedoraproject.org/wiki/Changes/NSSCryptoPolicies
Comment on attachment 1158414 [details] add two missing exports to % - in patch format Not needed as Bob pointed out.
Created attachment 1165273 [details] all changes crypto policy support - in patch format Not for review, including it in case someone want to reproduce what I did scratch build with those changes applied available at http://koji.fedoraproject.org/koji/taskinfo?taskID=14394808 I'll attach the separate patches next.
Created attachment 1165275 [details] all changes for system crypto policy support - in patch format Not for review, including it in case someone wants to reproduce what I did scratch build with those changes applied available at http://koji.fedoraproject.org/koji/taskinfo?taskID=14394808 I'll attach the separate patches next.
Created attachment 1165280 [details] conditionally ignore sytem-wide crypto policy Based on an email exchanges I had by Nikos where he suggested: > provide mechanism to skip or override the system-wide crypto policies if > they are present and compile using that mechanism. A question for Bob is whether >that mechanism is already there, but if not, a suggestion would be to use an >environment variable such as NSS_IGNORE_SYSTEM_POLICY and if it is set to "1" >to avoid reading nss.config. What do you think? I liked the idea, tried it, had good results, ... >That way we provide a clean solution to compile nss.spec including all >possible tests, and we provide the administrators a mechanism to ignore >crypto policies for specific NSS applications. ... and as a result I'm attaching this and the other patches for review.
Created attachment 1165281 [details] tests scripts check for the policy file and adjust behaviour accordingly
Created attachment 1165288 [details] changes to sslauth test data for policy file change expected values in some of the authentication tests as to match what teh system-wide crypto policy allows. In nss.config we have: config="disallow=ALL allow=tls-version-min=tls1.0:tls-version-max=tls1.2:dtls-version-min=dtls1.0:dtls-version-max=dtls1.2: .....
Created attachment 1165289 [details] changes to nss.spec - in patch format
Created attachment 1165319 [details] test crypts enforce versus ignore differences - patch format Not a patch for review, included to make some things clearer. In the nss.spec patch you'll see +# to keep nss from loading the policy file +%if %{nss_ignore_system_policy} +export NSS_IGNORE_SYSTEM_POLICY=1 +pushd nss +# must revert patch64 before running tests otherwise they will fail +patch -p1 -R < %{PATCH64} +popd +%endif + That's what causes the differences. The tests have different expectations according to the type of build.
Comment on attachment 1165281 [details] tests scripts check for the policy file and adjust behaviour accordingly Typos, the lines SSLAUTH=${QADIR}/ssl/sslauth.noPolicytxt SSLCOV=${QADIR}/ssl/sslcov.noPolicytxt SSLSTRESS=${QADIR}/ssl/sslstress.noPolicytxt Should be SSLAUTH=${QADIR}/ssl/sslauth.noPolicy.txt SSLCOV=${QADIR}/ssl/sslcov.noPolicy.txt SSLSTRESS=${QADIR}/ssl/sslstress.noPolicy.txt
Created attachment 1165441 [details] tests scripts check for the policy file and adjust behaviour accordingly - V2 added the missing dots.
OK, you have 2 different tacks you look like you are trying to take. 1) use and environment variable to turn off policy. I would note that this variable can be sent by anyone, not just build time. PR_GetEnvSecure() has to do with semantics with setuid programs, not generic use of the environment variable. 2) turning off the tests that fail with the policy file present. I prefer option 2. I have some comments: a) The patches you have turn off tests or change the expected results on generic tests. make sure the patches you eventually supply turn off the tests only in the noPolicy.txt side (using the diff for a review is good, however. b) you should just disable the failing tests rather than running them and expecting failure. NOTE: you should also just disable the related tests. Do you know why the client auth test are failing? perhaps the cert is using md5? In which case we should change it upstream since md5 is turned of by default there now anyway, or if the tests are using SSL 3.0, then just have the tests use TLS 1.0 in the with policy case. (or if we already have TLS 1.0, then just disable all the SSL 3.0 tests). I'll r- the obviously wrong patches and r+ the ones that are obviously right. bob
Comment on attachment 1165280 [details] conditionally ignore sytem-wide crypto policy r-. This doesn' just apply at build time.
> That way we provide a clean solution to compile nss.spec including all > possible tests, and we provide the administrators a mechanism to ignore > crypto policies for specific NSS applications. OK, I see that allowing any user override the policy on their use of crypto is a desired feature. I'll go back and update my review.
Comment on attachment 1165280 [details] conditionally ignore sytem-wide crypto policy I'm keeping the r- here. Please not only check for the existance of the variable, but that the variable is actually set to "1".
Comment on attachment 1165289 [details] changes to nss.spec - in patch format Ok, this one you should remove patch 64 if you are using the environment variable, don't just revert it.
OK, my r- for the nss.spec and the conditional variable is no longer an objection to the environment variable, only to the specific implementation. bob
(In reply to Bob Relyea from comment #36) > OK, you have 2 different tacks you look like you are trying to take. > > 1) use and environment variable to turn off policy. I would note that this > variable can be sent by anyone, not just build time. PR_GetEnvSecure() has > to do with semantics with setuid programs, not generic use of the > environment variable. Here I assume that anyone, you mean any user of the system can disable the system-wide crypto policies. That's desirable. The crypto policies apply only for the default settings of any application. There is no reason to prevent a user from re-enabling RC4 for a NSS application if he wishes to. > 2) turning off the tests that fail with the policy file present. I find that quite dangerous. It means we will include untested code which will be however run if the administrator switches the policy to a lower one than the compiled. Furthermore there is no way to tell which policy will be enabled at compilation time (will it have AES-CBC or SHA1 included in the banned algorithms or not?). In the end it will end up disabling the whole test suite. I'd recommend strongly against such an approach.
Created attachment 1166732 [details] all changes for crypto policy support V2 - in patch format
Created attachment 1166733 [details] tests scripts check for the policy file and adjust behaviour accordingly - V3
Created attachment 1166734 [details] nss-softokn.spec file changes V2 - in patch format
Created attachment 1166735 [details] nss.spec after changes are applied
Comment on attachment 1166735 [details] nss.spec after changes are applied The line %define nss_tests "libpkix cert dbtests tools fips sdr crmf smime ssl ocsp merge pkits chains" should be changed to %define nss_tests "libpkix cert dbtests tools fips sdr crmf smime ssl ocsp merge pkits chains pk11_gtests der_gtests" to match what's on all.sh so we run some of the gtest based unit tests. Missing are ssl gtests as some are fail as of nss-3.24 but are in better shape as of nss-2.5 upstream. The so called external_tests are currently in a state of flux. I'm monitoring upstream progress and when we upgrade to nss-3.25 we should be able to include them.
Created attachment 1167675 [details] spec file changes - in patch format - V3 executes more tests
Nikos and and Bob discussed this and agreed the environment variable will be the way to acceptable away complete this. bob
Comment on attachment 1165280 [details] conditionally ignore sytem-wide crypto policy Requesting review again based on Comment 49.
Comment on attachment 1167675 [details] spec file changes - in patch format - V3 Testing revealed a syntax error, cleaner one coming soon.
Comment on attachment 1166733 [details] tests scripts check for the policy file and adjust behaviour accordingly - V3 Let's wait a bit, new version under testing.
Created attachment 1168517 [details] adjust sslautch expected resilt for crypto policy - in patch form Adding it to the list of sources instead of the list patches as this is to be applied at test time - see nsss.spec. This will evolve and could be replaced by another mechanism yet to be determined. Things are in a state of flux upstream and one or more nss upstream releases are anticipated before f25 goes out.
Created attachment 1168519 [details] make ssl.sh system crypto policy aware and also of env var to ignore it
Created attachment 1168520 [details] spec file changes - in patch format
Created attachment 1168521 [details] nss.spec after previous patch has been applied
Comment on attachment 1165280 [details] conditionally ignore sytem-wide crypto policy please see my previous review Get the environment variable and check to see if it's value is "1". The code you have here only checks for the existance of the environment variable.
Comment on attachment 1168519 [details] make ssl.sh system crypto policy aware and also of env var to ignore it r- not necessary under the plan to just set the disable policy environment variable before running the test suite.
Comment on attachment 1168520 [details] spec file changes - in patch format r- There is still a lot of unnecessary junk in this patch. We need to talk about it.
Comment on attachment 1168521 [details] nss.spec after previous patch has been applied r-.
My objection to your supplied patches are as follows: 1) your patches are trying to implement both the environment variable AND the changes to the test suites. You should only implement one. Nikos and I agreed that it should be the environment variable. 2) You need to fetch the environment variable and check it's actual value, not it's existance. (does it equal the string "1", not does it exist). Both comments were made on previous reviews.
Created attachment 1173088 [details] conditionally ignore sytem-wide crypto policy
Created attachment 1175148 [details] conditionally ignore sytem-wide crypto policy
Created attachment 1175149 [details] changes to nss.spec - in patch format Implements changes requested in last review.
I'll commit those changes as Bob has reviewed them as he was looking over my shoulder and making suggestions as I worked on them.
(In reply to David Woodhouse from comment #21) > ...does that mean this is a potential way of fixing bug 1173577? > > One could argue that loading the right PKCS#11 tokens according to the > system (p11-kit) policy is quite closely related to this bug, and it lives > in basically the same nss_InitModules() code path. I've just tested this, by adding an additional 'library=p11-kit-proxy.so' to the policy file. It seems to do the right thing. However, that *isn't* the right thing to do when noModDB is true. So if we do something like this, then I think we can reassign bug 1173577 to the crypto-policies package... --- a/lib/nss/nssinit.c +++ b/lib/nss/nssinit.c @@ -704,7 +704,7 @@ nss_Init(const char *configdir, const char *certPrefix, const char *keyPrefix, "secmod='" POLICY_FILE "' " "flags=readOnly,noCertDB,forceSecmodChoice,forceOpen\" " "NSS=\"flags=internal,moduleDB,skipFirst,moduleDBOnly,critical\"", - parent, PR_TRUE); + parent, !noModDB); if (module) { PRBool isLoaded = module->loaded; SECMOD_DestroyModule(module);
(In reply to Bob Relyea from comment #13) > You can also force loading other PKCS #11 modules (like trust stores) with > this, but remember that this affects *ALL* nss programs (including programs > that initialized without a database). Hm. That doesn't actually work because we end up loading any such modules *after* calling STAN_LoadDefaultNSS3TrustDomain(). And thus we end up with the slot existing, but segfaulting later because we *assume* slot->nssToken is non-NULL and it isn't: $ gdb --args certutil -d sql:/etc/pki/nssdb -L -h PIV_II\ \(PIV\ Card\ Holder\ pin\) ... Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI Enter Password or Pin for "PIV_II (PIV Card Holder pin)": Program received signal SIGSEGV, Segmentation fault. 0x00007ffff7608abf in PK11_DoPassword (slot=0x866ec0, session=8808048, loadCerts=1, wincx=0x7fffffffd9e0, alreadyLocked=0, contextSpecific=0) at pk11auth.c:629 629 nssTrustDomain_UpdateCachedTokenCerts(slot->nssToken->trustDomain, Missing separate debuginfos, use: dnf debuginfo-install openssl-libs-1.0.2h-3.fc24.x86_64 (gdb) p slot $1 = (PK11SlotInfo *) 0x866ec0 (gdb) p slot->nssToken $2 = (NSSToken *) 0x0 (gdb) p *slot So, in addition to fixing the observation in comment 66 about the 'noModDB' flag, can we also move the whole POLICY_FILE thing up by a few lines? Sticking it right at the start of the /* finish up initialization */ if (!isReallyInitted) { code block should do it; does that break anything?
Created attachment 1191861 [details] Suggested fix from comments 66 and 67
Filed bug upstream (with fixed patch that actually remembers to include the !noModDB bit). Reopening.
NSS seems to try to enforce the system-wide crypto policy, but this crashes firefox if policy is set to FUTURE, see bug #1404439.
(In reply to Christian Stadelmann from comment #70) > NSS seems to try to enforce the system-wide crypto policy, but this crashes > firefox if policy is set to FUTURE, see bug #1404439. Christian thank you for reporting that; could you please open a new bug of the issue? There is little point convoluting this bug.
(In reply to Nikos Mavrogiannopoulos from comment #71) > (In reply to Christian Stadelmann from comment #70) > > NSS seems to try to enforce the system-wide crypto policy, but this crashes > > firefox if policy is set to FUTURE, see bug #1404439. > > Christian thank you for reporting that; could you please open a new bug of > the issue? There is little point convoluting this bug. I did. That's what bug #1404439 is about. If you think bug #1404439 is a bug in NSS (instead of firefox), feel free to change the component. I just wanted you to know that bug #1404439 probably is a result of implementing system-wide crypto-policy in NSS.