Bug 1157720 - NSS should enforce the system-wide crypto policy
Summary: NSS should enforce the system-wide crypto policy
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: nss
Version: 24
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Elio Maldonado Batiz
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: fedora-crypto-policies 1179271 1309228 1335909 1340846
TreeView+ depends on / blocked
 
Reported: 2014-10-27 14:50 UTC by Nikos Mavrogiannopoulos
Modified: 2018-06-04 12:56 UTC (History)
13 users (show)

Fixed In Version: nss-util-3.25.0-2.fc25, nss-3.25.0-6.fc25
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-01-09 12:59:23 UTC


Attachments (Terms of Use)
Patch to force loading policy if /etc/pki/nssdb/policy.cfg exists. (6.04 KB, patch)
2016-03-16 23:10 UTC, Bob Relyea
no flags Details | Diff
nss-util patch to enable check policy (584 bytes, patch)
2016-03-16 23:14 UTC, Bob Relyea
no flags Details | Diff
Environment variables to add to the nss.spec to turn on policy file checking (230 bytes, text/plain)
2016-03-16 23:16 UTC, Bob Relyea
no flags Details
Sample policy.cfg (488 bytes, text/plain)
2016-03-16 23:22 UTC, Bob Relyea
no flags Details
force loading policy if it exists - patch for upstream (10.44 KB, patch)
2016-03-28 19:47 UTC, Elio Maldonado Batiz
no flags Details | Diff
Update for upstream documentation of options (1016 bytes, patch)
2016-04-12 09:40 UTC, Nikos Mavrogiannopoulos
no flags Details | Diff
add two missing exports to % - in patch format (456 bytes, patch)
2016-05-17 17:03 UTC, Elio Maldonado Batiz
no flags Details | Diff
all changes crypto policy support - in patch format (18.97 KB, patch)
2016-06-06 16:04 UTC, Elio Maldonado Batiz
no flags Details | Diff
all changes for system crypto policy support - in patch format (18.97 KB, patch)
2016-06-06 16:07 UTC, Elio Maldonado Batiz
no flags Details | Diff
conditionally ignore sytem-wide crypto policy (860 bytes, patch)
2016-06-06 16:21 UTC, Elio Maldonado Batiz
rrelyea: review-
Details | Diff
tests scripts check for the policy file and adjust behaviour accordingly (5.42 KB, patch)
2016-06-06 16:25 UTC, Elio Maldonado Batiz
emaldona: review-
Details | Diff
changes to sslauth test data for policy file (14.62 KB, patch)
2016-06-06 16:42 UTC, Elio Maldonado Batiz
rrelyea: review-
Details | Diff
changes to nss.spec - in patch format (2.72 KB, patch)
2016-06-06 16:44 UTC, Elio Maldonado Batiz
rrelyea: review-
Details | Diff
test crypts enforce versus ignore differences - patch format (5.14 KB, patch)
2016-06-06 16:56 UTC, Elio Maldonado Batiz
no flags Details | Diff
tests scripts check for the policy file and adjust behaviour accordingly - V2 (5.42 KB, patch)
2016-06-06 23:19 UTC, Elio Maldonado Batiz
rrelyea: review-
Details | Diff
all changes for crypto policy support V2 - in patch format (20.34 KB, patch)
2016-06-10 20:29 UTC, Elio Maldonado Batiz
no flags Details | Diff
tests scripts check for the policy file and adjust behaviour accordingly - V3 (5.00 KB, patch)
2016-06-10 20:32 UTC, Elio Maldonado Batiz
no flags Details | Diff
nss-softokn.spec file changes V2 - in patch format (4.34 KB, patch)
2016-06-10 20:37 UTC, Elio Maldonado Batiz
no flags Details | Diff
nss.spec after changes are applied (77.24 KB, text/x-matlab)
2016-06-10 20:40 UTC, Elio Maldonado Batiz
no flags Details
spec file changes - in patch format - V3 (4.85 KB, patch)
2016-06-13 20:16 UTC, Elio Maldonado Batiz
no flags Details | Diff
adjust sslautch expected resilt for crypto policy - in patch form (14.62 KB, patch)
2016-06-15 21:45 UTC, Elio Maldonado Batiz
no flags Details | Diff
make ssl.sh system crypto policy aware and also of env var to ignore it (5.03 KB, patch)
2016-06-15 21:50 UTC, Elio Maldonado Batiz
rrelyea: review-
Details | Diff
spec file changes - in patch format (6.09 KB, patch)
2016-06-15 21:51 UTC, Elio Maldonado Batiz
rrelyea: review-
Details | Diff
nss.spec after previous patch has been applied (77.89 KB, text/x-matlab)
2016-06-15 21:54 UTC, Elio Maldonado Batiz
rrelyea: review-
Details
conditionally ignore sytem-wide crypto policy (1.91 KB, patch)
2016-06-27 18:55 UTC, Elio Maldonado Batiz
no flags Details | Diff
conditionally ignore sytem-wide crypto policy (1.98 KB, patch)
2016-07-02 01:05 UTC, Elio Maldonado Batiz
no flags Details | Diff
changes to nss.spec - in patch format (3.97 KB, patch)
2016-07-02 01:10 UTC, Elio Maldonado Batiz
no flags Details | Diff
Suggested fix from comments 66 and 67 (2.19 KB, patch)
2016-08-18 11:59 UTC, David Woodhouse
no flags Details | Diff


Links
System ID Priority Status Summary Last Updated
Mozilla Foundation 1009429 None None None 2019-07-29 14:14:39 UTC
Mozilla Foundation 1279520 None None None 2019-07-29 14:14:39 UTC
Mozilla Foundation 1296263 None None None 2019-07-29 14:14:39 UTC

Description Nikos Mavrogiannopoulos 2014-10-27 14:50:45 UTC
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

Comment 1 Jaroslav Reznik 2015-03-03 16:26:24 UTC
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

Comment 2 Nikos Mavrogiannopoulos 2015-08-03 14:39:08 UTC
Elio has there been any update on that issue?

Comment 3 Elio Maldonado Batiz 2015-08-03 18:12:45 UTC
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.

Comment 4 Nikos Mavrogiannopoulos 2015-08-04 08:21:09 UTC
(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.

Comment 5 Elio Maldonado Batiz 2015-08-04 14:26:55 UTC
(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

Comment 6 Nikos Mavrogiannopoulos 2015-08-07 09:04:04 UTC
(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.

Comment 7 Nikos Mavrogiannopoulos 2016-01-07 09:21:02 UTC
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.

Comment 8 Elio Maldonado Batiz 2016-02-10 15:07:52 UTC
(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.

Comment 9 Jan Kurik 2016-02-24 13:16:52 UTC
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

Comment 10 Bob Relyea 2016-03-16 23:10:18 UTC
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.

Comment 11 Bob Relyea 2016-03-16 23:14:58 UTC
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.

Comment 12 Bob Relyea 2016-03-16 23:16:18 UTC
Created attachment 1137208 [details]
Environment variables to add to the nss.spec to turn on policy file checking

Should be self explanatory.

Comment 13 Bob Relyea 2016-03-16 23:22:17 UTC
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

Comment 14 Elio Maldonado Batiz 2016-03-28 19:47:32 UTC
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.

Comment 15 Nikos Mavrogiannopoulos 2016-04-12 09:40:58 UTC
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

Comment 16 Bob Relyea 2016-04-15 19:16:40 UTC
> 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

Comment 17 Nikos Mavrogiannopoulos 2016-04-19 09:16:09 UTC
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"

Comment 18 Hubert Kario 2016-04-19 13:53:34 UTC
and change the documentation appropriately

Comment 19 Nikos Mavrogiannopoulos 2016-05-12 11:57:46 UTC
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?

Comment 20 Nikos Mavrogiannopoulos 2016-05-12 13:33:23 UTC
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)

Comment 21 David Woodhouse 2016-05-13 07:16:38 UTC
(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.

Comment 22 Elio Maldonado Batiz 2016-05-17 17:03:54 UTC
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 23 Elio Maldonado Batiz 2016-05-17 17:07:44 UTC
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 :-)

Comment 24 Bob Relyea 2016-05-17 18:53:33 UTC
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

Comment 25 Nikos Mavrogiannopoulos 2016-05-19 13:30:47 UTC
Added system wide change proposal for F25:
https://fedoraproject.org/wiki/Changes/NSSCryptoPolicies

Comment 26 Elio Maldonado Batiz 2016-05-25 16:56:27 UTC
Comment on attachment 1158414 [details]
add two missing exports to %  - in patch format

Not needed as Bob pointed out.

Comment 27 Elio Maldonado Batiz 2016-06-06 16:04:53 UTC
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.

Comment 28 Elio Maldonado Batiz 2016-06-06 16:07:19 UTC
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.

Comment 29 Elio Maldonado Batiz 2016-06-06 16:21:55 UTC
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.

Comment 30 Elio Maldonado Batiz 2016-06-06 16:25:52 UTC
Created attachment 1165281 [details]
tests scripts check for the policy file and adjust behaviour accordingly

Comment 31 Elio Maldonado Batiz 2016-06-06 16:42:15 UTC
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: .....

Comment 32 Elio Maldonado Batiz 2016-06-06 16:44:07 UTC
Created attachment 1165289 [details]
changes to nss.spec - in patch format

Comment 33 Elio Maldonado Batiz 2016-06-06 16:56:26 UTC
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 34 Elio Maldonado Batiz 2016-06-06 20:24:30 UTC
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

Comment 35 Elio Maldonado Batiz 2016-06-06 23:19:23 UTC
Created attachment 1165441 [details]
tests scripts check for the policy file and adjust behaviour accordingly - V2

added the missing dots.

Comment 36 Bob Relyea 2016-06-08 00:27:56 UTC
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 37 Bob Relyea 2016-06-08 00:28:42 UTC
Comment on attachment 1165280 [details]
conditionally ignore sytem-wide crypto policy

r-. This doesn' just apply at build time.

Comment 38 Bob Relyea 2016-06-08 00:48:26 UTC

> 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 39 Bob Relyea 2016-06-08 00:50:24 UTC
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 40 Bob Relyea 2016-06-08 00:51:55 UTC
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.

Comment 41 Bob Relyea 2016-06-08 00:52:54 UTC
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

Comment 42 Nikos Mavrogiannopoulos 2016-06-10 07:27:35 UTC
(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.

Comment 43 Elio Maldonado Batiz 2016-06-10 20:29:04 UTC
Created attachment 1166732 [details]
all changes for crypto policy support V2 - in patch format

Comment 44 Elio Maldonado Batiz 2016-06-10 20:32:49 UTC
Created attachment 1166733 [details]
tests scripts check for the policy file and adjust behaviour accordingly - V3

Comment 45 Elio Maldonado Batiz 2016-06-10 20:37:58 UTC
Created attachment 1166734 [details]
nss-softokn.spec file changes V2 - in patch format

Comment 46 Elio Maldonado Batiz 2016-06-10 20:40:33 UTC
Created attachment 1166735 [details]
nss.spec after changes are applied

Comment 47 Elio Maldonado Batiz 2016-06-13 16:00:29 UTC
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.

Comment 48 Elio Maldonado Batiz 2016-06-13 20:16:20 UTC
Created attachment 1167675 [details]
spec file changes - in patch format - V3

executes more tests

Comment 49 Bob Relyea 2016-06-14 16:23:22 UTC
Nikos and and Bob discussed this and agreed the environment variable will be the way to acceptable away complete this.

bob

Comment 50 Elio Maldonado Batiz 2016-06-15 00:36:17 UTC
Comment on attachment 1165280 [details]
conditionally ignore sytem-wide crypto policy

Requesting review again based on Comment 49.

Comment 51 Elio Maldonado Batiz 2016-06-15 16:17:42 UTC
Comment on attachment 1167675 [details]
spec file changes - in patch format - V3

Testing revealed a syntax error, cleaner one coming soon.

Comment 52 Elio Maldonado Batiz 2016-06-15 16:22:16 UTC
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.

Comment 53 Elio Maldonado Batiz 2016-06-15 21:45:27 UTC
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.

Comment 54 Elio Maldonado Batiz 2016-06-15 21:50:19 UTC
Created attachment 1168519 [details]
make ssl.sh system crypto policy aware and also of env var to ignore it

Comment 55 Elio Maldonado Batiz 2016-06-15 21:51:59 UTC
Created attachment 1168520 [details]
spec file changes - in patch format

Comment 56 Elio Maldonado Batiz 2016-06-15 21:54:24 UTC
Created attachment 1168521 [details]
nss.spec after previous patch has been applied

Comment 57 Bob Relyea 2016-06-16 01:34:27 UTC
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 58 Bob Relyea 2016-06-16 01:36:48 UTC
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 59 Bob Relyea 2016-06-16 01:39:41 UTC
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 60 Bob Relyea 2016-06-16 01:41:52 UTC
Comment on attachment 1168521 [details]
nss.spec after previous patch has been applied

r-.

Comment 61 Bob Relyea 2016-06-16 01:45:38 UTC
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.

Comment 62 Elio Maldonado Batiz 2016-06-27 18:55:17 UTC
Created attachment 1173088 [details]
conditionally ignore sytem-wide crypto policy

Comment 63 Elio Maldonado Batiz 2016-07-02 01:05:55 UTC
Created attachment 1175148 [details]
conditionally ignore sytem-wide crypto policy

Comment 64 Elio Maldonado Batiz 2016-07-02 01:10:13 UTC
Created attachment 1175149 [details]
changes to nss.spec - in patch format

Implements changes requested in last review.

Comment 65 Elio Maldonado Batiz 2016-07-02 01:19:16 UTC
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.

Comment 66 David Woodhouse 2016-08-15 09:26:55 UTC
(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);

Comment 67 David Woodhouse 2016-08-18 11:58:54 UTC
(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?

Comment 68 David Woodhouse 2016-08-18 11:59:57 UTC
Created attachment 1191861 [details]
Suggested fix from comments 66 and 67

Comment 69 David Woodhouse 2016-08-18 12:40:04 UTC
Filed bug upstream (with fixed patch that actually remembers to include the !noModDB bit). Reopening.

Comment 70 Christian Stadelmann 2016-12-13 21:17:23 UTC
NSS seems to try to enforce the system-wide crypto policy, but this crashes firefox if policy is set to FUTURE, see bug #1404439.

Comment 71 Nikos Mavrogiannopoulos 2016-12-14 11:42:33 UTC
(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.

Comment 72 Christian Stadelmann 2016-12-14 15:35:58 UTC
(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.


Note You need to log in before you can comment on or make changes to this bug.