#4024 Non FIPS140 compliant usage of PRNG
Closed: Fixed 4 years ago by atikhonov. Opened 4 years ago by atikhonov.

Ticket was cloned from Red Hat Bugzilla (product Red Hat Enterprise Linux 8): Bug 1720723

PRNG is used a lot in SSSD sources. Not all cases fall under notion of
"security relevant functionality".

But some does.
For example `util/secrets/secrets.c:generate_master_key()`: from my POV, this
code may be considered "security relevant", but it uses "/dev/urandom" to
generate encryption key, which is not FIPS approved method.

I think it is a good idea to introduce appropriate PRNG wrappers (backed by
RAND_bytes() in case of OpenSSL crypto backend) into `util/crypto` and to use
those wrappers in every applicable case where "raw" rand(), "/dev/urandom", etc
are used now (there are might be some exceptions since it is undesirable to
link some components like client libs and resolver with additional libraries).

Metadata Update from @atikhonov:
- Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1720723

4 years ago

Metadata Update from @atikhonov:
- Issue assigned to atikhonov

4 years ago

Metadata Update from @atikhonov:
- Issue tagged with: PR

4 years ago

Commit 93d0aba relates to this ticket

Commit 9f4b7d9 relates to this ticket

the change uses sss_rand() instead of rand() or rand_r(). 
However, sss_rand()'s return values are in the whole integer range, 
while man 3 rand says

       The  rand()  function returns a pseudo-random integer in the range 0 to
       RAND_MAX inclusive (i.e., the mathematical range [0, RAND_MAX]).

So the values returned by sss_rand() are not compatible with the expected values
returned in the context where rand() used to be.

( https://bugzilla.redhat.com/show_bug.cgi?id=1755643#c16 )

Metadata Update from @atikhonov:
- Issue status updated to: Open (was: Closed)

4 years ago

Copied from #4091:

Simo Sorce wrote:
"Through Fedora bugzilla https://bugzilla.redhat.com/show_bug.cgi?id=1755643 I've discovered the introduction of srand() in sss_crypto functions.

This is totally inappropriate and should be reverted asap."

Metadata Update from @atikhonov:
- Issue untagged with: PR

4 years ago

Hi @simo,

"Through Fedora bugzilla https://bugzilla.redhat.com/show_bug.cgi?id=1755643 I've discovered the introduction of srand() in sss_crypto functions.
This is totally inappropriate and should be reverted asap."

Actually this have been already discussed here. But let me summarize rationale again.

This whole patchset was driven by an idea to strive FIPS compliance in mind.

For the OpenSSL backend case RAND_bytes() is the only certified source of CSPRNG. But RAND_bytes() might fail and this fail should be handled. To be FIPS-compliant we can't fallback at all to anything else: srand() or getrandom() or reading /dev/(u)random -- those are not approved DRBGs. So the only way to handle fail of RAND_bytes() for security/FIPS relevant function is to fail completely.

Now, this sounds like an overkill for non security/FIPS relevant functionality. There were a lot of places thru the code that were using plain rand(), mostly for non security relevant functionality. Replacing all those rand() with a crypo-strong PRNG and handling a potential fail didn't make a sense. Still I saw some benefits to introduce a helper/wrapper to replace rand() (reasons are given in the PR comment by the link above).

Thus I decided to introduce two different functions:
* sss_generate_csprng_buffer() - to be used for security / FIPS relevant functionality
* sss_rand() - to be used for non security relevant functionality
I tried to emphasize this difference by the names of the functions and comments in the header file.

So sss_generate_csprng_buffer() fails if RAND_bytes() fails.

And sss_rand() falls back to srand()/rand() if RAND_bytes() fails.

I am reluctant to replace fallback part of sss_rand() with:
1) getrandom() due to issues with portability and blocking
2) reading /dev/(u)random because most probably RAND_bytes() failed due to absense of entropy and this would block caller
3) with anything else because chance of RAND_bytes() to fail are so low and sss_rand() is not intended to be used in sensitive codepaths anyway, so it would not worth an effort

But now I see there might be some questions:

(1) May be we should move sss_rand() out of util/crypto/?

(2) Are all uses of sss_rand() really non security relevant?
Should we replace some of sss_rand() calls with sss_generate_csprng_buffer() (even though previously in all those places plain rand() was used)?

You mentioned that usage of sss_rand() in murmur hash impl is security relevant. Could you please elaborate on this? I am asking because previously you mentioned that murmur hash itself is used in non security relevant functions (otherwise we would have to replace hash itself!). Thus I am making a conclusion that it is not important to use CSPRNG in murmur hash impl.

(3) What is really bad: sss_rand() may return any value (including negative), and it is used in the places that were using rand() that returns [0, RAND_MAX].
I checked all the places and difference RAND_MAX vs INT_MAX doesn't matter much, but negative values are not really expected in some places.
Thus I plan to return absolute value (with special handling of INT_MIN of course) in sss_rand() and to not strive full compatibility with [0, RAND_MAX]. What would you think?
(Btw, this also is only possible because sss_rand() is not intended to be used in FIPS relevant code, because I think any f(CSRPNG) is not CSRPNG, even if it is as simple as abs()...)

Hope this makes sense.

Hi @simo,

[...]

But now I see there might be some questions:
(1) May be we should move sss_rand() out of util/crypto/?

Definitely, it is misleading to keep a PRNG in crypto if it is not cryptographically strong.
The risk is of someone using it for cryptography or security related needs later on.

(2) Are all uses of sss_rand() really non security relevant?

I think at least the seeding of the murmur hash function is security relevant, as the randomization of the seed, there is used to prevent attacks based on predictable hashing.

Should we replace some of sss_rand() calls with sss_generate_csprng_buffer() (even though previously in all those places plain rand() was used)?

Where relevant, I would.

You mentioned that usage of sss_rand() in murmur hash impl is security relevant. Could you please elaborate on this?

See above.

I am asking because previously you mentioned that murmur hash itself is used in non security relevant functions (otherwise we would have to replace hash itself!).

The hash itself is not used for security purposes, it is just hash table. However the see drandomization is used to make the hashing unpredictable by an external attacker that wishes to DoS the system by causing many collisions. This is a security related use and I would prefer to use a non-predictable random number generator. srand() is predictable.

Thus I am making a conclusion that it is not important to use CSPRNG in murmur hash impl.

I do not agree. See above.

(3) What is really bad: sss_rand() may return any value (including negative), and it is used in the places that were using rand() that returns [0, RAND_MAX].
I checked all the places and difference RAND_MAX vs INT_MAX doesn't matter much, but negative values are not really expected in some places.
Thus I plan to return absolute value (with special handling of INT_MIN of course) in sss_rand() and to not strive full compatibility with [0, RAND_MAX]. What would you think?

It need to be a case by case evaluation, why was the change made in the first place, btw ?

(Btw, this also is only possible because sss_rand() is not intended to be used in FIPS relevant code, because I think any f(CSRPNG) is not CSRPNG, even if it is as simple as abs()...)
Hope this makes sense.

Note that FIPS is only a subset of security relevant algorithm, we must not be vulnerable in non-FIPS mode either, security is always important, FIPS is just a stricter mode for some stuff. So I would not conflate FIPS and Security in general, when discussing these matters as it may lead to incorrect conclusions.

(1) May be we should move sss_rand() out of util/crypto/?

Definitely, it is misleading to keep a PRNG in crypto if it is not cryptographically strong.

Ok.

(2) Are all uses of sss_rand() really non security relevant?

I think at least the seeding of the murmur hash function is security relevant, as the randomization of the seed, there is used to prevent attacks based on predictable hashing.

I am asking because previously you mentioned that murmur hash itself is used in non security relevant functions (otherwise we would have to replace hash itself!).

The hash itself is not used for security purposes, it is just hash table. However the see drandomization is used to make the hashing unpredictable by an external attacker that wishes to DoS the system by causing many collisions.

Murmur hash is used in idmap and memcache. I hardly can imagine an attacker to be able to manipulate input to cause enough collisions in both cases.

But ok, it is easier to be on a safe side and use CSPRNG here.

(3) What is really bad: sss_rand() may return any value (including negative), and it is used in the places that were using rand() that returns [0, RAND_MAX].
I checked all the places and difference RAND_MAX vs INT_MAX doesn't matter much, but negative values are not really expected in some places.
Thus I plan to return absolute value (with special handling of INT_MIN of course) in sss_rand() and to not strive full compatibility with [0, RAND_MAX]. What would you think?

It need to be a case by case evaluation, why was the change made in the first place, btw ?

Why did I replace plain rand() with a wrapper (sss_rand()) in non security relevant code?
The reasons to introduce this wrapper were:
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

Metadata Update from @atikhonov:
- Issue tagged with: PR

4 years ago
  • master
    • bb8b59d - Moved unsecure sss_rand() out of crypto lib
    • f3e89aa - MMAP_CACHE: use CSPRNG to init hash table seed

@atikhonov can this ticket be closed or is there some more work to do?

Metadata Update from @atikhonov:
- Custom field design_review adjusted to on
- Custom field mark adjusted to on
- Custom field patch adjusted to on
- Custom field review adjusted to on
- Custom field sensitive adjusted to on
- Custom field testsupdated adjusted to on
- Issue close_status updated to: Fixed
- Issue status updated to: Closed (was: Open)

4 years ago

@atikhonov can this ticket be closed or is there some more work to do?

Closed. All known issues were addressed to the best of my knowledge.

SSSD is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in SSSD's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/SSSD/sssd/issues/4995

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Login to comment on this ticket.

Metadata