openwall / tcb

Alternative password shadowing scheme
https://www.openwall.com/tcb/
Other
8 stars 3 forks source link

libnss_tcb: Use readdir(3) with glibc >= 2.24. #1

Closed besser82 closed 3 years ago

besser82 commented 3 years ago

It is recommended that applications use readdir(3) instead of readdir_r(3). Furthermore, since version 2.24, glibc deprecates readdir_r(3).

Also use thread local storage for the underlying directory stream in this case.

besser82 commented 3 years ago

@ldv-alt Thanks for looking into this!

solardiz commented 3 years ago

FWIW, we had discussed this issue in the thread ending here: https://www.openwall.com/lists/owl-dev/2020/07/16/3 - I think it's a good idea to re-read it now.

besser82 commented 3 years ago

@solardiz: From https://www.gnu.org/software/libc/manual/html_node/NSS-Module-Names.html:

In fact the NSS modules only contain reentrant versions of the lookup functions.
I.e., if the user would call the gethostbyname_r function this also would end in
the above function.  For all user interface functions the C library maps this call
to a call to the reentrant function.  For reentrant functions this is trivial since the
interface is (nearly) the same.  For the non-reentrant version the library keeps
internal buffers which are used to replace the user supplied buffer.

And from getspent(3):

Analogous to the reentrant functions for the password database, glibc also has
reentrant functions for the shadow password database.  The getspnam_r()
function is like getspnam() but stores the retrieved shadow password structure
in the space pointed to by spbuf.  This shadow password structure contains
pointers to strings, and these strings are stored in the buffer buf of size buflen.
A pointer to the result (in case of success) or NULL (in case no entry was found
or an error occurred) is stored in *spbufp.

The functions getspent_r(), fgetspent_r(), and sgetspent_r() are similarly
analogous to their nonreentrant counterparts.

So to me it looks like the 'reentrant' functions are meant to only provide a caller-provided return value buffer, so if different buffers are used then the previous ones are not overwritten by subsequent calls. Not that the functions neccessarily are really reentrant.

Anyways there shouldn't be any problem with concurrent access, as with thread-local storage each thread operates on its own directory stream of tcbdir.

ldv-alt commented 3 years ago

Just for the record, readdir(3) says the following:

In the current POSIX.1 specification (POSIX.1-2008), readdir() is not required
to be thread-safe.  However, in modern implementations (including the glibc
implementation), concurrent calls to readdir() that specify different directory
streams are thread-safe.  In cases where multiple threads must read from the
same directory stream, using readdir() with external synchronization is still
preferable to the use of the deprecated readdir_r(3) function.

The use of thread-local storage for tcbdir should make the whole thing MT-safe.

solardiz commented 3 years ago

@besser82 Feel free to fix fedoraporject.org when you update this one, or do that separately.

besser82 commented 3 years ago

@besser82 Feel free to fix fedoraporject.org when you update this one, or do that separately.

Done!

besser82 commented 3 years ago

@solardiz Thank you for merging!