Page MenuHomeFreeBSD

crypt_r(3): fix reentrancy problems with DES
ClosedPublic

Authored by trasz on Jun 7 2021, 2:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 20 2024, 9:55 PM
Unknown Object (File)
Dec 23 2023, 11:23 AM
Unknown Object (File)
Dec 14 2023, 10:14 PM
Unknown Object (File)
Nov 7 2023, 10:45 AM
Unknown Object (File)
Oct 23 2023, 4:32 AM
Unknown Object (File)
Oct 6 2023, 9:43 AM
Unknown Object (File)
Sep 25 2023, 11:48 PM
Unknown Object (File)
Sep 18 2023, 12:44 AM
Subscribers

Details

Summary

This code was originally written for non-reentrant crypt(3).
In 5f521d7ba72, a thread-safe crypt_r(3) was introduced. However,
it looks like the DES implementation is still not re-entrant;
routines like setup_salt() or des_setkey() still use global
variables.

Instead of something drastic, eg removing DES support altogether,
just mark those variables as thread-local.

Given that this only applies to DES, I think the impact is minimal.

Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 39769
Build 36658: arc lint + arc unit

Event Timeline

trasz requested review of this revision.Jun 7 2021, 2:55 PM
markj added inline comments.
secure/lib/libcrypt/crypt-des.c
77

It looks like this and some of the other tables below should be marked const. Can we do that as a part of this change?

84

Doesn't this need to be thread-local as well?

Add const, move 'static' first, and add __thread for stuff touched
by des_init().

trasz marked 2 inline comments as done.Jun 8 2021, 1:31 PM
trasz added inline comments.
secure/lib/libcrypt/crypt-des.c
77

Good idea, done.

84

These only seem to be written to by des_init(), so they should be equal for all threads, but... yeah, you're right.

I don't see any problems with the change, but note that this adds something like 30KB of data per thread. Probably not the end of the world but not ideal either.

This revision is now accepted and ready to land.Jun 8 2021, 1:59 PM

Yeah; I think it's acceptable, given it's a separate library and not libc.

(Tinderboxed.)

This revision was automatically updated to reflect the committed changes.