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



2 years ago
a year ago


(Reporter: Ehsan, Assigned: franziskus)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(3 attachments, 1 obsolete attachment)



2 years ago
I was trying to figure out why FREEBL_GetVector() seems to take up so much time in this startup profile:

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)


2 years 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

Comment 3

2 years 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.

Comment 6

2 years ago
Comment on attachment 8897402 [details] [diff] [review]

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+.


::: 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

2 years 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;
> +}

Attachment #8898718 - Flags: review?(rrelyea) → review+
Thanks Bob!
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: 3.32 → 3.33
Created attachment 8899451 [details] [diff] [review]

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

If the environment sets NSS_FORCE_FIPS, can we run and also automatically set the environment variables NSS_TEST_ENABLE_FIPS?
Attachment #8899451 - Flags: review?(franziskuskiefer)
Comment on attachment 8899451 [details] [diff] [review]

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

Attachment #8899451 - Flags: review?(franziskuskiefer) → review+
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 tests =============================== Error: Unknown server mode 1
Resolution: FIXED → ---
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";
+  }
Created attachment 8899812 [details] [diff] [review]

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 fails when something like this happens again, i.e. NSS_SSL_TESTS has invalid values.
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED


2 years ago
Depends on: 1398756
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 ?
Let's cleanup in bug 1402410, which showed a difference of behavior in make and gyp builds, non-fips.
Blocks: 1409516
You need to log in before you can comment on or make changes to this bug.