[musl] [PATCH 0/1] crypt(3) returns string literal "*", crashing PAM on Alpine

[musl] [PATCH 0/1] crypt(3) returns string literal "*", crashing PAM on Alpine

[Runxi Yu]

Hi,

Test_User <hax@runxiyu.org> discovered that using passwd(1) from
shadow-utils on Alpine Linux 3.21 with a password longer than 256
character causes a segmentation fault.  I reported it to
<https://gitlab.alpinelinux.org/alpine/aports/-/issues/16784>, and
people in #alpine-linux helped a bit.

It turns out that musl's crypt(3) returns a "*" from a string literal,
which PAM attempts to erase with pam_overwrite_string.

(Irrelevant lines truncated)

musl/src/crypt/crypt_sha512.c
> 	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash))
> 		return "*";

pam/modules/pass_unix/passverify.c
> 	sp = crypt(password, salt);
> 	if (!sp || strncmp(algoid, sp, strlen(algoid)) != 0) {
> 		pam_syslog(...);
> 		if(sp) {
> 		   pam_overwrite_string(sp);  /* <-- attempt to overwrite musl's string literal */
> 		}
> 		return NULL;
> 	}

https://pubs.opengroup.org/onlinepubs/9799919799/functions/crypt.html:
> The return value of crypt() points to static data that is overwritten
> by each call.
> 
> Upon successful completion, crypt() shall return a pointer to the
> hashed password; the first two bytes of the returned value shall be
> those of the salt argument. Otherwise, it shall return a null pointer
> and set errno to indicate the error.

I think musl's behavior of returning "*" is incorrect. It's rather
reasonable for the caller to attempt to erase whatever is returned by
crypt, and AFAIK there is no specification that allows returning "*" as
a failure return value.


Note: I am not on the mailing list; please CC me in replies.

--
Best regards,

Runxi Yu (they/them)
Year 11, E House
YK Pao School SJ
https://runxiyu.org

Runxi Yu (1):
  crypt: Do not return "*" from read-only memory regions

 src/crypt/crypt_blowfish.c | 4 +++-
 src/crypt/crypt_des.c      | 6 +++++-
 src/crypt/crypt_md5.c      | 9 ++++++---
 src/crypt/crypt_sha256.c   | 9 ++++++---
 src/crypt/crypt_sha512.c   | 9 ++++++---
 5 files changed, 26 insertions(+), 11 deletions(-)

-- 
2.47.1

[musl] [PATCH 1/1] crypt: Do not return "*" from read-only memory regions

[Runxi Yu]

https://pubs.opengroup.org/onlinepubs/9799919799/functions/crypt.html:
> The return value of crypt() points to static data that is overwritten
> by each call.
>
> Upon successful completion, crypt() shall return a pointer to the
> hashed password; the first two bytes of the returned value shall be
> those of the salt argument. Otherwise, it shall return a null pointer
> and set errno to indicate the error.

Returning "*" is probably incorrect. It's rather reasonable for the
caller to attempt to erase whatever is returned by crypt, which PAM
does, segfaulting passwd(1) on Alpine for passwords above 256 characters
long.

Signed-off-by: Runxi Yu <me@runxiyu.org>
---
 src/crypt/crypt_blowfish.c | 4 +++-
 src/crypt/crypt_des.c      | 6 +++++-
 src/crypt/crypt_md5.c      | 9 ++++++---
 src/crypt/crypt_sha256.c   | 9 ++++++---
 src/crypt/crypt_sha512.c   | 9 ++++++---
 5 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/src/crypt/crypt_blowfish.c b/src/crypt/crypt_blowfish.c
index d722607b..9a46ae3b 100644
--- a/src/crypt/crypt_blowfish.c
+++ b/src/crypt/crypt_blowfish.c
@@ -47,6 +47,7 @@
  * hadn't seen his code).
  */
 
+#include <errno.h>
 #include <string.h>
 #include <stdint.h>
 
@@ -802,5 +803,6 @@ char *__crypt_blowfish(const char *key, const char *setting, char *output)
 	if (ok && retval)
 		return retval;
 
-	return "*";
+	errno = EINVAL;
+	return NULL;
 }
diff --git a/src/crypt/crypt_des.c b/src/crypt/crypt_des.c
index 338a8f37..2790034a 100644
--- a/src/crypt/crypt_des.c
+++ b/src/crypt/crypt_des.c
@@ -53,6 +53,7 @@
  * by David Burren.  It has been heavily re-worked by Solar Designer.
  */
 
+#include <errno.h>
 #include <stdint.h>
 #include <string.h>
 
@@ -1012,5 +1013,8 @@ char *__crypt_des(const char *key, const char *setting, char *output)
 	if (p && !strcmp(p, test_hash) && retval)
 		return retval;
 
-	return (setting[0]=='*') ? "x" : "*";
+	if (setting[0]=='*')
+		return "x";
+	errno = EINVAL;
+	return NULL;
 }
diff --git a/src/crypt/crypt_md5.c b/src/crypt/crypt_md5.c
index 6e75b36c..e6e2d9b4 100644
--- a/src/crypt/crypt_md5.c
+++ b/src/crypt/crypt_md5.c
@@ -4,8 +4,9 @@
  * original md5 crypt design is from Poul-Henning Kamp
  * this implementation was created based on the code in freebsd
  * at least 32bit int is assumed, key is limited and $1$ prefix is mandatory,
- * on error "*" is returned
+ * on error NULL is returned
  */
+#include <errno.h>
 #include <string.h>
 #include <stdint.h>
 
@@ -279,7 +280,9 @@ char *__crypt_md5(const char *key, const char *setting, char *output)
 	p = md5crypt(key, setting, output);
 	/* self test and stack cleanup */
 	q = md5crypt(testkey, testsetting, testbuf);
-	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash))
-		return "*";
+	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash)) {
+		errno = EINVAL;
+		return NULL;
+	}
 	return p;
 }
diff --git a/src/crypt/crypt_sha256.c b/src/crypt/crypt_sha256.c
index e885dc68..03f45da6 100644
--- a/src/crypt/crypt_sha256.c
+++ b/src/crypt/crypt_sha256.c
@@ -5,9 +5,10 @@
  * in this implementation at least 32bit int is assumed,
  * key length is limited, the $5$ prefix is mandatory, '\n' and ':' is rejected
  * in the salt and rounds= setting must contain a valid iteration count,
- * on error "*" is returned.
+ * on error NULL is returned.
  */
 #include <ctype.h>
+#include <errno.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
@@ -316,7 +317,9 @@ char *__crypt_sha256(const char *key, const char *setting, char *output)
 	p = sha256crypt(key, setting, output);
 	/* self test and stack cleanup */
 	q = sha256crypt(testkey, testsetting, testbuf);
-	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash))
-		return "*";
+	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash)) {
+		errno = EINVAL;
+		return NULL;
+	}
 	return p;
 }
diff --git a/src/crypt/crypt_sha512.c b/src/crypt/crypt_sha512.c
index 39970caf..1024bdf3 100644
--- a/src/crypt/crypt_sha512.c
+++ b/src/crypt/crypt_sha512.c
@@ -5,9 +5,10 @@
  * in this implementation at least 32bit int is assumed,
  * key length is limited, the $6$ prefix is mandatory, '\n' and ':' is rejected
  * in the salt and rounds= setting must contain a valid iteration count,
- * on error "*" is returned.
+ * on error NULL is returned.
  */
 #include <ctype.h>
+#include <errno.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
@@ -365,7 +366,9 @@ char *__crypt_sha512(const char *key, const char *setting, char *output)
 	p = sha512crypt(key, setting, output);
 	/* self test and stack cleanup */
 	q = sha512crypt(testkey, testsetting, testbuf);
-	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash))
-		return "*";
+	if (!p || q != testbuf || memcmp(testbuf, testhash, sizeof testhash)) {
+		errno = EINVAL;
+		return NULL;
+	}
 	return p;
 }
-- 
2.47.1