1560329 - drbg: add continuous self-test on entropy source

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Daiki Ueno [:ueno]]

Attachment: nss-drbg-continous-self-test.patch

FIPS 140-2 section 4.9.2 requires a continuous self test of the entropy source, used as an input to DRBG. As getentropy() doesn't provide any checks on the output, the attached patch adds the self test at caller side.

Comment 1

[Daiki Ueno [:ueno]]

Attachment: nss-drbg-continous-self-test-v2.patch

Sorry, the previous patch was incorrect. According to FIPS 140-2, the check needs to be done against consecutive "blocks" in the same size. So the entropy gathering function must iteratively call getentropy (through RNG_SystemRNG).

Comment 2

[Daiki Ueno [:ueno]]

Attachment: nss-unix-random-afalg.patch

This is an alternative approach to the issue. Assuming the kernel DRBG accessed through AF_ALG has continuous random number generator test already, it prefers it over getentropy():
http://patchwork.lab.bos.redhat.com/patch/260889/

Comment 3

[Daiki Ueno [:ueno]]

Attachment: Bug 1560329, drbg: perform continuous test on entropy source

FIPS 140-2 section 4.9.2 requires a conditional self test to check that consecutive entropy blocks from the system are different. As neither getentropy() nor /dev/urandom provides that check on the output, this adds the self test at caller side.

Comment 4

[Robert Relyea]

Comment on attachment 9073209 [details] [diff] [review]
nss-unix-random-afalg.patch

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

Only one bikeshedding nit. In the old days it would be a stronger objection, but I don't think we depend on older C compiliers at this point. It's more of a style issue (this kind of usage does not show up in other locations in NSS.

::: lib/freebl/unix_urandom.c
@@ +42,5 @@
> +    struct sockaddr_alg sa = {
> +        .salg_family = AF_ALG,
> +        .salg_type = "rng",
> +        .salg_name = "stdrng"
> +    };

This is very C++ish for such a low level C function.

Comment 5

[Daiki Ueno [:ueno]]

Thank you for the review, but the AF_ALG patch has a couple of issues:

• there is a descriptor leak if accept fails
• recvmsg actually fails unless we explicitly call setsockopt to seed 0 byte, while the documentation says it is not necessary:
  http://www.chronox.de/crypto-API/crypto/userspace-if.html#random-number-generator-api

The fixes are trivial but I am rather leaning toward the other approach so as to avoid unnecessary assumptions on the kernel behavior.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:ueno, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 7

[Daiki Ueno [:ueno]]

Pushed as:
https://hg.mozilla.org/projects/nss/rev/934c8d0e7aba

Comment 8

[Daiki Ueno [:ueno]]

Backed out for new LSan failures:
https://hg.mozilla.org/projects/nss/rev/34a254dd1357

Comment 9

[Martin Thomson [:mt:]]

Daiki, I think that those LSAN errors are a problem with taskcluster. The taskcluster team suggested a fix that I'm going to trial, but I hadn't had time to open the bug. I think that you can re-land these.

Comment 10

[Martin Thomson [:mt:]]

Bug 1579290 opened to track the LSAN problem.

Comment 11

[Daiki Ueno [:ueno]]

Thank you, but I was suspecting that the patch itself might have a leak because the try result changes with/without the patch:

• With: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=91c83148ef8c4e5bdeff20ee3da1b7c13770cc11
• WIthout: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=3b32fd3fca264907282f56f8118abfd6e9eb38eb

Yes, the patch from bug 1579290 fixes the issue, but doesn't it also disable LSan when running tests?

Comment 12

[Daiki Ueno [:ueno]]

Re-landed now. Thank you for the CI fixes!
https://hg.mozilla.org/projects/nss/rev/c66dd879d16a