1279520 - Add support for enforcing a system-wide crypto policy

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Elio Maldonado]

As requested downstream for Fedora by Nikos Mavrogiannopoulos.

As it is now, the system-wide crypto policy [0] in Fedora 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

[Chris Newman]

Fixing this likely depends on fixing one or more of bug 358440, bug 480174, bug 967235.

Comment 2

[Elio Maldonado]

Attachment: adds support for system wide crypto policy support

loads system wide crypto policy if it exists and can conditionally ignore it

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8767504 [details] [diff] [review]
adds support for system wide crypto policy support

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

I have to confess to not knowing this code particularly well.  This seems mostly OK, but I have a few questions.

::: lib/nss/config.mk
@@ +106,5 @@
>  endif
>  endif
> +
> +ifdef POLICY_FILE
> +DEFINES += -DPOLICY_FILE=\"$(POLICY_FILE)\" -DPOLICY_PATH=\"$(POLICY_PATH)\"

What if POLICY_PATH is undefined?  Don't you want to avoid having it defined, but empty?

Maybe:
ifndef POLICY_PATH
$(error You must define POLICY_PATH if you set POLICY_FILE)
endif

::: lib/nss/nssinit.c
@@ +439,3 @@
>  	PR_smprintf_free(moduleSpec);
>  	if (module) {
> +	    if (!module->loaded) {

I'd change this to:

if (module && !module->loaded) {
  SECMOD_DestroyModule(module);
  return NULL;
}

@@ +693,5 @@
>  		}
>  	    }
>  	}
> +#ifdef POLICY_FILE
> +	/* Load the system crypo policy file if it exists,

typo

@@ +697,5 @@
> +	/* Load the system crypo policy file if it exists,
> +	 * unless the NSS_IGNORE_SYSTEM_POLICY environment
> +	 * variable has been set to 1. */
> +	ignoreVar = PR_GetEnvSecure("NSS_IGNORE_SYSTEM_POLICY");
> +	if (ignoreVar == NULL || strncmp(ignoreVar, "1", strlen("1")) != 0) {

This will proceed if NSS_IGNORE_SYSTEM_POLICY is 11 or 1x or anything else starting with 1.  You should use strlen("1")+1, sizeof("1"), or even 2.

@@ +698,5 @@
> +	 * unless the NSS_IGNORE_SYSTEM_POLICY environment
> +	 * variable has been set to 1. */
> +	ignoreVar = PR_GetEnvSecure("NSS_IGNORE_SYSTEM_POLICY");
> +	if (ignoreVar == NULL || strncmp(ignoreVar, "1", strlen("1")) != 0) {
> +	    if (PR_Access(POLICY_PATH "/" POLICY_FILE, PR_ACCESS_READ_OK) == PR_SUCCESS ) {

Do you really want to let the policy file be ignored if it is merely unreadable?

I would have failed the process if this check fails, which probably means that you can avoid the PR_Access call entirely.

Comment 4

[Elio Maldonado]

Comment on attachment 8767504 [details] [diff] [review]
adds support for system wide crypto policy support

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

Thanks for the prompt review. I'll let Bob answer the part part on whether we can avoid the PR_Access call.

::: lib/nss/config.mk
@@ +106,5 @@
>  endif
>  endif
> +
> +ifdef POLICY_FILE
> +DEFINES += -DPOLICY_FILE=\"$(POLICY_FILE)\" -DPOLICY_PATH=\"$(POLICY_PATH)\"

Yes, will change that way.

::: lib/nss/nssinit.c
@@ +439,3 @@
>  	PR_smprintf_free(moduleSpec);
>  	if (module) {
> +	    if (!module->loaded) {

Yes I agree.

@@ +693,5 @@
>  		}
>  	    }
>  	}
> +#ifdef POLICY_FILE
> +	/* Load the system crypo policy file if it exists,

Yes, s/crypo/crypto/

@@ +698,5 @@
> +	 * unless the NSS_IGNORE_SYSTEM_POLICY environment
> +	 * variable has been set to 1. */
> +	ignoreVar = PR_GetEnvSecure("NSS_IGNORE_SYSTEM_POLICY");
> +	if (ignoreVar == NULL || strncmp(ignoreVar, "1", strlen("1")) != 0) {
> +	    if (PR_Access(POLICY_PATH "/" POLICY_FILE, PR_ACCESS_READ_OK) == PR_SUCCESS ) {

I defer to Bob on this one as he wrote this part.

::: lib/util/utilpars.c
@@ +1149,5 @@
>          *rw = PR_FALSE;
>     }
>  
>     /* only use the renamed secmod for legacy databases */
>     if ((*dbType != NSS_DB_TYPE_LEGACY) && 

trailing whitespace

Comment 5

[Elio Maldonado]

Attachment: the system crypto policy for nss

For informational purposes, this is the crypto policy file for nss which gets installed automatically by the crypto-policies package 
https://fedoraproject.org/wiki/Changes/CryptoPolicy

Comment 6

[Elio Maldonado]

Comment on attachment 8767504 [details] [diff] [review]
adds support for system wide crypto policy support

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

::: lib/nss/nssinit.c
@@ +699,5 @@
> +	 * variable has been set to 1. */
> +	ignoreVar = PR_GetEnvSecure("NSS_IGNORE_SYSTEM_POLICY");
> +	if (ignoreVar == NULL || strncmp(ignoreVar, "1", strlen("1")) != 0) {
> +	    if (PR_Access(POLICY_PATH "/" POLICY_FILE, PR_ACCESS_READ_OK) == PR_SUCCESS ) {
> +	    SECMODModule *module = SECMOD_LoadModule(

Martin, I don't think we can avoid the PR_Access call. The SEC_LoadModule line will call the parser on the policy file and so we must ensure not that policy file exist but also it's readable. Bob can confirm if that was his motivation intention for doing that check.

@@ +714,5 @@
> +		    goto loser;
> +		}
> +	    }
> +	}
> +    }

This call

Comment 7

[Robert Relyea]

> Do you really want to let the policy file be ignored if it is merely unreadable?

We want to ignore if the policy file is non-existent. That's a pretty common situation.

It's probably a good idea to fail if the policy file exists, but not readable.


> I would have failed the process if this check fails, which probably means that you can avoid the PR_Access
> call entirely.

We do want to proceed if the file is nonexistant, so it just means we probably need a different parameter to PR_Access.


bob

Comment 8

[Elio Maldonado]

The possible parameters are:
    PR_ACCESS_READ_OK. Test for read permission.
    PR_ACCESS_WRITE_OK. Test for write permission.
    PR_ACCESS_EXISTS. Check existence of file.
The one that makes sense is PR_ACCESS_READ_OK which is what you chose and you proceed, just don't try to load a file you can't parse, and thus ignore policy. So your code is right as far as I can tell. What am I missing in my reading our you statement?

Comment 9

[Elio Maldonado]

Attachment: support system wide crypto policy - V2

Addresses most of Martin's review comments.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8770300 [details] [diff] [review]
support system wide crypto policy - V2

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

::: lib/nss/nssinit.c
@@ +645,5 @@
>  
>      /* Skip the module init if we are already initted and we are trying
>       * to init with noCertDB and noModDB */
>      if (!(isReallyInitted && noCertDB && noModDB)) {
> +	parent = nss_InitModules(configdir, certPrefix, keyPrefix, secmodName, 

trailing ws

@@ +690,5 @@
>  		    nss_FindExternalRoot(dbpath, secmodName);
>  		}
>  	    }
>  	}
> +#ifdef POLICY_FILE

This block would be much cleaner as a new static function.

Comment 11

[Elio Maldonado]

Yes, I agree and will clean up that block later. In the meantime I pushed to nss-try as has been recently recommended https://hg.mozilla.org/projects/nss-try/rev/f0f2c22dba22

Comment 12

[Elio Maldonado]

pushed http://hg.mozilla.org/projects/nss/rev/f0f2c22dba22

Comment 13

[David Woodhouse]

This broke when I tried to use it. Bug 1296263.