1254334 - There are plans to deprecate readdir_r which is used by lib/freebl/unix_rand.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Elio Maldonado]

Attachment: unix_rand-readdir_r-deprecated.patch

As I was building nss-softoken for Fedora Rawhide (development branch) I ran into this:

gcc -o Linux4.3_x86_64_cc_glibc_PTH_64_OPT.OBJ/Linux_SINGLE_SHLIB/sysrand.o -c -O2 -fPIC -DLINUX2_1 -m64 -Wall -Werror -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -UDEBUG -DNDEBUG -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_64 -DFREEBL_NO_DEPEND -DFREEBL_USE_PRELINK -DNSS_X86_OR_X64 -DNSS_X64 -DNSS_BEVAND_ARCFOUR -DMPI_AMD64 -DMP_ASSEMBLY_MULTIPLY -DNSS_USE_COMBA -DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN -DUSE_HW_AES -DINTEL_GCM -DMP_API_COMPATIBLE -I/usr/include/nspr4 -I/usr/include/nss3 -I/usr/include/nspr4 -I../../../dist/Linux4.3_x86_64_cc_glibc_PTH_64_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -Impi -Iecl -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic sysrand.c
In file included from sysrand.c:16:0:
unix_rand.c: In function 'ReadOneFile':
unix_rand.c:1091:6: error: 'readdir_r' is deprecated [-Werror=deprecated-declarations]
      error = readdir_r(fd, &entry_dir, &result);
      ^~~~~
In file included from unix_rand.c:1033:0,
                 from sysrand.c:16:
/usr/include/dirent.h:183:12: note: declared here
 extern int readdir_r (DIR *__restrict __dirp,
            ^~~~~~~~~
cc1: all warnings being treated as errors
../../coreconf/rules.mk:388: recipe for target 'Linux4.3_x86_64_cc_glibc_PTH_64_OPT.OBJ/Linux_SINGLE_SHLIB/sysrand.o' failed

Searching I found the reason here

https://sourceware.org/ml/libc-alpha/2016-02/msg00093.html

After I applied the attached patch I could build. 

Later on I revisited the issue and didn't apply the patch and it built fine.
-------------------


....
gcc -o Linux4.3_x86_64_cc_glibc_PTH_64_OPT.OBJ/Linux_SINGLE_SHLIB/sysrand.o -c -O2 -fPIC -DLINUX2_1 -m64 -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Wall -DNSS_NO_GCC48 -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -UDEBUG -DNDEBUG -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_64 -DFREEBL_NO_DEPEND -DFREEBL_USE_PRELINK -DNSS_X86_OR_X64 -DNSS_X64 -DNSS_BEVAND_ARCFOUR -DMPI_AMD64 -DMP_ASSEMBLY_MULTIPLY -DNSS_USE_COMBA -DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN -DUSE_HW_AES -DINTEL_GCM -DMP_API_COMPATIBLE -I/usr/include/nspr4 -I/usr/include/nss3 -I/usr/include/nspr4 -I../../../dist/Linux4.3_x86_64_cc_glibc_PTH_64_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -Impi -Iecl -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic sysrand.c
In file included from sysrand.c:16:0:
unix_rand.c: In function 'ReadOneFile':
unix_rand.c:1091:6: warning: 'readdir_r' is deprecated [-Wdeprecated-declarations]
      error = readdir_r(fd, &entry_dir, &result);
      ^~~~~
In file included from unix_rand.c:1033:0,
                 from sysrand.c:16:
/usr/include/dirent.h:183:12: note: declared here
 extern int readdir_r (DIR *__restrict __dirp,
            ^~~~~~~~~

This time it was a warning not treated as an error. These options
-Wall -Werror

are no longer used by default.

No need for concern at the present time. It's merely something we should watch out for in the future.

Comment 1

[Florian Weimer]

Looks like the -Werror change was something on the NSS end:

changeset: 11970:d8064837a36f
user: Martin Thomson <[e-mail]>
date: Wed Mar 09 11:55:05 2016 +0100
summary: Bug 1254406 - Remove excess check against CC_NAME, r=ttaubert

Even if it compiles, the fact that readdir_r is quite broken on most systems still remains, so removing it makes sense.  No additional synchronization is needed because the directory handle is opened and closed locally, without sharing it with other threads.

But you really should not seed the PRNG from something on the local file system.  This is just asking for trouble.  Why are you even compiling this on systems which have support for /dev/urandom?  I think that's the larger problem here.

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8727649 [details] [diff] [review]
unix_rand-readdir_r-deprecated.patch

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

::: lib/freebl/unix_rand.c
@@ -1087,5 @@
>  
>      for (i=0; i <= fileToRead; i++) {
>  	struct dirent *result = NULL;
>  	do {
> -	    error = readdir_r(fd, &entry_dir, &result);

I don't know readdir() from experience - I only read the manpage - but this looks insufficient.

The code below reads and writes to `entry`, which is now uninitialized.  I would expect that you would have to change `firstEntry` to a pointer and assign it to `result`.

You can probably remove the solaris-specific hack when removing `entry`.

Comment 3

[Martin Thomson [:mt:]]

Florian, this function isn't called if /dev/urandom can be used.

   1146     file = fopen("/dev/urandom", "r");
   1147     if (file == NULL) {
   1148         return rng_systemFromNoise(dest, maxLen);
   1149     }

That is the only entry point that leads to readdir[_r] from what I can see.

Comment 4

[Florian Weimer]

(In reply to Martin Thomson [:mt:] from comment #3)
> Florian, this function isn't called if /dev/urandom can be used.
> 
>    1146     file = fopen("/dev/urandom", "r");
>    1147     if (file == NULL) {
>    1148         return rng_systemFromNoise(dest, maxLen);
>    1149     }
> 
> That is the only entry point that leads to readdir[_r] from what I can see.

I still think it's better to remove the fallback code.  What is the intention here?  Support running in a chroot which does not have /dev/urandom?

Comment 5

[Martin Thomson [:mt:]]

> I still think it's better to remove the fallback code.  What is the intention here?  Support running in a chroot which does not have /dev/urandom?

I can't answer that question.  I can only speculate that it is to support ancient systems that don't have /dev/urandom.  I'd be happier failing to init in the chroot case.

Comment 6

[Elio Maldonado]

(In reply to Martin Thomson [:mt:] from comment #2)
> Comment on attachment 8727649 [details] [diff] [review]
> unix_rand-readdir_r-deprecated.patch
> 
> Review of attachment 8727649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/freebl/unix_rand.c
> @@ -1087,5 @@
> >  
> >      for (i=0; i <= fileToRead; i++) {
> >  	struct dirent *result = NULL;
> >  	do {
> > -	    error = readdir_r(fd, &entry_dir, &result);
> 
> I don't know readdir() from experience - I only read the manpage - but this
> looks insufficient.

I'm in the same bot and ...
> 
> The code below reads and writes to `entry`, which is now uninitialized.  I
> would expect that you would have to change `firstEntry` to a pointer and
> assign it to `result`.

You are absolutely right.

> 
> You can probably remove the solaris-specific hack when removing `entry`.

Let's check first with other folks, we may be still playing nice with Oracle that still has to support some servers from Sun.

As for Florian's feedback I want to consult with Bob first as he's the one who did the FIPS-140 validation work downstream for RHEL-7 that we now have under review in Bug 1181814. I do not see unix-rand.c being touched in that other patch.

New version of the patch to address most of your review comments coming next.

Comment 7

[Elio Maldonado]

Attachment: Relace usage of readir_r with readdir

Addresses most, thought not all, of Martin's review comments.

Comment 8

[Martin Thomson [:mt:]]

Comment on attachment 8728666 [details] [diff] [review]
Relace usage of readir_r with readdir

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

::: lib/freebl/unix_rand.c
@@ +1064,5 @@
>      typedef union {
>  	unsigned char space[sizeof(struct dirent) + MAXNAMELEN];
>  	struct dirent dir;
>      } dirent_hack;
>      dirent_hack entry, firstEntry;

I think that you can remove this entire block and change `entry` to a pointer; the hack seems to be as a result of using readdir_r and needing to allocate space.

If you don't, then firstEntry needs a star here too.

@@ +1068,5 @@
>      dirent_hack entry, firstEntry;
>  
>  #define entry_dir entry.dir
>  #else
> +    struct dirent entry, *firstEntry = NULL;

Please don't declare two different types on the same line.

@@ +1087,5 @@
>  
>      for (i=0; i <= fileToRead; i++) {
>  	struct dirent *result = NULL;
>  	do {
> +	    firstEntry = result = readdir(fd);

You shouldn't assign firstEntry here; it won't be the first entry on the second loop around.

@@ +1095,4 @@
>  	    resetCount = 1; /* read to the end, start again at the beginning */
>  	    if (i != 0) {
>  		/* ran out of entries in the directory, use the first one */
> +	 	PORT_Memcpy(&entry, firstEntry, sizeof(struct dirent));

Since the use of entry is immediately below, avoid the memcpy and turn entry into a pointer.

Comment 9

[Martin Thomson [:mt:]]

Attachment: bug1254334.patch

Elio, does this look better?  I think that it's probably worth doing as Florian suggests, but that would mean taking time to work out if we support platforms without /dev/urandom and I'm not interested in spending that time.

Comment 10

[Elio Maldonado]

Comment on attachment 8728870 [details] [diff] [review]
bug1254334.patch

r+, This is much better than what I had posted.

Comment 11

[Elio Maldonado]

(In reply to Martin Thomson [:mt:] from comment #9)
> Created attachment 8728870 [details] [diff] [review]
> bug1254334.patch
> 
> Elio, does this look better?  I think that it's probably worth doing as
> Florian suggests, but that would mean taking time to work out if we support
> platforms without /dev/urandom and I'm not interested in spending that time.

I understand, I'll do the digging, it may take me a while given other high priority tasks on my plate.

Comment 12

[Martin Thomson [:mt:]]

What I recommend we do then is land this change (I think that I have a small bug to fix first though before asking you for review), then open a separate bug on which we can do the investigation.  If it turns out that we can rely on urandom alone, then we have a LOT of code to delete (which would be great).

Comment 13

[Martin Thomson [:mt:]]

Attachment: bug1254334.patch

This should work; I didn't previously copy the first entry out, and that doesn't work:

       The data returned by readdir() may be overwritten by subsequent calls to readdir()  for  the  same
       directory stream.

Comment 14

[Elio Maldonado]

Comment on attachment 8731486 [details] [diff] [review]
bug1254334.patch

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

Looks good to me but given the implications this has on the crypto work is best to have Bob review it.
I agree with your idea of splitting of the other changes that Florian suggested into a separate bug.

Comment 15

[Robert Relyea]

re removing the fallback.

most systems do have /dev/urandom, but we have ran into some cases, like containers, where we can't get to /dev/urandom. Not having a fallback can lead into improperly seeded random number generators.

Comment 16

[Ryan Sleevi]

(In reply to Robert Relyea from comment #15)
> re removing the fallback.
> 
> most systems do have /dev/urandom, but we have ran into some cases, like
> containers, where we can't get to /dev/urandom. Not having a fallback can
> lead into improperly seeded random number generators.

Bob, if that's true, then as containers, they're almost certainly misconfigured, and almost certainly leading to improper results. While it's conceivable to imagine such systems exist - and, indeed, the Chrome sandbox, for a time, in appropriately blocked access to /dev/urandom due to over-restricting the space - it should seem quite defensible to argue they're misconfigured and insecure, especially given how other cryptographic libraries (GnuTLS, OpenSSL, MatrixSSL, yaSSL) would behave in such systems - which is to fail to properly generate correct results.

While I can understand and appreciate NSS's efforts to "do the right thing," I would like to strongly argue that it's impossible for us to do so, and so to attempt to do so is a disservice to users, who might otherwise be motivated to properly configure their systems (allowing either a pass-through /dev/urandom or the actual /dev/urandom)

Comment 17

[Robert Relyea]

Comment on attachment 8731486 [details] [diff] [review]
bug1254334.patch

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

Nothing to prevent a checkin, but I would like you to use PORT_Strncpy as a safety measure.

::: lib/freebl/unix_rand.c
@@ +1080,2 @@
>  	do {
> +	    entry = readdir(fd);

My only worry here is can we be confident that this is thread safe on all platforms. As long a readdir returns space allocated out of fd, we are cool, but if readdir returns space from a static, then we are in trouble. Florian confirmed we're fine on Linux, I suspect we are fine on most platforms. We probably should add a comment. A platform that will likely fail however, is also likely not to have /dev/urandom, so it would probably show up pretty quickly.

@@ +1088,2 @@
>  	    }
>  	    /* if i== 0, there were no readable entries in the directory */

Probably don't need this comment any more since you removed the if (i!=0) test, now it looks like a non-sequitor.

@@ +1092,5 @@
> +        name = entry->d_name;
> +	if (i == 0) {
> +	    /* copy the name of the first in case we run out of entries */
> +            PORT_Assert(PORT_Strlen(name) <= NAME_MAX);
> +	    PORT_Strcpy(firstName, name);

Use PORT_Strncpy and you know you won't overflow.

Comment 18

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/7309fcbce2ec

Comment 19

[Franziskus Kiefer [:franziskus]]

This fails on Sun OS [1] with 

> "unix_rand.c", line 1064: undefined symbol: NAME_MAX

[1] https://bot.nss-crypto.org:8011/builders/1-sunos-x32-DBG/builds/1072/steps/shell/logs/stdio

Comment 20

[Martin Thomson [:mt:]]

Ugh, the variable appears on the man page that I found online, so I expected it to work.  Time to throw mud at the wall and see what sticks.  First, include the headers that the man page recommends.

https://hg.mozilla.org/projects/nss/rev/9ed2d8bec608

Comment 21

[Martin Thomson [:mt:]]

Take 2: presence of NAME_MAX on the man page obviously doesn't correspond with it being defined.  Doing the nasty thing because I need to sleep.

https://hg.mozilla.org/projects/nss/rev/1b20905cdf52

Comment 22

[Florian Weimer]

(In reply to Robert Relyea from comment #15)
> re removing the fallback.
> 
> most systems do have /dev/urandom, but we have ran into some cases, like
> containers, where we can't get to /dev/urandom. Not having a fallback can
> lead into improperly seeded random number generators.

On Linux, you should just abort if there is no /dev/urandom or it cannot be opened for any other reason.  It's an invalid execution environment.  It's highly unexpected that NSS will silently use a weaker randomness source.

Comment 23

[Franziskus Kiefer [:franziskus]]

(In reply to Florian Weimer from comment #22)
> (In reply to Robert Relyea from comment #15)
> > re removing the fallback.
> > 
> > most systems do have /dev/urandom, but we have ran into some cases, like
> > containers, where we can't get to /dev/urandom. Not having a fallback can
> > lead into improperly seeded random number generators.
> 
> On Linux, you should just abort if there is no /dev/urandom or it cannot be
> opened for any other reason.  It's an invalid execution environment.  It's
> highly unexpected that NSS will silently use a weaker randomness source.

I agree. We'll get to that soonish in bug 889116.

Comment 24

[Elio Maldonado]

See Fedora bug - nss: Failing to use /dev/urandom as source of randomness in container environment
https://bugzilla.redhat.com/show_bug.cgi?id=1329518