1258669 - add checks for OPT builds in unix_rand.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

unix_rand.c relies on PORT_Assert to check for successful file access. That won't work in optimised builds such that |read| is called on those file pointers even if the file wasn't be opened successfully.

Comment 1

[Matt Wobensmith [:mwobensmith][:matt:]]

Franziskus, can you comment on the potential security impact of this? Is it exploitable?

Comment 2

[Franziskus Kiefer [:franziskus]]

I think this particular issue is relatively low risk as NSS will read something, but not actually use it.

> PORT_Assert(fd != -1);
> while (limit > fileBytes) {
>   bytes = PR_MIN(sizeof buffer, limit - fileBytes);
>   bytes = read(fd, buffer, bytes); // read with fd == -1
>   if (bytes <= 0) // bytes == -1 here so we bail
>     break;

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: urand_read_file.patch

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8733841 [details] [diff] [review]
urand_read_file.patch

Review of attachment 8733841 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine as a fix but I'm worried about the behavior when opening /dev/urandom fails. The RNG_SystemRNG() callers seem to properly fail when we read zero bytes, all RNG_FileUpdate() call sites however do not seem to check if the call succeeded. I think that while we're here we probably should fix that or at least determine that it's fine to not check.

Comment 5

[Franziskus Kiefer [:franziskus]]

889116(In reply to Tim Taubert [:ttaubert] from comment #4)
> Comment on attachment 8733841 [details] [diff] [review]
> urand_read_file.patch
> 
> Review of attachment 8733841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems fine as a fix but I'm worried about the behavior when opening
> /dev/urandom fails. The RNG_SystemRNG() callers seem to properly fail when
> we read zero bytes, all RNG_FileUpdate() call sites however do not seem to
> check if the call succeeded. I think that while we're here we probably
> should fix that or at least determine that it's fine to not check.

I agree, but we have to be careful to do this only on systems where a certain that we have /dev/urandom. But lets do this in Bug 889116 that was filed for this.

Comment 6

[Franziskus Kiefer [:franziskus]]

landed as https://hg.mozilla.org/projects/nss/rev/e7b6fca4f014