RESOLVED FIXED 146473
Errors in read() are not handled in WTF::cryptographicallyRandomValuesFromOS.
https://bugs.webkit.org/show_bug.cgi?id=146473
Summary Errors in read() are not handled in WTF::cryptographicallyRandomValuesFromOS.
Keith Miller
Reported 2015-06-30 14:49:02 PDT
Currently, we do not check for errors when reading / opening files in WTF::cryptographicallyRandomValuesFromOS. This should be remedied by checking if an EAGAIN or EINTR has occurred and continuing to get more data.
Attachments
Patch (3.12 KB, patch)
2015-06-30 14:54 PDT, Keith Miller
no flags
Patch (3.14 KB, patch)
2015-06-30 15:00 PDT, Keith Miller
fpizlo: review-
fpizlo: commit-queue-
Patch (3.14 KB, patch)
2015-06-30 15:12 PDT, Keith Miller
msaboff: review-
Patch (3.18 KB, patch)
2015-06-30 15:40 PDT, Keith Miller
no flags
Patch (3.06 KB, patch)
2015-06-30 15:48 PDT, Keith Miller
no flags
Patch (3.07 KB, patch)
2015-06-30 16:31 PDT, Keith Miller
no flags
Keith Miller
Comment 1 2015-06-30 14:54:22 PDT
Keith Miller
Comment 2 2015-06-30 15:00:04 PDT
Created attachment 255860 [details] Patch Changed the name of the crashing functions to make it clear they were related to /dev/urandom
Keith Miller
Comment 3 2015-06-30 15:03:53 PDT
Filip Pizlo
Comment 4 2015-06-30 15:04:11 PDT
Comment on attachment 255860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255860&action=review > Source/WTF/wtf/OSRandomSource.cpp:53 > +NEVER_INLINE NO_RETURN_DUE_TO_CRASH static void crashUnableToOpenURandom() > +{ > + CRASH(); > +} > + > +NEVER_INLINE NO_RETURN_DUE_TO_CRASH static void crashUnableToReadFromURandom() > +{ > + CRASH(); > +} Is there a good reason for these, isntead of just using CRASH or RELEASE_ASSERT_NOT_REACHED directly? > Source/WTF/wtf/OSRandomSource.cpp:72 > + if (currentRead < 0 && !(errno == EAGAIN || errno == EINTR)) > + crashUnableToReadFromURandom(); This seems broken - if the currentRead is -1, then we don't want to add it to amountRead.
Michael Saboff
Comment 5 2015-06-30 15:06:17 PDT
Comment on attachment 255860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255860&action=review > Source/WTF/wtf/OSRandomSource.cpp:63 > + int fd; > + // We need to check for both EAGAIN and EINTR since on some systems /dev/urandom > + // is blocking and on others it is non-blocking. > + do { > + fd = open("/dev/urandom", O_RDONLY, 0); > + } while (fd < 0 && (errno == EAGAIN || errno == EINTR)); I don't think there is a need to check for the EAGAIN and EINTR cases for open(). EAGAIN is documented to only happen for slave ptys and EINTR due to a signal.
Keith Miller
Comment 6 2015-06-30 15:12:32 PDT
Created attachment 255864 [details] Patch Fixed issue with currentRead being < 0
Michael Saboff
Comment 7 2015-06-30 15:14:56 PDT
Comment on attachment 255864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255864&action=review > Source/WTF/wtf/OSRandomSource.cpp:63 > + int fd; > + // We need to check for both EAGAIN and EINTR since on some systems /dev/urandom > + // is blocking and on others it is non-blocking. > + do { > + fd = open("/dev/urandom", O_RDONLY, 0); > + } while (fd < 0 && (errno == EAGAIN || errno == EINTR)); Remove this loop. I don't think there is a need to check for the EAGAIN and EINTR cases for open(). EAGAIN is documented to only happen for slave ptys and EINTR due to a signal.
Geoffrey Garen
Comment 8 2015-06-30 15:22:11 PDT
Comment on attachment 255860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255860&action=review >> Source/WTF/wtf/OSRandomSource.cpp:53 >> +} > > Is there a good reason for these, isntead of just using CRASH or RELEASE_ASSERT_NOT_REACHED directly? I requested this so that we get back definitive crash traces that differentiate the one kind of failure from the other. Currently, our crash traces do not tell us which failure happened. > Source/WTF/wtf/OSRandomSource.cpp:70 > + ssize_t currentRead = read(fd, buffer+amountRead, length-amountRead); Our style requires spacing around + and -.
Keith Miller
Comment 9 2015-06-30 15:40:32 PDT
Created attachment 255868 [details] Patch Removed checks for EINTR and EAGAIN from the open call.
Keith Miller
Comment 10 2015-06-30 15:48:08 PDT
Created attachment 255869 [details] Patch For some reason the other patch didn't update.
Geoffrey Garen
Comment 11 2015-06-30 16:00:41 PDT
Comment on attachment 255869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255869&action=review > Source/WTF/wtf/OSRandomSource.cpp:67 > + if (currentRead < 0 && !(errno == EAGAIN || errno == EINTR)) I think it would be nicer to check for -1 explicitly, rather than implicitly by including all negatives in the test. The documentation states that read will return -1, 0, or a positive number, and it is nice to be precise. You can simplify the logic here a bit and avoid testing currentRead twice like so: if (currentRead == -1) { if (!(errno == EAGAIN || errno == EINTR)) crashUnableToReadFromURandom(); } else amountRead += currentRead; This helps to clearly separate the failure case from the success case.
Keith Miller
Comment 12 2015-06-30 16:09:54 PDT
Comment on attachment 255869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255869&action=review >> Source/WTF/wtf/OSRandomSource.cpp:67 >> + if (currentRead < 0 && !(errno == EAGAIN || errno == EINTR)) > > I think it would be nicer to check for -1 explicitly, rather than implicitly by including all negatives in the test. The documentation states that read will return -1, 0, or a positive number, and it is nice to be precise. > > You can simplify the logic here a bit and avoid testing currentRead twice like so: > > if (currentRead == -1) { > if (!(errno == EAGAIN || errno == EINTR)) > crashUnableToReadFromURandom(); > } else > amountRead += currentRead; > > This helps to clearly separate the failure case from the success case. That makes sense. Do you think the code you wrote is clearer than: if (currentRead >= 0) amountRead += currentRead; else if (!(errno == EAGAIN || errno == EINTR)) crashUnableToReadFromURandom(); I guess it depends on whether or not you think about the success or failure case first. It also, doesn't make the -1 condition explicit.
Geoffrey Garen
Comment 13 2015-06-30 16:23:00 PDT
> That makes sense. Do you think the code you wrote is clearer than: > > if (currentRead >= 0) > amountRead += currentRead; > else if (!(errno == EAGAIN || errno == EINTR)) > crashUnableToReadFromURandom(); I like both. I would still use " != -1" instead of "> = 0". -1 is a sentinel value and I think it helps readability to call it out explicitly.
Keith Miller
Comment 14 2015-06-30 16:31:51 PDT
Created attachment 255872 [details] Patch Refactored code slightly.
WebKit Commit Bot
Comment 15 2015-06-30 17:37:14 PDT
Comment on attachment 255872 [details] Patch Clearing flags on attachment: 255872 Committed r186151: <http://trac.webkit.org/changeset/186151>
WebKit Commit Bot
Comment 16 2015-06-30 17:37:19 PDT
All reviewed patches have been landed. Closing bug.
Geoffrey Garen
Comment 17 2015-08-17 10:41:59 PDT
Geoffrey Garen
Comment 18 2015-08-17 10:42:11 PDT
LOL. arc4random_buf just uses /dev/random, so we have only moved the goalposts, and we have not fixed this bug :(.
Note You need to log in before you can comment on or make changes to this bug.