[sssd PR#838][opened] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/838
Author: alexey-tikhonov
 Title: #838: FIPS140 compliant usage of PRNG
Action: opened

PR body:
"""
Resolves https://pagure.io/SSSD/sssd/issue/4024
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/838/head:pr838
git checkout pr838

[sssd PR#838][synchronized] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/838
Author: alexey-tikhonov
 Title: #838: FIPS140 compliant usage of PRNG
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/838/head:pr838
git checkout pr838

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

alexey-tikhonov commented:
"""
Rebased on current master
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-505809444

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[mzidek-rh]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

mzidek-rh commented:
"""
Nitpick: You used tabs instead of spaces in the Makefile.am. It is better to be consistent
and use spaces.
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-505831595

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

alexey-tikhonov commented:
"""
>  Nitpick: You used tabs instead of spaces in the Makefile.am. It is
> better to be consistent and use spaces.
>  
Could you please elaborate on this?
```
git show c86a8033c6628c51d478e44d8dc4a0726988c826 Makefile.am | cat -T
...
-^I^I^Isrc/util/crypto/sss_crypto.c \
-^I^I^Isrc/util/atomic_io.c \
-^I^I^I$(NULL)
+                        src/util/crypto/nss/nss_prng.c \
+                        src/util/atomic_io.c \
+                        $(NULL)
...
-^I^I^Isrc/util/crypto/sss_crypto.c \
-^I^I^Isrc/util/atomic_io.c \
-^I^I^I$(NULL)
+                        src/util/crypto/libcrypto/crypto_prng.c \
+                        src/util/atomic_io.c \
+                        $(NULL)
```
It is other way around: I replaced tabs with spaces.
But I didn't touch it everywhere to keep diff small.
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-505836890

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[mzidek-rh]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

mzidek-rh commented:
"""
>  > Nitpick: You used tabs instead of spaces in the Makefile.am. It
> is better to be consistent and use spaces.
>  
>  Could you please elaborate on this?
>  
>  ```
>  git show c86a8033c6628c51d478e44d8dc4a0726988c826 Makefile.am | cat -T
>  ...
>  -^I^I^Isrc/util/crypto/sss_crypto.c \
>  -^I^I^Isrc/util/atomic_io.c \
>  -^I^I^I$(NULL)
>  +                        src/util/crypto/nss/nss_prng.c \
>  +                        src/util/atomic_io.c \
>  +                        $(NULL)
>  ...
>  -^I^I^Isrc/util/crypto/sss_crypto.c \
>  -^I^I^Isrc/util/atomic_io.c \
>  -^I^I^I$(NULL)
>  +                        src/util/crypto/libcrypto/crypto_prng.c \
>  +                        src/util/atomic_io.c \
>  +                        $(NULL)
>  ```
>  
>  It is other way around: I replaced tabs with spaces.
>  But I didn't touch it everywhere to keep diff small.
>  
Ahm yes, sorry, disregard my comment, I was reading the diff wrong. The code LGTM, but I
would prefer if someone else reviewed the FIPS related patches.
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-506340376

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[frozencemetery]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

frozencemetery commented:
"""
IMO, you should be using `getrandom()` (with no flags) in preference to srand, or reading
/dev/[u]random, etc.  You're guaranteed to have getrandom from kernel 3.17 onward. 
This matches what we do in krb5.
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-506437810

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

alexey-tikhonov commented:
"""
>  IMO, you should be using `getrandom()` (with no flags) in preference
> to srand, or reading /dev/[u]random, etc. You're guaranteed to have getrandom from
> kernel 3.17 onward. This matches what we do in krb5.
>  
Support of NSS in SSSD is going to be deprecated very soon, so we don't care what to
put there.

For OpenSSL case `RAND_bytes()` is used, and this is what FIPS really wants. The
`getrandom()` or `/dev/(u)random` are not approved DRBG.

srand()/rand() are only used as a fallback in case `RAND_bytes()` fails. (And my
understanding is, this (fail) is only possible if there is no entropy available so it
doesn't make any sense to try reading "/dev/[u]random" as those are used by
OpenSSL to draw entropy from)

In regards of `getrandom()`: `Support was added to glibc in version 2.25`. It is possible
to check this in compile time and use `getrandom()` if available, but again, I do not
think it makes sense for "fallback" branch...
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-506443909

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[frozencemetery]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

frozencemetery commented:
"""
In the FIPS case, you need to fail if RAND_bytes() fails; otherwise you're
noncompliant.  If you want to use that in non-FIPS as well, I don't know why you'd
bother with fallback at all - just fail if RAND_bytes() fails.  If you don't want to
use RAND_bytes() in the non-FIPS case, then you should use getrandom().

Do you actually support any platforms which wouldn't have it?  Keep in mind that el7
does support the getrandom syscall(), which is what we do in krb5 for this reason.

But really, if you don't have any entropy, you shouldn't be doing crypto, full
stop.
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-506479147

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

alexey-tikhonov commented:
"""
>  In the FIPS case, you need to fail if RAND_bytes() fails; otherwise
> you're noncompliant.
>  
That's correct, but only for random numbers used in security relevant functionality.

This is exactly the case with `sss_generate_csprng_buffer()` function, which might be used
in security relevant functionality, thus it fails if `RAND_bytes()` fails.

But `sss_rand()` is not used in security relevant functionality. Hence, I just do not see
a reason to fail instead of fallback to `rand()`.
The reasons to introduce this wrapper are:
(1) it simplifies code as there is no need to call `srand()` explicitly
(2) it reduces amount of covscan complains
(3) usage of better quality PRNG won't hurt even if doesn't make much sense

But I agree it might be good idea to put comments regarding usage restriction in the
header.
And may be I could add `int sss_cs_rand` that would fail instead of fallback. But I am not
eager to do so since it would not be used currently.

>  If you want to use that in non-FIPS as well, I don't know why
> you'd bother with fallback at all - just fail if RAND_bytes() fails.
>  
1) We definitely do not want to check in run time if system is in FIPS mode or not. That
would complicate stuff for no good reason.
2) And what is the reason to fail instead of fallback, if caller doesn't actually need
crypto strong random number? (Be it because of functionality context or because of
"in non-FIPS"). Again, that would complicate function usage for no reason (need
to check return code and handle it appropriately).

>  getrandom(). Do you actually support any platforms which wouldn't
> have it?
>  
IMO, there is just absolutely no sense to prefer `getrandom()` over available
`RAND_bytes()` in our case.

>  Keep in mind that el7 does support the getrandom syscall(), which is
> what we do in krb5 for this reason.
>  
Yes, I have [this](https://mivehind.net/2017/05/14/please-remove-your-prng/) read.

"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-506759590

[sssd PR#838][+Changes requested] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

Label: +Changes requested

[sssd PR#838][synchronized] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/838
Author: alexey-tikhonov
 Title: #838: FIPS140 compliant usage of PRNG
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/838/head:pr838
git checkout pr838

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

alexey-tikhonov commented:
"""
>  This is exactly the case with `sss_generate_csprng_buffer()`
> function, which might be used in security relevant functionality, thus it fails if
> `RAND_bytes()` fails.
>  But `sss_rand()` is not used in security relevant functionality. Hence, I just do not see
> a reason to fail instead of fallback to `rand()`.
>  But I agree it might be good idea to put comments regarding usage restriction in the
> header.
>  
Done.
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-507275540

[sssd PR#838][-Changes requested] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

Label: -Changes requested

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

jhrozek commented:
"""
Looks like this PR needs to be rebased, so I'm adding Changes requested. Nonetheless,
Pavel seemed to agree to take a look.
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-508708579

[sssd PR#838][+Changes requested] FIPS140 compliant usage of PRNG

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

Label: +Changes requested

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

pbrezina commented:
"""
I agree with Alexey's statements, i.e. it is not necessary to fail in cases where
security concerns are not relevant and just simple random number is needed (for example to
compute a random delay to distribute load of start up tasks etc.).

That being said, there are few minor coding style violations but otherwise I accept these
patches. And also it needs rebasing on current master.

* **Patch: util/crypto: added sss_rand()**
  * there are multiple new lines around `sss_rand()` functions, only one empty line should
be there
  * opening bracket should be on the first line
```c
if (expr)    if (expr) {
{                   ....
    ...      }
}
```
* **Patch: crypto/libcrypto/crypto_nite.c: memory leak fixed**
  * Please, do not use `if (ptr)` but rather `if (ptr == NULL)` (and vise versa for
non-null case)
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-509135784

[sssd PR#838][synchronized] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

   URL: https://github.com/SSSD/sssd/pull/838
Author: alexey-tikhonov
 Title: #838: FIPS140 compliant usage of PRNG
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/838/head:pr838
git checkout pr838

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

alexey-tikhonov commented:
"""
Hi @pbrezina,

Thanks for the review.

>  there are few minor coding style violations but otherwise I accept
> these patches. 
>  
I rebased patches and addressed all comments.

>  there are multiple new lines around `sss_rand()` functions, only one
> empty line should be there
>  
I changed this as well, but actually I was unable to find such a requirement in [Coding
guidelines](https://docs.pagure.org/SSSD.sssd/developers/coding_style.html).
Do you think guide should be updated?


"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-509312412

[sssd PR#838][-Changes requested] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

Label: -Changes requested

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

pbrezina commented:
"""
Thank you. Ack.

>  >  there are multiple new lines around sss_rand() functions, only
> one empty line should be there
>  
>  I changed this as well, but actually I was unable to find such a requirement in Coding
> guidelines.
>  Do you think guide should be updated?
>  
Well, it's not explicitly written but it is shown:
https://docs.pagure.org/SSSD.sssd/developers/coding_style.html#if-statements
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-509537742

[sssd PR#838][+Accepted] FIPS140 compliant usage of PRNG

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

Label: +Accepted

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[alexey-tikhonov]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

alexey-tikhonov commented:
"""
>  > > there are multiple new lines around sss_rand() functions,
> only one empty line should be there
>  > 
>  > I changed this as well, but actually I was unable to find such a requirement in
> Coding guidelines.
>  > Do you think guide should be updated?
>  
>  Well, it's not explicitly written but it is shown:
>  https://docs.pagure.org/SSSD.sssd/developers/coding_style.html#if-statements
>  
This is a good example of braces placement rules.
But I hardly can understand this as an example of "newlines rule".
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-509556008

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

pbrezina commented:
"""
Right, sorry I misread your comment. It is not said explicitly.
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-509590953

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

jhrozek commented:
"""
* master:
 * 548ea574645f405307b14bb1113d66f9da1abf2b
 * bfc02ea2cdc111bfb8df044f359655cce3337ccd
 * 93d0aba5a49fdf9df87037eba42986eee02d1d35
 * 9f4b7d9fbec9a7746fc39d9e69054ac469c14d19
 * e839acd1fda573b11170a3d074f60eff9d654008
 * 8be1a0e829ba9eaf6769622adcb3be827575f551
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-511373367

[sssd PR#838][closed] FIPS140 compliant usage of PRNG

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/838
Author: alexey-tikhonov
 Title: #838: FIPS140 compliant usage of PRNG
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/838/head:pr838
git checkout pr838

[sssd PR#838][+Pushed] FIPS140 compliant usage of PRNG

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

Label: +Pushed

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

jhrozek commented:
"""
* master:
 * 548ea574645f405307b14bb1113d66f9da1abf2b
 * bfc02ea2cdc111bfb8df044f359655cce3337ccd
 * 93d0aba5a49fdf9df87037eba42986eee02d1d35
 * 9f4b7d9fbec9a7746fc39d9e69054ac469c14d19
 * e839acd1fda573b11170a3d074f60eff9d654008
 * 8be1a0e829ba9eaf6769622adcb3be827575f551
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-511381691

[sssd PR#838][comment] FIPS140 compliant usage of PRNG

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/838
Title: #838: FIPS140 compliant usage of PRNG

jhrozek commented:
"""
(sorry for the two comments, I re-send a FF tab by mistake)
"""

See the full comment at https://github.com/SSSD/sssd/pull/838#issuecomment-511381773