Re: crypt_r()?
Discussion about a reasonable API is ongoing, but I'll give some
review comments anyway -- including some of my thoughts on why the
crypt_r API is not ideal -- in case they're helpful.
--- include/unistd.h 15 Oct 2021 22:32:28 - 1.162
+++ include/unistd.h 12 Feb 2022 12:47:04 -
@@ -325,8 +325,15 @@
* Implementation-defined extensions
*/
#if defined(_NETBSD_SOURCE)
+struct crypt_data {
+int initialized; /* glibc compat */
+char __buf[512]; /* buffer returned by crypt_r */
+};
I'd avoid putting this in unistd.h -- maybe a public header
file instead if it's to match the API documented in
.
Using _NETBSD_SOURCE _or_ _GNU_SOURCE, for APIs compatible with glibc,
has precedent -- e.g., for feenableexcept/fedisableexcept/fegetexcept.
Where does 512 come from? This should be documented in a comment.
If the result is to be statically allocated -- and I'm not opining on
that, just taking it as a premise -- it needs to be large enough for
all of the password hashes in tree. 512 bytes is almost certainly
enough, but this magic number should be documented somewhere, and
verified with __CTASSERT for all of the password hashes involved.
Quick back-of-the-envelope estimate: with a 32-byte salt and a 32-byte
hash (which is the largest that there is any cryptographic motivation
for, even in the face of quantum computers), base64-encoded into up to
43 bytes each, that leaves 426 bytes for parameters and other overhead
like the `$argon2' tag, which seems like a lot more than the caller
has to reserve stack space for.
So maybe, say, 188 bytes instead of 512 bytes would be enough (still
leaves over 100 bytes for overhead); then the whole struct fits in 192
bytes or three cache lines on many CPUs. I'm not saying 188 is the
right choice, just giving an example with reasoning -- maybe 512 is
better but it needs a rationale. Obviously we want to overestimate
this to future-proof the ABI, but this also shouldn't be unreasonably
burdensome on the caller's stack space which is often much more
limited than heap space.
And, of course, this is a reason to prefer heap allocation, even if
that means slightly more API complexity (caller must free the result):
we just don't need to think about burden on the caller's stack usage.
On the other hand, there is some value to just being able to drop this
API in and have existing code work. Certainly the glibc approach of
putting >128 KB in struct crypt_data is, uhhh, not great! No modern
justification for doing that -- either it's too little for modern
password hashes, or it's a waste of stack space because they'll
allocate storage separately on the heap anyway.
--- lib/libcrypt/bcrypt.c 16 Oct 2021 10:53:33 - 1.22
+++ lib/libcrypt/bcrypt.c 12 Feb 2022 12:47:04 -
@@ -74,9 +74,9 @@
static void encode_base64(u_int8_t *, u_int8_t *, u_int16_t);
static void decode_base64(u_int8_t *, u_int16_t, const u_int8_t *);
-crypt_private char *__bcrypt(const char *, const char *); /* XXX */
+/* crypt_private char *__bcrypt(const char *, const char *);*/ /* XXX */
-static charencrypted[_PASSWORD_LEN];
+/* static charencrypted[_PASSWORD_LEN]; */
Just delete unused things like this, instead of commenting them out,
and __CTASSERT sizeof((struct crypt_data *)0) >= _PASSWORD_LEN.
(or maybe struct crypt_data::__buf should just be _PASSWORD_LEN bytes
long)
--- lib/libcrypt/crypt-argon2.c 22 Nov 2021 14:30:24 - 1.15
+++ lib/libcrypt/crypt-argon2.c 12 Feb 2022 12:47:04 -
@@ -379,10 +379,10 @@
/* argon2 pwd buffer */
char pwdbuf[128];
/* returned static buffer */
- static char rbuf[512];
+ /* static char rbuf[512]; */
delete, __CTASSERT
This should also be a __CTASSERT to confirm that the size in
crypt_data is enough -- with a name or reference for where 512 came
from, like the Argon2id paper or API documentation:
__CTASSERT(sizeof(data->__buf) >= ARGON2_MAX_HASH);
--- lib/libcrypt/crypt-sha1.c 29 Oct 2021 13:22:08 - 1.10
+++ lib/libcrypt/crypt-sha1.c 12 Feb 2022 12:47:04 -
@@ -107,12 +107,12 @@
* hmac key.
*/
crypt_private char *
-__crypt_sha1 (const char *pw, const char *salt)
+__crypt_sha1 (const char *pw, const char *salt, struct crypt_data *data)
{
static const char *magic = SHA1_MAGIC;
static unsigned char hmac_buf[SHA1_SIZE];
-static char passwd[(2 * sizeof(SHA1_MAGIC)) +
- CRYPT_SHA1_SALT_LENGTH + SHA1_SIZE];
+/* static char passwd[(2 * sizeof(SHA1_MAGIC)) +
+ CRYPT_SHA1_SALT_LENGTH + SHA1_SIZE]; */
Same deal with __CTASSERT here.
--- lib/libcrypt/crypt.c22 Feb 2020 10:29:17 - 1.38
+++ lib/libcrypt/crypt.c12 Feb 2022 12:47:04 -
@@ -466,7 +466,8 @@
static C_block
Re: crypt_r()?
> Date: Wed, 16 Feb 2022 10:27:08 -0500 (EST) > From: Mouse > > > Thi is an essential hardening step against FPGA/custom ASIC > > implementations. > > I can't help feeling that there should be better ways to do that. I > like the idea of resistance to such things, but, for at least my > purposes, the ability to check passwords without major resource > consumption is a routine desire; resistance against an attacker willing > to invest in custom hardware is not. Then for your purposes, you can set default parameters in /etc/passwd.conf that are bounded according to the resources of the least capable machine in your environment. But a _program_ that is supposed to work with any /etc/master.passwd has to be able to handle the parameters set there, so it's not sensible to ask the caller to preallocate enough storage for any password hashing computation since there is, a priori, no static upper bound on how much storage that might be (not to mention it might also need to spawn threads for parallelism).
Re: crypt_r()?
On Wed, Feb 16, 2022 at 10:27:08AM -0500, Mouse wrote: > That sounds like a recipe for disaster. It is a complete fail for > heterogenous environments where the same hash needs to be checkable on > widely disparate hardware, where a small machine may not have the > resources to perform the check _at all_. It is managable and configurable. You can configure your passwd.master to not use argon2 at all, or you can use the settings on one of the smaller machines and copy that over to the others (or only ever add new users on that machine and copy the hash over). We tested the NetBSD passwd implementation using argon2 on a SparcStation LX and it auto-configured itself fine. I should have tested on the VAX and the mac68k too, but updating those was inconveninent at the time nia did the changes - but I'm sure both will work fine too. Martin
Re: crypt_r()?
> Given that malloc will cache any reasonable small allocation anyway, > we are talking about a few dozen or 100 cpu cycles for an operation > that is expected to take several orders of magnitude more. I wouldn't be concerned about CPU time costs, no. I'm more concerned about malloc _failing_. Given what you said in another message about password hashing being a deliberate(!) memory hog, it probably doesn't matter much in practice, in most cases. But I would still dislike an API that requires it; replacing a hashing algorithm for non-"most" cases is a lot easier than replacing the API. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: crypt_r()?
> All modern password hashing algorithms use a large memory buffers and > will attempt to scale them according to system ressources. So merely attempting to log in *requires* a major fraction of the amount of memory the system had when the password was set? That sounds like a recipe for disaster. It is a complete fail for heterogenous environments where the same hash needs to be checkable on widely disparate hardware, where a small machine may not have the resources to perform the check _at all_. It is also a total fail when trying to log in to a machine to deal with severe memory pressure. (Perhaps fortunately, password-hash checking does not apply to ssh, or at least doesn't have to.) > This applies at least to ARGON2 (which we include) and scrypt. Then perhaps it's just as well I haven't been tracking -current, because I would consider that sort of resource grabbing a "must be fixed before this is even minimally usable" bug. > Thi is an essential hardening step against FPGA/custom ASIC > implementations. I can't help feeling that there should be better ways to do that. I like the idea of resistance to such things, but, for at least my purposes, the ability to check passwords without major resource consumption is a routine desire; resistance against an attacker willing to invest in custom hardware is not. Personally, my lines of defense would be (a) keeping the hashes secret and (b) using good passwords. I would also suggest using a private hashing algorithm, but that depends on having the expertise to pick a unique-enough good-enough algorithm and implement it, leading me to say it is probably suitable only for particularly high-value targets or hardcore geeks like me. I'd guess that probably a majority of the people on this list are competent to add an algorithm, though perhaps not to choose/design one - though I would also guess we are very much a minority among NetBSD users in that respect. Mouse
Re: crypt_r()?
Am Tue, Feb 15, 2022 at 05:25:10PM -0800 schrieb Konrad Schroder: > Of course the clearest use-case for crypt_r(3) is a password cracker: each > thread sets up its own local memory and blasts through calls to crypt_r(3) > as fast as it can. I've run a cracker as a white-hat. But I can see not > wanting to add the capability to base if that's the only convincing use > case. If you are using a general CPU to do password cracking, you don't care about performance or efficiency anyway. It's a micro-optimisation for a use case that is not that relevant anyway. Given that malloc will cache any reasonable small allocation anyway, we are talking about a few dozen or 100 cpu cycles for an operation that is expected to take several orders of magnitude more. It's not even background noise. Joerg
Re: crypt_r()?
Am Tue, Feb 15, 2022 at 08:04:29PM -0500 schrieb Mouse: > > Given that the goal of the crypt(3) interface is to be slow, > > optimizing a memory allocation away saves nothing, except making a > > more complicated interface. > > I disagree. It saves a bunch of error paths. Many (most? all?) of the > password hashing algorithms run, or can be made to run, without needing > any heap memory at all, if you use a caller-provided buffer for the > returned string. I really don't like making them depend on malloc, > though I have a hard time articulating what bothers me about it. All modern password hashing algorithms use a large memory buffers and will attempt to scale them according to system ressources. This applies at least to ARGON2 (which we include) and scrypt. Thi is an essential hardening step against FPGA/custom ASIC implementations. Even ignoring that, the application has to deal with errors at least from general password hashing function anyway, so there is no extra complexity in the application. Joerg
Re: crypt_r()?
Am Tue, Feb 15, 2022 at 03:58:30PM -0800 schrieb Jason Thorpe: > There really should be a function that takes a user name or ID and a > cleartext password string, and IPCs to another (trusted system) process > to do the verification, so that programs that want to verify passwords > don’t need root privileges. Same for changing passwords. For the verification half of that, see security/pam-pwuath_suid. Joerg
Re: crypt_r()?
> On Feb 15, 2022, at 5:13 PM, Mouse wrote: > >> There really should be a function that takes a user name or ID and a clearte$ > > Maybe. But then you have a lot more failure modes and a lot more > possible attack surface. It would also mean that you can't check or > change passwords in single-user mode without starting the magic daemon; > that would be a substantial regression as far as user experience goes, > if nothing else. And what about checking the root password for > single-user boot with insecure console? You put the fallback logic into the function libc that can satisfy the request using the Old Way if the helper isn’t available. Obviously, to satisfy it the Old Way, the process would need to have root privileges, but this would be OK in the scenario you’re describing. > It _is_, however, very much in keeping with the "encapsulate > single-purpose code into a single place" attitude that has brought a > lot of benefits. I wonder if there isn't some better way I'm missing. It’s certainly a lot better than having a big complex program (that exposes itself to the network, potentially) require root privileges just to check passwords. -- thorpej
Re: crypt_r()?
On 2/15/2022 5:04 PM, Mouse wrote: (2) Hashing password, which takes the password and the settings and returns an allocated string with the resulting hash. [...] I really don't like making them depend on malloc, though I have a hard time articulating what bothers me about it. I can't say what bothers *you* about it :^) but what bothers me is that it makes it impossible to pre-allocate memory for the purpose. This might be a concern, for example, on a system with no swap. Combine that with a need for thread safety, say for a threaded login daemon on a system with no swap, and you get crypt_r(3) as described. Of course the clearest use-case for crypt_r(3) is a password cracker: each thread sets up its own local memory and blasts through calls to crypt_r(3) as fast as it can. I've run a cracker as a white-hat. But I can see not wanting to add the capability to base if that's the only convincing use case. Take care, Konrad Schroder perse...@.org
Re: crypt_r()?
> There really should be a function that takes a user name or ID and a clearte$ Maybe. But then you have a lot more failure modes and a lot more possible attack surface. It would also mean that you can't check or change passwords in single-user mode without starting the magic daemon; that would be a substantial regression as far as user experience goes, if nothing else. And what about checking the root password for single-user boot with insecure console? It _is_, however, very much in keeping with the "encapsulate single-purpose code into a single place" attitude that has brought a lot of benefits. I wonder if there isn't some better way I'm missing. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: crypt_r()?
> There are two sensible interface contracts here: > (1) Verification only, which takes the password and the expected hash > and returns a bool. [...] > (2) Hashing password, which takes the password and the settings and > returns an allocated string with the resulting hash. [...] Well, I disagree about the "allocated" part; I think there is a place for returning into a buffer specified by the caller. But, more what's leading me to write now: if you go with (1), you still need something like (2) in order to _set_ passwords. (2) - or something like (2) into a caller-specified string - can be used for either checking or setting. (1) can't. > Given that the goal of the crypt(3) interface is to be slow, > optimizing a memory allocation away saves nothing, except making a > more complicated interface. I disagree. It saves a bunch of error paths. Many (most? all?) of the password hashing algorithms run, or can be made to run, without needing any heap memory at all, if you use a caller-provided buffer for the returned string. I really don't like making them depend on malloc, though I have a hard time articulating what bothers me about it. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: crypt_r()?
> On Feb 15, 2022, at 3:30 PM, Joerg Sonnenberger wrote: > > Am Wed, Feb 16, 2022 at 12:04:16AM +0100 schrieb Niclas Rosenvik: >> do you mean that the interface should be >> crypt_r(const char *key, const char setting, char * storage, size_t >> *storage_len) >> where storage can be set to NULL to return the needed storage size in >> storage_len? > > No. There are two sensible interface contracts here: > > (1) Verification only, which takes the password and the expected hash > and returns a bool. No separate settings necessary as the hash already > contains all the necessary parameters. > > (2) Hashing password, which takes the password and the settings and > returns an allocated string with the resulting hash. This is essentially > the same interface as crypt(3), but the caller is responsible for > free(3) the return value. Given that the goal of the crypt(3) interface > is to be slow, optimizing a memory allocation away saves nothing, except > making a more complicated interface. While we’re on the topic of “what we’d like the interfaces to do”… There really should be a function that takes a user name or ID and a cleartext password string, and IPCs to another (trusted system) process to do the verification, so that programs that want to verify passwords don’t need root privileges. Same for changing passwords. Then there’s no need for any of this crypt(3) tomfoolery. -- thorpej
Re: crypt_r()?
Am Wed, Feb 16, 2022 at 12:04:16AM +0100 schrieb Niclas Rosenvik: > do you mean that the interface should be > crypt_r(const char *key, const char setting, char * storage, size_t > *storage_len) > where storage can be set to NULL to return the needed storage size in > storage_len? No. There are two sensible interface contracts here: (1) Verification only, which takes the password and the expected hash and returns a bool. No separate settings necessary as the hash already contains all the necessary parameters. (2) Hashing password, which takes the password and the settings and returns an allocated string with the resulting hash. This is essentially the same interface as crypt(3), but the caller is responsible for free(3) the return value. Given that the goal of the crypt(3) interface is to be slow, optimizing a memory allocation away saves nothing, except making a more complicated interface. Joerg
Re: crypt_r()?
On Sat, 12 Feb 2022 23:32:31 +0100 Joerg Sonnenberger wrote: > Am Sat, Feb 12, 2022 at 05:25:11PM +0100 schrieb Niclas Rosenvik: > > On Mon, 7 Feb 2022 16:12:17 +0100 > > Thomas Klausner wrote: > > > > > Hi! > > > > > > I've been asked by the filezilla software developer if NetBSD > > > will add crypt_r() as a thread-safe crypt() replacement. > > > > > > Is anyone interested in working on this? > > > Thomas > > > > Here is a cvs diff that implements crypt_r, as mouse pointed out > > it is really trivial since __crypt is already essentially crypt_r. > > Please don't commit a new interface when my initial question has not > been answered. What is this interface supposed to solve? This is > essential as a password verification interface very naturally can > internalize all storage necessary for the different backends where as > a contract that involves returning a "decrypted" string can not. For > the same reason, the interface contract here is completely wrong. > There is no need to expose the internals like this. The only > non-thread-safe space used by crypt(3) that should ever be exposed is > the space necessary for the return storage. > > Joerg Nothing is being committed before review and discussion and maybe not committed all since there seems to be resistance. The first thing crypt_r solves is that the function and its interface is used by programs in pkgsrc and they need patching or will be missing functionality. The static storage that you mention is considered a bug according to NetBSD:s crypt man page. crypt_r solves this bug. You claim that the interface is completely wrong. When you write "expose the internals", do you mean the initialized variable in struct crypt_data? It is unused in this implementation and is there for compatibility with glibc. Programs written for glibc are to set initialized to 0 before calling crypt_r the first time and by including it in crypt_data compilation of these programs will not break because they set initialized. __buf is the space needed for return storage or do you mean that the interface should be crypt_r(const char *key, const char setting, char * storage, size_t *storage_len) where storage can be set to NULL to return the needed storage size in storage_len? I see a point with your claim about the static buffer, if a new algorithm is introduced that needs more than 512 bytes returned from crypt and crypt_r then the length of __buf will have to be updated for it to work correctly so the change won't be internal to the crypt library. Niclas
Re: crypt_r()?
Am Sat, Feb 12, 2022 at 05:25:11PM +0100 schrieb Niclas Rosenvik: > On Mon, 7 Feb 2022 16:12:17 +0100 > Thomas Klausner wrote: > > > Hi! > > > > I've been asked by the filezilla software developer if NetBSD will add > > crypt_r() as a thread-safe crypt() replacement. > > > > Is anyone interested in working on this? > > Thomas > > Here is a cvs diff that implements crypt_r, as mouse pointed out > it is really trivial since __crypt is already essentially crypt_r. Please don't commit a new interface when my initial question has not been answered. What is this interface supposed to solve? This is essential as a password verification interface very naturally can internalize all storage necessary for the different backends where as a contract that involves returning a "decrypted" string can not. For the same reason, the interface contract here is completely wrong. There is no need to expose the internals like this. The only non-thread-safe space used by crypt(3) that should ever be exposed is the space necessary for the return storage. Joerg
Re: crypt_r()?
On Mon, 7 Feb 2022 16:12:17 +0100 Thomas Klausner wrote: > Hi! > > I've been asked by the filezilla software developer if NetBSD will add > crypt_r() as a thread-safe crypt() replacement. > > Is anyone interested in working on this? > Thomas Here is a cvs diff that implements crypt_r, as mouse pointed out it is really trivial since __crypt is already essentially crypt_r. I have tested this and it gives the same return strings as the old crypt. Some questions that I have. Where is the best place to define struct crypt_data? should it really be in unistd.h like in FreeBSD or is it better to have it in sys/types.h or some more fitting header. I wonder since our unistd.h has no data definitions just declarations. should crypt_r be under just _NETBSD_SOURCE or _NETBSD_SOURCE || _GNU_SOURCE since it is a GNU extension? __brypt has a declaration in brypt.c and in crypt.h, wonder why it is in brypt.c since brypt.c includes crypt.h? I would like nia, jhigh, christos, riastradh and others to look at the diff to see that it is okay and correct. Niclas crypt_r-diff Description: Binary data
Re: crypt_r()?
>> I've been asked by the filezilla software developer if NetBSD will >> add crypt_r() as a thread-safe crypt() replacement. > I don't exactly see the point. Thread safety, presumably. crypt(3) returns a pointer into a static buffer. I daresay it could be made to return a pointer into TLS when running threaded, but I think crypt_r makes more sense. I even have, on some small number of occasions, wanted a crypt_r without threads (though without threads or at least signals it's just a strcpy or strdup away). > If I wanted to provide a more modern interface, it would be for > password verification only and include constant time guarantees, > which the existing interface can't provide. Nobody's stopping you. For many purposes, crypt_r has the major advantage over what you describe that it is trivial changes to existing code. (In most cases, I suspect using what you're thinking of would be almost as trivial. But without a design to look at it's hard to tell. Admittedly, crypt_r _could_ mean pretty much anything, but it strikes me as unlikely wiz would use that name without meaning exactly what I daresay everybody here takes it to mean.) /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: crypt_r()?
Am Mon, Feb 07, 2022 at 04:12:17PM +0100 schrieb Thomas Klausner: > I've been asked by the filezilla software developer if NetBSD will add > crypt_r() as a thread-safe crypt() replacement. I don't exactly see the point. If I wanted to provide a more modern interface, it would be for password verification only and include constant time guarantees, which the existing interface can't provide. Joerg
