1474887 - nss-policy-check: a tool to check a NSS policy configuration for errors

Status: RESOLVED FIXED

(NSS :: Tools, enhancement)

Description

[Kai Engert (:KaiE:)]

NSS supports policy configuration, using the config= statement in text configuration files (since bug 1009429).

We have the following issues with that:

Currently handling of policy configuration files is very lenient. Errors are silently ignored. I understand this is intentional, to avoid that a typo can cause a system to fail. As a result, today we don't have any mechanism to test that a policy configuration is accepted by NSS.

A policy configuration file may contain valid syntax, but may request a configuration that is probably unusable, if it disables too many mechanisms.

This bug requests to implement a tool, that can be used to identify the above issues.

Comment 1

[Kai Engert (:KaiE:)]

We should reuse the existing NSS code that process policy configuration, to avoid misalignment between implementation and test code.

In this bug we don't want to change the behavior of NSS. We continue to accept and ignore mistakes by default for the NSS library runtime operation. If changes to this are requested, such changes should be handled in a separate bug.

This means, we need to parameterize the internal policy processing code. If it's for NSS library operation, it should continue its current lenient operation. However, when running as part of the checking tool, we must report any problems that are detected.

I'd like to avoid introducing or modifying interfaces if possible.

I think the simplest implementation approach is the following:
- we reuse the existing configuration strings that NSS uses internally when loading modules, which is also used when loading policy configuration
- we don't produce hard errors, we simply print information
- the user of the checking tool must interpret the information

I suggest to add a new NSS flag "printPolicyFeedback". If present, the NSS internal code will print information while processing policy configuration.

I suggest to add a new tool named nss-policy-check. Only this tool will use the flag "printPolicyFeedback".

I suggest that we print two categories of messages.
- informational messages to stdout, which start with NSS-POLICY-INFO
- failure messages to stderror, which start with NSS-POLICY-FAIL

If NSS considers a configuration aspect as problematic, it prints a NSS-POLICY-FAIL statement.

Any number of messages of both categories may be produced.

I suggest that the consumer of the tool output should check the output for NSS-POLICY-FAIL messages. If failures messages are found, the policy is likely problematic and should be fixed.

I suggest that the exit code of the tool is undefined. The reason is that the configuration processing code inside NSS is decoupled from the tool execution, and result codes cannot be passed from it to the nss-policy-check tool.

I believe it is difficult to define clear rules which configurations are good or bad. Some environments might be OK with disabling all TLS protocol versions. Therefore I suggest that it's left for consumers of the tool output to make the decision whether a configuration is acceptable or not, by inspecting the information printed by the tool.

Comment 2

[Kai Engert (:KaiE:)]

I'll attach an initial implementation.

When given a simple configuration such as
  config="disallow=all"
it prints:

NSS-POLICY-INFO: number of enabled algorithms for SSL Key Exchange: 0
NSS-POLICY-INFO: number of enabled algorithms for SSL: 0
NSS-POLICY-INFO: number of enabled algorithms for Certificate Signatures: 0
NSS-POLICY-INFO: number of enabled algorithms of type Elliptic Curve (ECC): 0
NSS-POLICY-INFO: number of enabled algorithms of type Hash: 0
NSS-POLICY-INFO: number of enabled algorithms of type Mac: 0
NSS-POLICY-INFO: number of enabled algorithms of type Cipher: 0
NSS-POLICY-INFO: number of enabled algorithms of type Key Exchange: 0
NSS-POLICY-FAIL: insufficient number of enabled algorithms
NSS-POLICY-INFO: number of enabled TLS Ciphersuites: 0
NSS-POLICY-FAIL: insufficient number of enabled TLS ciphersuites
NSS-POLICY-INFO: number of enabled TLS protocol versions: 3
NSS-POLICY-INFO: number of enabled DTLS protocol versions: 2


When not given any configuration, only a partial summary is printed. The reason is that the policy processing code isn't reached:
NSS-POLICY-INFO: number of enabled TLS Ciphersuites: 31
NSS-POLICY-INFO: number of enabled TLS protocol versions: 3
NSS-POLICY-INFO: number of enabled DTLS protocol versions: 2


When using invalid syntax like
  config="disallow=whatever"
the output contains
NSS-POLICY-FAIL disallow: ignoring unknown identifier: whatever

When using unknown values for variables like
  config="disallow=ALL"config="allow=tls-version-min=bla"
we get:
NSS-POLICY-FAIL tls-version-min=whatever: unknown value: whatever
NSS-POLICY-FAIL: policy config parsing failed, not loading module Policy
NSS-POLICY-INFO: number of enabled TLS Ciphersuites: 0
NSS-POLICY-FAIL: insufficient number of enabled TLS ciphersuites

This is the only place where the code changes the implementation to return a failure. Function secmod_applyCryptoPolicy already has code to check for successfully setting a value with NSS_OptionSet. If we cannot find a known value, I conclude it's fine to fail in that scenario, too.

Here is sample output, when using one of the valid configuration that we use in the NSS test suite (from sslpolicy.txt):
  config="disallow=all allow=md2/all:md4/all:md5/all:sha1/all:sha256/all:sha384/all:sha512/all:hmac-sha1/all:hmac-sha224/all:hmac-sha256/all:hmac-sha384/all:hmac-sha512/all:hmac-md5/all:camellia128-cbc/all:camellia192-cbc/all:camellia256-cbc/all:seed-cbc/all:des-ede3-cbc/all:des-40-cbc/all:des-cbc/all:null-cipher/all:rc2/all:rc4/all:idea/all:rsa/all:rsa-export/all:dhe-rsa/all:dhe-dss/all:ecdhe-ecdsa/all:ecdhe-rsa/all:ecdh-ecdsa/all:ecdh-rsa/all:tls-version-min=ssl2.0:tls-version-max=tls1.2"

NSS-POLICY-INFO: MD2 is enabled for KX
NSS-POLICY-INFO: MD2 is enabled for CERT-SIGNATURE
NSS-POLICY-INFO: MD4 is enabled for KX
NSS-POLICY-INFO: MD4 is enabled for CERT-SIGNATURE
NSS-POLICY-INFO: MD5 is enabled for KX
NSS-POLICY-INFO: MD5 is enabled for CERT-SIGNATURE
NSS-POLICY-INFO: SHA1 is enabled for KX
NSS-POLICY-INFO: SHA1 is enabled for CERT-SIGNATURE
NSS-POLICY-INFO: SHA256 is enabled for KX
NSS-POLICY-INFO: SHA256 is enabled for CERT-SIGNATURE
NSS-POLICY-INFO: SHA384 is enabled for KX
NSS-POLICY-INFO: SHA384 is enabled for CERT-SIGNATURE
NSS-POLICY-INFO: SHA512 is enabled for KX
NSS-POLICY-INFO: SHA512 is enabled for CERT-SIGNATURE
NSS-POLICY-INFO: HMAC-SHA1 is enabled for SSL
NSS-POLICY-INFO: HMAC-SHA224 is enabled for SSL
NSS-POLICY-INFO: HMAC-SHA256 is enabled for SSL
NSS-POLICY-INFO: HMAC-SHA384 is enabled for SSL
NSS-POLICY-INFO: HMAC-SHA512 is enabled for SSL
NSS-POLICY-INFO: HMAC-MD5 is enabled for SSL
NSS-POLICY-INFO: CAMELLIA128-CBC is enabled for SSL
NSS-POLICY-INFO: CAMELLIA192-CBC is enabled for SSL
NSS-POLICY-INFO: CAMELLIA256-CBC is enabled for SSL
NSS-POLICY-INFO: SEED-CBC is enabled for SSL
NSS-POLICY-INFO: DES-EDE3-CBC is enabled for SSL
NSS-POLICY-INFO: DES-40-CBC is enabled for SSL
NSS-POLICY-INFO: DES-CBC is enabled for SSL
NSS-POLICY-INFO: NULL-CIPHER is enabled for SSL
NSS-POLICY-INFO: RC2 is enabled for SSL
NSS-POLICY-INFO: RC4 is enabled for SSL
NSS-POLICY-INFO: IDEA is enabled for SSL
NSS-POLICY-INFO: RSA is enabled for KX
NSS-POLICY-INFO: RSA-EXPORT is enabled for KX
NSS-POLICY-INFO: DHE-RSA is enabled for KX
NSS-POLICY-INFO: DHE-DSS is enabled for KX
NSS-POLICY-INFO: ECDHE-ECDSA is enabled for KX
NSS-POLICY-INFO: ECDHE-RSA is enabled for KX
NSS-POLICY-INFO: ECDH-ECDSA is enabled for KX
NSS-POLICY-INFO: ECDH-RSA is enabled for KX
NSS-POLICY-INFO: number of enabled algorithms for SSL Key Exchange: 15
NSS-POLICY-INFO: number of enabled algorithms for SSL: 17
NSS-POLICY-INFO: number of enabled algorithms for Certificate Signatures: 17
NSS-POLICY-INFO: number of enabled algorithms of type Elliptic Curve (ECC): 0
NSS-POLICY-INFO: number of enabled algorithms of type Hash: 7
NSS-POLICY-INFO: number of enabled algorithms of type Mac: 6
NSS-POLICY-INFO: number of enabled algorithms of type Cipher: 11
NSS-POLICY-INFO: number of enabled algorithms of type Key Exchange: 8
NSS-POLICY-FAIL: insufficient number of enabled algorithms
NSS-POLICY-INFO: number of enabled TLS Ciphersuites: 5
NSS-POLICY-INFO: number of enabled TLS protocol versions: 3
NSS-POLICY-INFO: number of enabled DTLS protocol versions: 2

As it can be seen, a failure is printed, because no curves are enabled. This may or may not be a mistake.

When originally discussing how to define a configuration as valid, it has been suggested that at least one mechanism from each category should be enabled. This example shows that it might be difficult to define a rule that works for all environments. By printing messages only, the user is able to inspect and make final decisions.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: 1474887-v1.patch

Comment 4

[Kai Engert (:KaiE:)]

I'd like to slightly change the implementation and introduce a third category NSS-POLICY-WARN. I'd like to use it for scenarios that aren't obvious mistakes, such as a configuration with zero ECC curves enabled.

Comment 5

[Robert Relyea]

I think that's reasonable. You could use your tool to check pretty much any configuration you want automatically as follows:


if [ `nss-policy-check 1> /dev/null 2>&1 | grep -v -f {path}/OK_policy_warnings.txt | wc -l` -ne 0 ]; then
   echo "policy file is broken"
   nss-policy-check
   {do abort or recovery here}
fi
# policy OK, continue

OK_policy_warnings.txt would be a list of substrings from policy errors and warnings that are expected, like no ECC curves, or even certain unknown ciphers (that may be from policy files shared with newer systems).

If you use NSS-POLICY-WARN then the user could put NSS-POLICY-WARN in the OK_policy_warnings.txt file and ingore all the messages of that type.

bob

Comment 6

[Robert Relyea]

Comment on attachment 8991298 [details] [diff] [review]
1474887-v1.patch

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

::: lib/pk11wrap/pk11pars.c
@@ +1859,5 @@
>                      break;
>                  }
> +                if (!forwardPolicyFeedback) {
> +                    child = SECMOD_LoadModule(*index, module, PR_TRUE);
> +                } else {

Unsolicited review. This whole block is crying out to be it's own function to add strings to an existing cipher spec.

The more general way would be to have the signature
     newspec= NSSUTIL_AddToModuleSpec(char *spec, char *addition);

where addition would be something like "nss='flags=printPolicyFeedback'"). That is probably beyond the scope of what you are trying to do. Instead probably something like:

     newspec = NSSUTIL_AddToModuleSpec(char *spec, char *parameters, char *nss, char *config)
where you would drop the "nss-", so you would call it with

     newspec = NSSUTIL_AddToModuleSpec(*index, NULL, nss, NULL);

I would be OK if only the nss version is implemented initially and the others return errors. The body of the code would look pretty  much like what you have now.

bob

Comment 7

[Kai Engert (:KaiE:)]

Bob, I had assumed the "nss" part of the module string is always just a simple "Flags=a,b" string. In this scenario, the simple strcat works to add a flag. But now I'm worried my simplification won't work.

Now that you've asked me to make the code more general, I've looked at more examples of files that are parsed using the module-spec code, and I'm learning the syntax is much more complex.

For example, in a pkcs11.txt file, I see:

NSS=trustOrder=75 cipherOrder=100 slotParams={0x00000001=[slotFlags=RSA,RC4,RC2,DES,DH,SHA1,MD5,MD2,SSL,TLS,AES,RANDOM askpw=any timeout=30 ] }  Flags=internal,critical

I assume the order of the whitespace-separated entries in that line is flexible. The "Flags=" section could appear at any position, is that correct?

If true, the simple "strcat" strategy doesn't work. I'll have to split the line into segments (separated by whitespace), then identify the segment with the flags, append to it (or create it), and then reassemble the line.

Is my understanding of the required complexity correct?

Is it necessary to allow whitespace inside bracketed sections? In other words, if whitespace appears inside [] or {} brackets, it isn't treated as a separator for the groups inside the line, correct?

Comment 8

[Kai Engert (:KaiE:)]

Ok, I just found that we already have many helper functions, like NSSUTIL_ArgGetParamValue, which probably already handles the complexity of the complex lines. That's another argument to implement NSSUTIL_AddToModuleSpec inside NSS, as it will allow access to all helper functions, including those that aren't exported. I'll work on that.

Comment 9

[Robert Relyea]

Yes, that's correct. You'll need the helper functions already there. You should handle the nss= correctly. The others (parameters and config) you can put aside for now. Actually you can only really parse parameters for softoken, everything else is free form, so having a 'add to' function for parameters doesn't make too much sense), and config is basically policy right now, so NSS seems like the only needed function.

bob

Comment 10

[Kai Engert (:KaiE:)]

Attachment: 1474887-v2.patch

Comment 11

[Kai Engert (:KaiE:)]

Regarding the general helper function. I specifically need a function that appends a simple string flag to such an existing list of flags, or create it.

However, the NSS= section allows more complex entries such as
  slotParams={0x00000001=[slotFlags=RSA,RC4,RC2,DES,DH,SHA1,MD5,MD2,SSL,TLS,AES,RANDOM askpw=any timeout=30 ] }

Implementing a generic function that works with any item inside NSS= seems out of scope. The other functions like trustOrder and cipherOrder also seem inappropriate for an appending function.

I'd like to suggest to specifically focus on the Flags= section inside NSS. I've implemented NSSUTIL_AddNSSFlagToModuleSpec see the attached patch.

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 8992312 [details] [diff] [review]
1474887-v2.patch

Minor issues found in self-review, will attach an updated patch.

Comment 13

[Robert Relyea]

Comment on attachment 8992312 [details] [diff] [review]
1474887-v2.patch

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

No issues need to be fixed. I do have some comments. The only ones that I think are most worthy of attention are the error values when returning -1, but potentially no policy code produced NSS-POLICY-FAIL.

bob

::: cmd/nss-policy-check/nss-policy-check.c
@@ +116,5 @@
> +        path, filename);
> +
> +    module = SECMOD_LoadModule(moduleSpec, NULL, PR_TRUE);
> +    if (!module || !module->loaded || atoi(PR_GetEnv("NSS_POLICY_LOADED")) != 1) {
> +        fprintf(stderr, "Error: failed to load policy file\n");

shouldn't this be NSS-POLICY-FAIL: instead of Error:, or do we depend on the printPolicyFeedback to tag the error?

@@ +122,5 @@
> +    }
> +
> +    rv = SSL_OptionSetDefault(SSL_SECURITY, PR_TRUE);
> +    if (rv != SECSuccess) {
> +        fprintf(stderr, "enable SSL_SECURITY failed: %s\n", PORT_ErrorToString(PR_GetError()));

Same question as above. (though in this case we know SSL_OptionSetDefault isn't going to print anything.) In this case it's probably an NSS issue, so it could be a warning or a info, though it stops our validation of the policy file, so it's probably something more fatal than less fatal.
 ---Also shouldn't we set an Environment variable here? -- no answer is the environment variables are not for the calling application, as they go away once the process for policy check goes away. The application can only trigger on the output string if it wants to distinguish between errors, warnings, and failures.

@@ +164,5 @@
> +    if (atoi(PR_GetEnv("NSS_POLICY_FAIL")) != 0) {
> +        result = -1;
> +    } else if (atoi(PR_GetEnv("NSS_POLICY_WARN")) != 0) {
> +        result = 1;
> +    }

OK looks like you are  just using the environment variable to control the result of this program. I thought it was also for the caller. Now that I think about it, though, the environment variable is going away at process end. Caller looks at the return value (0 OK, -1 FAIL, 1 WARN).

@@ +168,5 @@
> +    }
> +
> +    rv = NSS_Shutdown();
> +    if (rv != SECSuccess) {
> +        fprintf(stderr, "NSS_Shutdown failed: %s\n", PORT_ErrorToString(PR_GetError()));

I'm not sure if shutdown failure should be a FAIL, WARN, or INFO, since it really indicates a problem with NSS rather than a problem with the policy. I guess if it returns -1 it should be FAIL.

::: lib/pk11wrap/pk11pars.c
@@ +495,5 @@
>                  break;
>              }
>          }
> +        if (unknown && printPolicyFeedback) {
> +            NSS_PutEnv("NSS_POLICY_FAIL", "1");

I would be tempted to make this a WARN rather than FAIL.
I'm thinking the case where you distribute a policy file for a newer version of NSS and included it with an older version, there will likely be unknown elements in that policy file.

@@ +633,5 @@
> +                rv = secmod_getPolicyOptValue(policyValue, policyValueLength,
> +                                              &val);
> +                if (rv != SECSuccess) {
> +                    if (printPolicyFeedback) {
> +                        NSS_PutEnv("NSS_POLICY_FAIL", "1");

Same comment as above for FAIL versus WARN

@@ +1798,2 @@
>          PORT_Free(library);
> +        library = NULL;

I think these (setting pointers to NULL after free) were left over from having the adding to the nss flags inline rather than a separate function. I'm OK with leaving them (good paranoia/defensive programing), but they are no longer needed.

bob

@@ +1870,5 @@
> +                    child = SECMOD_LoadModule(*index, module, PR_TRUE);
> +                } else {
> +                    /* Add printPolicyFeedback to the nss flags */
> +                    char *specWithForwards =
> +                        NSSUTIL_AddNSSFlagToModuleSpec(*index, "printPolicyFeedback");

Good. I'm find with a function that just adds to the NSS Flags.

Comment 14

[Kai Engert (:KaiE:)]

Bob, thanks for your review.

Your most significant comment is related to the treatment of unknown identifiers and values. You suggest they should be WARN, not FAIL. I'd argue against your suggestion.

The purpose of the tool is specifically to identify issues. If NSS doesn't understand a value from the policy file, then it will fall back to a default value. This could be something completely different than the intended behavior. (For example, previous policy could have been min-tls=tls1.3, new policy could be min-tls=tls1.4, while the software default is still min-tls=tls1.2. The failure to recognize the tls1.4 value would mean a much weaker policy than intended.)

Although the fall back is expected for the library behavior, I think the checking tool shouldn't tolerate unknown values. I think we can require that the policy file will be specifically tested with the version of NSS and the tool that is expected to be used with the given policy file.

Also, if we treated unknown values as warnings, we'd effectively remove the justification for separate FAIL and WARN categories. No file content would ever produce a failure. Only processing failures, such as internal library errors or file access failures could produce a real failure. Any file content would produce warnings, only.

Regarding other aspects of the code:

Yes, as you found out, environment variables are used internally, only, to communicate the validation status to the outer tool code.

The exit code "-1" worked different on Linux and Windows. Therefore the latest patch changes it to error code "2".

Instead of returning early, I've change it to use "goto loser:" to ensure we shutdown and ensure that we never get reports about leaks from the test automation.

You noted that some failures scenarios don't print the NSS-POLICY-FAIL string. That's a good point. I've added code that will ensure this string is printed, whenever we return a failure exit code - even if the reason for failure isn't related to policy syntax, and the string hasn't been printed yet.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: 1474887-v4.patch

Comment 16

[Kai Engert (:KaiE:)]

As a consequence of my suggestion, an accidental upgrade of NSS while keeping an old policy file, or an accidental upgrade to a new policy file while keeping old NSS, would be reported by the checking tool. (While the software continues to operate based on fallbacks.)

I believe this kind of detection is an intended purpose of the tool, and treating unknown identifiers as failures can avoid accidental mistakes, such as upgrading just one without the other.

Comment 17

[Robert Relyea]

OK, if the only fatal are unknown (and internal NSS issues), then we can keep them. It's probably better to handle the unknown through post processing anyway (screening out the specific unknown values from the output and not depending on the return code). So that's fine. I think not having fatal for the non-policy related errors was the bigger issue.

Comment 18

[Robert Relyea]

Comment on attachment 8992421 [details] [diff] [review]
1474887-v4.patch

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

r+ rrelyea

Comment 19

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/a6d6a56b6e39

Comment 20

[Kai Engert (:KaiE:)]

Attachment: 1474887-skip-shutdown-before-init.patch

If we haven't reached init yet, we should skip shutdown.

For example, this prevents an error message to be printed, if started without parameters.

Comment 21

[Robert Relyea]

Comment on attachment 8993642 [details] [diff] [review]
1474887-skip-shutdown-before-init.patch

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

r+ rrelyea

Comment 22

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/f7aaf4ead845