Underscores in MD5 salts rejected

[mzealey]

So, on a glibc-2.17 based Centos 7 system the following happens:

$ perl -E 'say crypt($ARGV[0], $ARGV[1])' 'foo' '$1$OiaynUa_$AEUnQiyEzpgFp42qhmUlX1'  
$1$OiaynUa_$UKu.5CDrnTktg39GKrZpP0

But on ubuntu (libxcrypt 4.4.10-10ubuntu4):

$ perl -E 'say crypt($ARGV[0], $ARGV[1])' 'foo' '$1$OiaynUa_$AEUnQiyEzpgFp42qhmUlX1'  
*0

and on centos 8 (libxcrypt-devel-4.1.1-4.el8), it produces an empty string and I think an 'invalid argument' message.

If this library is meant to be backwards-compatible it should presumably emulate the behaviour of the centos 7 algo? I don't know much about the salt, but I can't see that substituting the underscroe with something else is going to replicate it correctly?

[besser82]

Hello,

and thanks for your question.

Having an underscore character _ in the salt portion violates the Modular Crypt Format (MFC) string.

Let me explain a bit:

An MFC string is formatted as ${algo_id}[[$]{rounds_or_settings}]${salt}[${hash}]. Although there is no official technical specification, it is agreed upon all characters used in such a string must be valid BASE64 encoded: [a-zA-Z0-9./].

At the time the hash implementations of glibc's libcrypt have been written, considerations have been users of the crypt() function would only pass trusted and/or validated input strings into it. Considerations and concerns about untrusted and/or invalid inputs came up a decade or so later.

Sadly the libcrypt in glibc is not that actively developed, I mean it is maintained, but does not that frequent development as other parts of glibc do. For this (and some other reasons) some people (including me) came up to revive and vastly improve libxcrypt as a (drop in) replacement.

From what I can see here: The behaviour you describe is a bug in glibc's libcrypt which has been fixed for its implementation of descrypt hash several years ago, which made that hash function accept any character.

I conclude libxcrypt has the correct behaviour of refusing any MFC string input that is not valid BASE64 encoded, while the behaviour of glibc libcrypt is faulty.

@zackw Do you agree with me?

[solardiz]

@besser82 You didn't ask me, but FWIW I would have treated this rejection of historically accepted unusual (not even invalid) md5crypt salts as a libxcrypt bug.

Like you say, "there is no official technical specification" on what characters the salt string may contain, except with hashes like bcrypt and descrypt where the salt is decoded before being used.

(I also wouldn't have changed glibc's descrypt to reject invalid salts like was done in that commit you reference. Instead, we could have enhanced libxcrypt's FreeSec to handle invalid salts in the same way that old glibc's UFC crypt handled them prior to that commit. I have a revision of FreeSec implementing that. However, now that glibc already went the other way, I am not sure it's a good idea to reintroduce that for descrypt.)

[mzealey]

I agree that the behaviour here is in terms of rejecting non-base64 characters is correct according to the spec (whether written down or generally understood), however the aim of this project is to be a drop-in replacement for glibc's libcrypt then this is not happening here. Centos 6 & 7 glibc have this behaviour so it would seem to be a fairly mainstream 'bug' - perhaps 'feature' or 'implementation edge case' would be a better description of it?

[mzealey]

Note also that these password salts were generated by older cpanel versions (they now use sha512 or so by default), however many of these hashes are still around. cpanel does not currently support centos 8 so most hosters are using centos 7. However I can imagine this bug being fairly widely seen once the cpanel centos 8 upgrade is available.

[besser82]

Let me setup a RHEL 7 VM and do some tests how sha256crypt and sha512crypt act upon unexpected characters in the salt… I will adjust libxcrypt then to behave the same way for the hash functions in question.

@solardiz Sorry for not asking you about this, too. I know you are a quiet busy person, and didn't want to bother you in the first place. Good to know I can explicitly put you in the loop for other reports like this also.
BTW, the change to descrypt in glibc's libcrypt had been make around mid. 2012 or so; long before libxcrypt was meant to be a real drop-in replacement.

[besser82]

Okay, it seems libcrypt on RHEL / CentOS 7 takes any chars (non-printable as well as printable) as a valid salt for {md5,sha256,sha512}crypt. I'm going to release a patch for libxcrypt within the next few days.

[solardiz]

No problem about not asking me - I didn't mean to complain.

Maybe it makes sense to deliberately reject $ and : and \n inside salt strings for these hashes. For $, it can simply be not supporting it rather than needing to reject it explicitly, depending on how the preceding parsing works.

[besser82]

@mzealey I hope my solution in #106 works for you.

[mzealey]

Looks great, thanks for your hard work!

[besser82]

np, you're welcome!

[rfc1036]

Did you check if all the other characters actually need to be accepted because something is really using them as salt or if _ is enough? Since ~everybody will enable this then maybe it's a good idea to be conservative and start from just allowing the characters which we know that are being used.

[btwe]

Pfew, this one lead to an erratic error during user authentication of our ldap users. The hashes were generated some time ago with another lib for >6.5k users (rehashing is no option). And some few of them have unsupported chars in their salt (+).
This issue occured after upgrading to centos8. Centos 8.3 still has the unpatched version in their repos. Could you please push them urgently to update libxcrypt in their next minor-release?

[OleHolmNielsen]

We have been bitten by the issue of user logins being rejected on CentOS 8.4 (and all other EL8 variants) as well as FC34, which was caused by the user's SHA512 password hash having one or more "+" characters in the salt. Such SHA512 hashes are generated by the university's central IT service on Windows systems, and such hashes ought to be valid according to libxcrypt's crypt(5) man-page:

Hashed passphrases are always entirely printable ASCII, and do not contain any whitespace or the characters ‘:’, ‘;’, ‘*’, ‘!’, or ‘\’.

One is lead to believe that almost any ASCII character is OK in the salt for SHA512 and other hashes, but that is unfortunately not true with the libxcrypt implementation in EL8. Quoting the comment above (June 26, 2020):

Although there is no official technical specification, it is agreed upon all characters used in such a string must be valid BASE64 encoded: [a-zA-Z0-9./].

Question: Could you please change the crypt(5) man-page so it becomes obvious that only the characters [a-zA-Z0-9./] are permitted in salts?

I note that there is in fact an official technical specification of this: The POSIX.1-2017 standard declares in
https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/crypt.html that only the following characters may be used in the salt:

The salt argument shall be a string of at least two bytes in length not including the null character chosen from the set:
a b c d e f g h i j k l m n o p q r s t u v w x y z
A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
0 1 2 3 4 5 6 7 8 9 . / 

So it seems that we are on a solid standards-based ground. We only need the documentation to be consistent with the libxcrypt implementation.

[OleHolmNielsen]

I have a trivial test code for the crypt() function on a CentOS 8.4 system:

/* compile: /usr/bin/gcc -lcrypt -o passwd-sha512 passwd-sha512.c */
/* usage: passwd-sha512 <password> <salt (16 chars max)> */

#define _XOPEN_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <crypt.h>

int main(int argc, char *argv[]) {
  if ( argc < 3 || (int) strlen(argv[2]) > 16 ) {
    printf("usage: %s password salt\n", argv[0]);
    printf("--salt must not larger than 16 characters\n");
    return 0;
  }

  char salt[21];
  sprintf(salt, "$6$%s$", argv[2]);

  printf("%s\n", crypt((char*) argv[1], (char*) salt));
  return 0;
}

When executing this code we get a core dump when non-standard characters are used in the salt:

$ ./passwd-sha512  abcd abcd
$6$abcd$bT9Tu4itxmzGnycF0Rp8CgywJpZsvlNyak/XXgqhZQlP.4o6xJdBos7vwBaUb4UL.wgj1FAWTzBSeVagy/hMA0
$ ./passwd-sha512  abcd abcd+
Segmentation fault (core dumped)

CentOS 8.4 has a libxcrypt-4.1.1-4.el8.x86_64 RPM.

Interestingly, the code does not core dump on CentOS 8 Stream:

$ ./passwd-sha512  abcd abcd+
$6$abcd+$Ey10fMRkH3D.6uK9MjTdRKxxm6TFE1EjG13.GeZ5ua0o8XByuiYia7IffPGR9g9LYado.P5fcn9gejkan2Uv./

with a slightly newer libxcrypt-4.1.1-6.el8.x86_64 RPM.

[zackw]

@OleHolmNielsen First off, I'm pretty sure your test program segfaults because crypt() is returning NULL. Can you confirm?

We have been allowing + in salts since b2b813a; I believe the oldest upstream version with this change is 4.4.17. Looking at the change log on https://centos.pkgs.org/8-stream/centos-baseos-x86_64/libxcrypt-4.1.1-6.el8.x86_64.rpm.html, I think CentOS backported that change in their 4.1.1-5. I don't know anything about how or when Stream packages propagate into numbered releases of CentOS, you'll need to take that up with them.

Let's move further discussion of what punctuation should be accepted in salts to #135.

[OleHolmNielsen]

I confirm that crypt() returns NULL on CentOS 8.4. I added code to display the error:

$ ./passwd-sha512 aaaa aaaa+
crypt returned NULL with errno 22 Invalid argument

The same binary works on CentOS 8 Stream:

$  ./passwd-sha512 aaaa aaaa+
$6$aaaa+$jBuvbAFjg5D3jgr1qnPHjC5N1LjcpfbE/fGaQqHGpQxF.Wwl3zT2GGkZrsro2Xlsv1QG0d7GSWq5C1n81fT2n1

So CentOS and RHEL 8.4 don't like "+" in the salt strings, whereas CentOS 8 Stream accepts "+" happily.