Make crypt and crypt_gensalt use thread-local output buffers.

[besser82]

This change makes crypt and crypt_gensalt as thread-safe as they can be without changing their interfaces. Solaris already made this change, and it’s being discussed by glibc (with suggestion that it should be pushed upstream to the C
and POSIX standards committees): https://sourceware.org/ml/libc-alpha/2018-10/msg00437.html

Portable programs should still use the r-variants, though, because this is not a guaranteed feature that is portable to other implementations of these functions. Also it doesn’t help threads to not clobber their corresponding output buffer on a second call from within the same thread.

The way I've implemented this mimicks the implementation from Solaris.

I've also taken into account some problems with -fno-plt code and thread-local storage on PPC64-architectures. See: https://sourceware.org/PR32387

---

This also fixes #62.

[solardiz]

I didn't review the m4 stuff.

Overall, I am not sure this is worth the complexity and reduced portability, but I don't mind (once the issues I pointed out are addressed - especially what looks like a trivial yet major bug).

[solardiz]

Now this calls crypt_r with uninitialized stack data in nr_crypt_ctx - a bug? IIRC, at least the initialized field should be set to 0.

Also, we probably incur some performance penalty by having this struct recreated each time, but perhaps only for descrypt?

[besser82]

Now this calls crypt_r with uninitialized stack data in nr_crypt_ctx - a bug? IIRC, at least the initialized field should be set to 0.

Fixed by initializing the whole struct with explicit_bzero.

Also, we probably incur some performance penalty by having this struct recreated each time, but perhaps only for descrypt?

Well, the performance penalty is meassurable, but not noticeable: This change adds on average about 220 milliseconds of computing time for 1 Million invocations; so in a range of 200 - 300 nanoseconds per call. As those numbers are measured on an Intel Celeron 4205U with Fedora Workstation 41, the real life time penalty on affordable faster hardware, like a Ryzen 5 2400G, are expected be lower.

[solardiz]

Fixed by initializing the whole struct with explicit_bzero.

Feels like mild misuse of explicit_bzero, but that's not necessarily bad. If we want to use it not only to initialize, but also to wipe any sensitive data that might have been left from a previous call, then perhaps we also want to do it for the output buffer before the strcpy_or_abort. And then maybe move the explicit_bzero of the data struct to after the strcpy_or_abort and introduce simple initialization (just memset)?

[besser82]

strcpy_or_abort already zeroizes the left over space after the copied string.

The cleanup of struct crypt_data is also already done by do_crypt (called from inside crypt_r) after the computation has been finished.

I've used explicit_bzero here intentionally to ensure the just stack-allocated struct crypt_data is really initialized properly before use.

[besser82]

Feels like mild misuse of explicit_bzero, but that's not necessarily bad.

Changed to memset for initialization.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.16%. Comparing base (ca15167) to head (cfc9d92).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #201      +/-   ##
===========================================
+ Coverage    90.15%   90.16%   +0.01%     
===========================================
  Files           32       32              
  Lines         3626     3631       +5     
  Branches       689      690       +1     
===========================================
+ Hits          3269     3274       +5     
  Misses         226      226              
  Partials       131      131              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zackw]

@besser82 I should have time to look at this early next week, but not before that.

[zackw]

I approve of the goal here. It's something I wanted to do myself quite some time ago, in fact, but I thought there was no way to do it without making libcrypt.so have a hard dependency on libpthread.so (which is a thing we almost certainly cannot do, for compatibility reasons). You found a way to avoid that dependency, which is great...

... except that I think the way you did do it is also going to be a compatibility problem. You're using CRYPT_OUTPUT_SIZE + CRYPT_GENSALT_OUTPUT_SIZE == 576 bytes of thread-local storage. If I'm reading glibc's elf/dl-tls.c correctly, a bunch of internal data structures, and the official minimum size for thread stacks, are sized on the assumption that any given shared library is going to want only a couple of pointers' worth of TLS space. What this means is, processes that load libcrypt.so are going to be pushed into a slow, poorly tested, fallback code path within the dynamic linker, and any process that loads libcrypt.so and also uses small thread stacks may start crashing, whether or not they use crypt or crypt_gensalt. On top of which, if they do call crypt, this change makes that require 32kB more stack than it used to.

I also have concerns about all the sketchy autoconf-archive code this patch imports. Just on a quick skim I saw several constructs that are asking for trouble with autoconf 2.70 and later.

---

So how can we do this?

I think we shouldn't use TLS at all (which conveniently gets rid of two-thirds of the questionable autoconf macros). I think we should use pthread_getspecific to acquire one thread-local pointer for each function, and allocate the bulk of the data with malloc (which also lets us go back to reusing struct crypt_data across invocations).

Unfortunately, this brings back the problem of a dependency on libpthread. For GNU libc, libpthread was merged into libc.so.6 in glibc 2.34, and as I understand it, musl libc has always shipped a single integrated shared object for the whole C library. But we probably still need to care about older glibc, and maybe also about non-Linux-based systems. I think what we need to do about this is split libcrypt into four pieces that are tied back together with a GROUP linker script (for an example of what I mean by GROUP linker script, look at the contents of the file glibc installs as libc.so (not libc.so.6)):

• libcrypt.so.3 defines only crypt_checksalt, crypt_gensalt_ra, crypt_gensalt_rn, crypt_preferred_method, crypt_ra, and crypt_rn, with their XCRYPT_x.y symbol versions.
• libcrypt_legacy_nonreentrant.so.1 defines only crypt and crypt_gensalt, using pthread_getspecific-based thread-local pointers. It has a dependency on libpthread if necessary. We install a symlink to this file under the name libcrypt.so.2.
• libcrypt_legacy_des.so.1 defines encrypt, encrypt_r, setkey, and setkey_r. (I propose that we do not bother making encrypt and setkey use thread-local storage.)
• libcrypt.so.1 defines all the compatibility symbols.

Thoughts?

[solardiz]

I approve of the goal here. It's something I wanted to do myself quite some time ago

If implementations look so risky and/or complicated to you, then I'm puzzled that you think the benefits outweigh the risks and effort and complexity. Maybe just don't do this?

I think we should use pthread_getspecific to acquire one thread-local pointer for each function, and allocate the bulk of the data with malloc

This sounds somewhat similar to what I saw Solaris do, #62 (comment)

[rfc1036]

Why care about old glibc versions? It's not like people normally build libxcrypt to replace the one shipped by their distribution.

[zackw]

@rfc1036 The entire point of this library is to be a drop-in replacement for old glibc's libcrypt. If it didn't need to be that, it would have a completely different API.

[rfc1036]

Of course, but reasonably nobody is replacing glibc's libcrypt on existing systems!

[zackw]

Not true, people do it all the time, just look through the outstanding bug reports.