Oh, this is a rather compact way to mimic (I guess) libxcrypt's translation of a single cost parameter to yescrypt's multiple ones, which it'd then encode with yescrypt_encode_params_r(). I didn't verify it, but I assume the resulting strings were checked to match what libxcrypt produces? If so, that's fine.

Exactly. At first I tried to actually implement a proper parameter generation. Unfortunately, I found no documentation and very few comments in the code. I did not spent too much time on it, but I think it involves bit masks and non-standard base64 encoding. Instead of reading some foreign code for days and re-implementing it, I generated the 11 possible values using libxcrypt's crypt_gensalt.

I accept your criticism here: I should write a specification of the encoding, document the rationale behind the choices, and add some source code comments. The compact encoding of numeric parameters is actually specific to yescrypt, and needs comments.

This leading strcat() call looks unneeded as the loop contains the same (will just do one more iteration of this line is omitted).

I copy/pasted this code from gensalt_bcrypt(), which seems to be an adaptation from gensalt(). Since I do not fully understand how this code works, I didn't touched it except for the salt's length.

Makes sense to reuse the same (non-ideal) code for this PR - in fact, this is why I didn't complain about the uses of random(). I should have realized the redundant strcat() probably came from there as well.

I wouldn't increase salt size. Sure 144 bits gets encoded more optimally, but it's excessive and besides with random() we only have at most 32 bits for real:

The Owl patch I referred to above also switched to use of /dev/urandom, but of course this shouldn't be in this same PR.

BTW, it looked weird to me at first that the same code was used to generate salts for bcrypt, since bcrypt uses the crypt base64 alphabet in different order than all other crypts. This probably ended up relying on the bcrypt implementation truncating the unsupported bit values for the last character, but that's OK'ish since it's truncation from 132 to 128 no matter in which order the alphabet is.

I wouldn't increase salt size. Sure 144 bits gets encoded more optimally

Oh, maybe the reason you did that is the salt strings truncated at 22 were not being accepted, because they contained extra 4 bits in the last char, and you didn't feel like implementing encoding of exactly 128 bits?

It's not really that I don't "feel like" implementing it. My reasoning was that 144 bits does not harm in any way (libxcrypt's man 5 crypt states that the salt can be up to 512 bits) and truncating to 128 bits, while not adding value, would require me to take time study the undocumented non-standard base64 encoding with the risk of introducing bugs in my implementation. Less code, less time spent, less risks and no loss in terms of security, all the odds were in favor of a 144 bits salt.

Obviously, if you recommend to use 128 bits anyway (I'm not sure what you imply by saying it's excessive), I can do that.

the undocumented non-standard base64 encoding

For salt and hash, yescrypt uses the exact same standard crypt base64 encoding that traditional Unix crypt(3) did since the 1970s. It's also the encoding that md5crypt, sha256crypt, and sha512crypt use. (bcrypt is an outlier here.)

However, the yescrypt implementation is strict in that it only accepts salts that are exactly the same as what would be in the resulting salt+hash string. This is different from bcrypt, which silently turns the last character of salt into how it actually sees it in binary, re-encoding it in the returned salt+hash string. This is also different from md5crypt, sha256crypt, and sha512crypt, which use the salt as a mostly opaque ASCII string, preserving the full 132 bits. So it's probably because of these differences that the same code producing 132-bit salts didn't work for yescrypt here, while it did for others. Not the different base64 encoding.

Sure 144 bits does not harm, it's just different. By "excessive", I was referring to 144 vs. 128, not to 128.