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

RESOLVED FIXED in 3.33

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: Ehsan, Assigned: fkiefer)

Tracking

(Blocks: 1 bug)

other
3.33
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 months ago
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?
Flags: needinfo?(dkeeler)
(Reporter)

Updated

9 months ago
Summary: Does → Does freebl_fipsPowerUpSelfTest() have a good reason to run during startup (or ever for that matter)?
(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).
Flags: needinfo?(dkeeler)
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.
Assignee: nobody → franziskuskiefer
Component: Security: PSM → Libraries
Priority: -- → P1
Product: Core → NSS
Target Milestone: --- → 3.32
Version: unspecified → other
(Reporter)

Comment 3

8 months ago
ping? Comment 2 seems like a nice low hanging fruit if we can fix this...
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.
Status: NEW → ASSIGNED
Created attachment 8897402 [details] [diff] [review]
fips-startup-tests.patch

https://nss-review.dev.mozaws.net/D348
Attachment #8897402 - Flags: review?(rrelyea)

Comment 6

6 months ago
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.
Attachment #8897402 - Flags: review?(rrelyea) → review-
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.
Created attachment 8898718 [details] [diff] [review]
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.
Attachment #8897402 - Attachment is obsolete: true
Attachment #8898718 - Flags: review?(rrelyea)

Comment 9

6 months ago
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!
Attachment #8898718 - Flags: review?(rrelyea) → review+
Thanks Bob!

https://hg.mozilla.org/projects/nss/rev/711b3866b2ce311baa27316c53aeeab6f7980e86
https://hg.mozilla.org/projects/nss/rev/30c6078f307516729317acc64a0f969823db2cde
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: 3.32 → 3.33

Comment 11

6 months ago
Created attachment 8899451 [details] [diff] [review]
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?
Attachment #8899451 - Flags: review?(franziskuskiefer)
Comment on attachment 8899451 [details] [diff] [review]
all-sh-fips.patch

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

lgtm
Attachment #8899451 - Flags: review?(franziskuskiefer) → review+

Comment 14

6 months ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 15

6 months ago
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

6 months ago
Created attachment 8899812 [details] [diff] [review]
1370667-fail-on-invalid-ssl-test.patch

How about this approach to abort the tests, if invalid/unknown tests are requested?
Attachment #8899812 - Flags: review?(franziskuskiefer)
Attachment #8899812 - Flags: review?(franziskuskiefer) → review+
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
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago6 months ago
Resolution: --- → FIXED

Updated

6 months ago
Depends on: 1398756

Comment 18

5 months ago
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

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

Updated

4 months ago
Blocks: 1409516
You need to log in before you can comment on or make changes to this bug.