besser82/libxcrypt

Opening /dev/urandom without O_NOFOLLOW

Closed this issue · 4 comments

noloader commented

Hi Everyone,

A quick comment about util-get-random-bytes.c (https://github.com/besser82/libxcrypt/blob/develop/lib/util-get-random-bytes.c#L135). Some distros harden the call to open by including O_NOFOLLOW in the open flags:

int fd = open ("/dev/urandom", O_RDONLY|O_CLOEXEC);

Distros that harden that include Android and FreeBSD. I was surprised to see OpenWall did not include the hardening.

I know SolarDesigner is very meticulous. I want to ensure it is by design, and not an oversight.

solardiz commented

Hi @noloader. Do Android and FreeBSD really use O_NOFOLLOW when opening /dev/urandom? If so, why do they do it? And where exactly (in their codebases)?

I don't know why you refer to me here. I was not the one to write that part of libxcrypt. However, I wouldn't use O_NOFOLLOW there even if I did write it. I see no reason to - /dev is supposed to be trusted; if it isn't then we have bigger issues anyway; and if /dev is not compromised but has urandom as a symlink, the sysadmin (or distro) probably knew what they were doing and why.

zackw commented

As the person who did write this code, I concur with @solardiz. The contents of /dev are implicitly trusted. I can think of at least two legitimate reasons for /dev/urandom to be a symlink. I can even think of legitimate reasons for /dev/urandom to not be a character device (e.g. a user space CSPRNG daemon has taken over the job, so it's a pipe or something).

I wouldn't be surprised to learn that Android, the distribution, has a policy of patching O_NOFOLLOW into every open call unless there's a known need for that call to follow symlinks. That's fine for them, as they get to decide what their /dev will be like, but not appropriate for us to adopt. FreeBSD might have a similar policy but it would surprise me in that case, as they're trying to stick to the general-purpose Unix tradition.

I would also like to know where these patches are in the Android and FreeBSD codebases, and what their documented or discussed rationale is. (And the same for any other instance you can dig up.)

noloader commented

Thanks @solardiz, @zackw.

I don't have good answers to your questions. I think I need to ping the FreeBSD folks to get a definitive answer.

In the past, I believe I've read the FreeBSD folks want to make sure the character device is created correctly, and not doing something unexpected like symlinking to /dev/zero.

But if folks are happy with things, I think this issue can be closed.

In the meantime, here is an example from the FreeBSD folks in their Unbound patch. Source code for Unbound in the FreeBSD tree is at https://github.com/freebsd/freebsd-src/blob/main/contrib/unbound/compat/getentropy_linux.c#L212, and the patch is at https://www.freebsd.org/security/patches/SA-20%3A19/unbound.11.3.patch.

solardiz commented

Yes, I think we should just close this issue.

here is an example from the FreeBSD folks in their Unbound patch

Thanks. So they do O_NOFOLLOW | O_CLOEXEC. This makes me wonder why not also O_NOCTTY - the only flag/behavior I'd have wanted to be the default in kernel or libc, if it were not for historical reasons.

  • 👍1