Page MenuHomeFreeBSD

Fortuna: fix a correctness issue in reseed (fortuna_pre_read)
ClosedPublic

Authored by cem on Sep 1 2018, 5:22 AM.
Tags
None
Referenced Files
Unknown Object (File)
Nov 22 2023, 8:26 PM
Unknown Object (File)
Nov 13 2023, 12:13 PM
Unknown Object (File)
Oct 29 2023, 8:19 PM
Unknown Object (File)
Oct 19 2023, 5:34 AM
Unknown Object (File)
Sep 29 2023, 12:20 AM
Unknown Object (File)
Sep 19 2023, 2:51 PM
Unknown Object (File)
Aug 29 2023, 7:23 AM
Unknown Object (File)
Aug 29 2023, 7:14 AM
Subscribers

Details

Summary

'i' counts the number of pools included in the array 's'. Passing 'i+1' to
reseed_internal() as the number of blocks in 's' is a bogus overrun of the
initialized portion of 's' -- technically UB.

I found this via code inspection, referencing §9.5.2 "Pools" of the Fortuna
chapter, but I would expect Coverity to notice the same issue.
Unfortunately, it doesn't appear to.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Sep 1 2018, 10:13 AM

Ping secteam -- I'd like to apply this :-).

I have no objection. I'm not sure I'm qualified to weigh in on it. If markm is okay with it, then I would go ahead with it.

I have no objection. I'm not sure I'm qualified to weigh in on it. If markm is okay with it, then I would go ahead with it.

I'd like a quick double-check of my logic :-). The intuition is: the directly preceding loop iterates for (i = 0; i < RANDOM_FORTUNA_NPOOLS; i++) {, populating the ith logical element of array s, and sometimes breaks early. The invariant is that i at loop exit is always 1 greater than the last index we populated.

This is obviously true when we exit the loop due to the loop condition. And re: the explicit break condition, the population of element i and break are mutually exclusive in a single step.

'Last valid index + 1' translates directly into 'number of populated elements.' The section of line changed in this diff is the number of elements of s to process.

Does that make sense? Thanks!

This revision was automatically updated to reflect the committed changes.