1370667 - Does freebl_fipsPowerUpSelfTest() have a good reason to run during startup (or ever for that matter)?

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[(no longer active)]

I was trying to figure out why FREEBL_GetVector() seems to take up so much time in this startup profile: https://perfht.ml/2rROMTI

I noticed that BL_POSTRan() calls freebl_fipsPowerUpSelfTest().  That seemed quite surprising!  AFAIK Firefox isn't FIPS certified any more.  Does this code have any business to run during our startup at all?

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #0)
...
> I noticed that BL_POSTRan() calls freebl_fipsPowerUpSelfTest().  That seemed
> quite surprising!  AFAIK Firefox isn't FIPS certified any more.

Firefox certainly isn't. My understanding is people have been saying "well, if the crypto library is FIPS-certified, then it's fine". Of course, as far as I know no recent version of NSS (and Firefox essentially must have the most recent version at the time of release) has been FIPS-certified, but that's another story.

> Does this code have any business to run during our startup at all?

Not if NSS isn't in FIPS mode. I would file an NSS bug (or move this to NSS :: Libraries).

Comment 2

[Franziskus Kiefer [:franziskus]]

I moved this over to NSS. It looks like the latest FIPS related changes introduced this without disabling it in non-FIPS mode. The call to freebl_fipsPowerUpSelfTest should be inside a #ifndef NSS_NO_INIT_SUPPORT.

Comment 3

[(no longer active)]

ping? Comment 2 seems like a nice low hanging fruit if we can fix this...

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

This is currently being worked on. Franziskus submitted a new patch yesterday that I'm currently reviewing. We'll just need one more review from RedHat and then we're good to go.

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: fips-startup-tests.patch

https://nss-review.dev.mozaws.net/D348

Comment 6

[Robert Relyea]

Comment on attachment 8897402 [details] [diff] [review]
fips-startup-tests.patch

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

Just for the record, I still strongly prefer to keep the FIPS plumbing in and create a 'fake' FIPS mode where we don't actually verify the library integrity, but all the other stuff happens. I'm still worried that we won't be getting FIPS testing at all on the mozilla side.

I also recognized that I've lost that battle, so I'm not holding Franziskus to that standard. (Just registering my objection for posterity).

The r- is reviewing the patch assuming the goal is to have a completely non-FIPS compiled version of softoken as well as a full-FIPS version. The patch falls down in 2 cases:

1) in the non-FIPS case, the integrity checks should always FAIL. Anyone depending on those integrity checks should get failures if they aren't using the full-FIPS mode (in my proposed fake-FIPS mode, you want success, because you are 'faking' FIPS mode).
2) The unrelated rng change needs to allow reinitialization, so the goRNGInit variable needs to bel cleard on RNG_Shutdown if we are going to clear the rng_lock.
The bike shedding comment is just my preference and does not need to be changed to get an r+.

bob

::: lib/freebl/det_rng.c
@@ +112,5 @@
>  {
> +    if (rng_lock) {
> +        PZ_DestroyLock(rng_lock);
> +        rng_lock = NULL;
> +    }

Hmm I think we also need to clear the coRNGInit variable. We need to support the case where we've shutdown NSS and restarted it.

::: lib/freebl/nsslowhash.c
@@ +45,3 @@
>      return 1;
>  }
> +#endif /* NSS_FIPS_DISABLED */

Bike shedding: I would have preferred just to keep the function and always return 0. 

Doing it your way does save a call and some code, my way has less ifdefs.

::: lib/freebl/shvfy.c
@@ +555,5 @@
> +PRBool
> +BLAPI_VerifySelf(const char *name)
> +{
> +    return PR_TRUE;
> +}

Hmm, do we actually fail if we return FALSE here? We need to keep these entry points, but if something actually uses them, they should get a PR_FALSE return if we didn't actually verify the library.

If we were keeping the 'fake' FIPS mode (do all the POST checks, keep the FIPS plumbing, just disable the SELF tests) like I recommended, then this would be correct, but if we aren't faking FIPS mode, we definitely should return PR_FALSE for both of these functions.

Comment 7

[Franziskus Kiefer [:franziskus]]

Yeah, the PRNG fix is unrelated. It was only triggered by changes in here. I'll pull that out.

> ::: lib/freebl/shvfy.c
> Hmm, do we actually fail if we return FALSE here?

Yes we should fail. That shouldn't break anything. I changed that.

Comment 8

[Franziskus Kiefer [:franziskus]]

Attachment: fips-startup-tests.patch v2

Return false now in the integrity check functions when FIPS is disabled.
I pulled out the RNG change to make this cleaner.

Comment 9

[Robert Relyea]

Comment on attachment 8898718 [details] [diff] [review]
fips-startup-tests.patch v2

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

r+ I also see you removed the RNG changes. I'm OK with the idea of them, but we do need to make sure we can reinitialize once we've finalized them.

::: lib/freebl/shvfy.c
@@ +555,5 @@
> +PRBool
> +BLAPI_VerifySelf(const char *name)
> +{
> +    return PR_FALSE;
> +}

Thanks!

Comment 10

[Franziskus Kiefer [:franziskus]]

Thanks Bob!

https://hg.mozilla.org/projects/nss/rev/711b3866b2ce311baa27316c53aeeab6f7980e86
https://hg.mozilla.org/projects/nss/rev/30c6078f307516729317acc64a0f969823db2cde

Comment 11

[Kai Engert (:KaiE:)]

Attachment: all-sh-fips.patch

We should fix all.sh to automatically run or skip the fips.sh test, based on build config and environment.

If the environment sets NSS_FORCE_FIPS, can we run fips.sh and also automatically set the environment variables NSS_TEST_ENABLE_FIPS?

Comment 12

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8899451 [details] [diff] [review]
all-sh-fips.patch

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

lgtm

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 8899451 [details] [diff] [review]
all-sh-fips.patch

https://hg.mozilla.org/projects/nss/rev/0ca145683a70

Comment 14

[Kai Engert (:KaiE:)]

Reopening, the commits from comment 10 resulted in the SSL tests no longer being executed (and the test scripts didn't detect it, which is another bug in the test scripts we should fix).

Running tests for ssl
TIMESTAMP ssl BEGIN: Mon Aug 21 10:56:57 UTC 2017
ssl.sh: SSL tests ===============================
ssl.sh: Error: Unknown server mode 1

Comment 15

[Kai Engert (:KaiE:)]

This part of your commit seems to set an incorrect variable.

NSS_SSL_TESTS contains a list of tests to be executed, which you replace with "1". Which variable did you really want to set?

+  // We don't run FIPS SSL tests
+  if (task.tests == "ssl") {
+    if (!task.env) {
+      task.env = {};
+    }
+    task.env.NSS_SSL_TESTS = "1";
+  }

Comment 16

[Kai Engert (:KaiE:)]

Attachment: 1370667-fail-on-invalid-ssl-test.patch

How about this approach to abort the tests, if invalid/unknown tests are requested?

Comment 17

[Franziskus Kiefer [:franziskus]]

This enables SSL tests again and makes sure that all.sh fails when something like this happens again, i.e. NSS_SSL_TESTS has invalid values.

https://hg.mozilla.org/projects/nss/rev/c4850aa1c407e7dad4f94d28676d92ac75eeacf6
https://hg.mozilla.org/projects/nss/rev/31dfdf2057f881332329fd5df5b884b5fa60603e

Comment 18

[Kai Engert (:KaiE:)]

Why did you introduce new symbol NSS_FIPS_DISABLED ?

Why did you define NSS_FIPS_DISABLED only in gyp ?

Why didn't you reuse build flag NSS_FORCE_FIPS ?

Comment 19

[Kai Engert (:KaiE:)]

Let's cleanup in bug 1402410, which showed a difference of behavior in make and gyp builds, non-fips.