Page MenuHomeFreeBSD

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

Authored by cem on Sep 1 2018, 5:22 AM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem created this revision.Sep 1 2018, 5:22 AM
markm accepted this revision.Sep 1 2018, 10:13 AM
This revision is now accepted and ready to land.Sep 1 2018, 10:13 AM
cem added a comment.Oct 20 2018, 9:25 PM

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

gordon added a subscriber: gordon.Oct 20 2018, 10:57 PM

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.

cem added a comment.Oct 20 2018, 11:57 PM

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.