Review 52a38d55097bf0532b0eaa97552e001203808e88

* Changelog: Update documentation of 2013-07-28  mancha entry.
	* lib/prototypes.h, lib/encrypt.c: Update splint marker,
	pw_encrypt can return NULL.
	* lib/encrypt.c: Fix outdated statement on GNU crypt.
	* src/chgpasswd.c: Improve diagnostic to user when pw_encrypt
	fails and use fail_exit() instead of exit().
	* src/chpasswd.c: Likewise.
	* src/newusers.c: Likewise.
	* src/passwd.c: Likewise when new password is encrypted.
	* src/newgrp.c: Improve diagnostic to user and syslog when
	pw_encrypt fails.  Do not apply 1s penalty as this is not an
	invalid password issue.
	* src/passwd.c: Likewise when password is checked.

ChangeLog

@@ -1,3 +1,19 @@
+2013-08-03  Nicolas François  <nicolas.francois@centraliens.net>
+
+	* Changelog: Update documentation of 2013-07-28  mancha entry.
+	* lib/prototypes.h, lib/encrypt.c: Update splint marker,
+	pw_encrypt can return NULL.
+	* lib/encrypt.c: Fix outdated statement on GNU crypt.
+	* src/chgpasswd.c: Improve diagnostic to user when pw_encrypt
+	fails and use fail_exit() instead of exit().
+	* src/chpasswd.c: Likewise.
+	* src/newusers.c: Likewise.
+	* src/passwd.c: Likewise when new password is encrypted.
+	* src/newgrp.c: Improve diagnostic to user and syslog when
+	pw_encrypt fails.  Do not apply 1s penalty as this is not an
+	invalid password issue.
+	* src/passwd.c: Likewise when password is checked.
+
 2013-08-02  Nicolas François  <nicolas.francois@centraliens.net>
 
 	* libmisc/setupenv.c: xstrdup the static char* temp_pw_dir /
@@ -52,19 +68,24 @@
 
 2013-07-28  mancha  <mancha1@hush.com>
 
-        * lib/encrypt.c: crypt() in glibc/eglibc 2.17 now fails if passed
+	* lib/encrypt.c (pw_encrypt): crypt() in glibc/eglibc 2.17 now
-          a salt that violates specs. On Linux, crypt() also fails with
+	fails if passed a salt that violates specs. On Linux, crypt() also
-          DES/MD5 salts in FIPS140 mode. Rather than exit() on NULL returns
+	fails with DES/MD5 salts in FIPS140 mode. Rather than exit() on
-          we send them back to the caller for appropriate handling.
+	NULL returns we send them back to the caller for appropriate
-          Closes: alioth#314234
+	handling (instead of exiting).  Closes: alioth#314234
-        * lib/pwauth.c: Handle NULL return from crypt().
+	* lib/pwauth.c: Handle NULL return from pw_crypt(), return non
-        * libmisc/valid.c: Likewise.
+	zero (as in case of failure).
-        * src/chgpasswd.c: Likewise.
+	* libmisc/valid.c: Likewise.
-        * src/chpasswd.c: Likewise.
+	* src/chgpasswd.c: Handle NULL return from pw_crypt(), report
-        * src/gpasswd.c: Likewise.
+	crypt error to stderr and exit.
-        * src/newgrp.c: Likewise.
+	* src/chpasswd.c: Likewise.
-        * src/newusers.c: Likewise.
+	* src/gpasswd.c: Likewise.
-        * src/passwd.c: Likewise.
+	* src/newusers.c: Likewise.
+	* src/passwd.c: Likewise when new password is encrypted.
+	* src/newgrp.c: Handle NULL return from pw_crypt(), report crypt
+	error to stderr and syslog and return to report unchanged
+	password.
+	* src/passwd.c: Likewise when password is checked.
 
 2013-07-28  Christian Perrier  <christian@perrier.eu.org>
 

lib/encrypt.c

@@ -40,22 +40,22 @@
 #include "prototypes.h"
 #include "defines.h"
 
-/*@exposed@*/char *pw_encrypt (const char *clear, const char *salt)
+/*@exposed@*//*@null@*/char *pw_encrypt (const char *clear, const char *salt)
 {
 	static char cipher[128];
 	char *cp;
 
 	cp = crypt (clear, salt);
-	if (!cp) {
+	if (NULL == cp) {
 		/*
 		 * Single Unix Spec: crypt() may return a null pointer,
 		 * and set errno to indicate an error. In this case return
 		 * the NULL so the caller can handle appropriately.
 		 */
-		return cp;
+		return NULL;
 	}
 
-	/* The GNU crypt does not return NULL if the algorithm is not
+	/* Some crypt() do not return NULL if the algorithm is not
 	 * supported, and return a DES encrypted password. */
 	if ((NULL != salt) && (salt[0] == '$') && (strlen (cp) <= 13))
 	{

lib/prototypes.h

@@ -124,7 +124,7 @@ extern int copy_tree (const char *src_root, const char *dst_root,
                       gid_t old_gid, gid_t new_gid);
 
 /* encrypt.c */
-extern /*@exposed@*/char *pw_encrypt (const char *, const char *);
+extern /*@exposed@*//*@null@*/char *pw_encrypt (const char *, const char *);
 
 /* entry.c */
 extern void pw_entry (const char *, struct passwd *);

lib/pwauth.c

@@ -179,10 +179,11 @@ int pw_auth (const char *cipher,
 	 */
 
 	encrypted = pw_encrypt (input, cipher);
-	if (encrypted!=NULL)
+	if (NULL != encrypted) {
 		retval = strcmp (encrypted, cipher);
-	else
+	} else {
 		retval = -1;
+	}
 
 #ifdef  SKEY
 	/*

src/chgpasswd.c

@@ -459,6 +459,7 @@ int main (int argc, char **argv)
 		    && (   (NULL == crypt_method)
 		        || (0 != strcmp (crypt_method, "NONE")))) {
 			void *arg = NULL;
+			const char *salt;
 			if (md5flg) {
 				crypt_method = "MD5";
 			}
@@ -467,12 +468,14 @@ int main (int argc, char **argv)
 				arg = &sha_rounds;
 			}
 #endif
-			cp = pw_encrypt (newpwd,
+			salt = crypt_make_salt (crypt_method, arg);
-			                 crypt_make_salt (crypt_method, arg));
+			cp = pw_encrypt (newpwd, salt);
-			if (cp == NULL) {
+			if (NULL == cp) {
-				perror ("crypt");
+				fprintf (stderr,
-				exit (EXIT_FAILURE);
+				         _("%s: failed to crypt password with salt '%s': %s\n"),
-			}	
+				         Prog, salt, strerror (errno));
+				fail_exit (1);
+			}
 		}
 
 		/*

src/chpasswd.c

@@ -482,6 +482,7 @@ int main (int argc, char **argv)
 		    && (   (NULL == crypt_method)
 		        || (0 != strcmp (crypt_method, "NONE")))) {
 			void *arg = NULL;
+			const char *salt;
 			if (md5flg) {
 				crypt_method = "MD5";
 			}
@@ -490,11 +491,13 @@ int main (int argc, char **argv)
 				arg = &sha_rounds;
 			}
 #endif
-			cp = pw_encrypt (newpwd,
+			salt = crypt_make_salt (crypt_method, arg);
-			                 crypt_make_salt(crypt_method, arg));
+			cp = pw_encrypt (newpwd, salt);
-			if (cp == NULL) {
+			if (NULL == cp) {
-				perror ("crypt");
+				fprintf (stderr,
-				exit (EXIT_FAILURE);
+				         _("%s: failed to crypt password with salt '%s': %s\n"),
+				         Prog, salt, strerror (errno));
+				fail_exit (1);
 			}
 		}
 

src/gpasswd.c

@@ -898,6 +898,7 @@ static void change_passwd (struct group *gr)
 	char *cp;
 	static char pass[BUFSIZ];
 	int retries;
+	const char *salt;
 
 	/*
 	 * A new password is to be entered and it must be encrypted, etc.
@@ -938,10 +939,13 @@ static void change_passwd (struct group *gr)
 		exit (1);
 	}
 
-	cp = pw_encrypt (pass, crypt_make_salt (NULL, NULL));
+	salt = crypt_make_salt (NULL, NULL);
-	if (cp==NULL) {
+	cp = pw_encrypt (pass, salt);
-		perror ("crypt");
+	if (NULL == cp) {
-		exit (EXIT_FAILURE);
+		fprintf (stderr,
+		         _("%s: failed to crypt password with salt '%s': %s\n"),
+		         Prog, salt, strerror (errno));
+		exit (1);
 	}
 	memzero (pass, sizeof pass);
 #ifdef SHADOWGRP

src/newgrp.c

@@ -184,8 +184,17 @@ static void check_perms (const struct group *grp,
 		cpasswd = pw_encrypt (cp, grp->gr_passwd);
 		strzero (cp);
 
-		if (cpasswd == NULL ||
+		if (NULL == cpasswd) {
-		    grp->gr_passwd[0] == '\0' ||
+			fprintf (stderr,
+			         _("%s: failed to crypt password with previous salt: %s\n"),
+			         Prog, strerror (errno));
+			SYSLOG ((LOG_INFO,
+			         "Failed to crypt password with previous salt of group '%s'",
+			         groupname));
+			goto failure;
+		}
+
+		if (grp->gr_passwd[0] == '\0' ||
 		    strcmp (cpasswd, grp->gr_passwd) != 0) {
 #ifdef WITH_AUDIT
 			snprintf (audit_buf, sizeof(audit_buf),

src/newusers.c

@@ -98,7 +98,7 @@ static int add_group (const char *, const char *, gid_t *, gid_t);
 static int get_user_id (const char *, uid_t *);
 static int add_user (const char *, uid_t, gid_t);
 #ifndef USE_PAM
-static void update_passwd (struct passwd *, const char *);
+static int update_passwd (struct passwd *, const char *);
 #endif				/* !USE_PAM */
 static int add_passwd (struct passwd *, const char *);
 static void process_flags (int argc, char **argv);
@@ -384,7 +384,12 @@ static int add_user (const char *name, uid_t uid, gid_t gid)
 }
 
 #ifndef USE_PAM
-static void update_passwd (struct passwd *pwd, const char *password)
+/* 
+ * update_passwd - update the password in the passwd entry
+ *
+ * Return 0 if successful.
+ */
+static int update_passwd (struct passwd *pwd, const char *password)
 {
 	void *crypt_arg = NULL;
 	char *cp;
@@ -399,14 +404,18 @@ static void update_passwd (struct passwd *pwd, const char *password)
 	if ((crypt_method != NULL) && (0 == strcmp(crypt_method, "NONE"))) {
 		pwd->pw_passwd = (char *)password;
 	} else {
-		cp=pw_encrypt (password, crypt_make_salt (crypt_method, 
+		const char *salt = crypt_make_salt (crypt_method, crypt_arg);
-		                                          crypt_arg));
+		cp = pw_encrypt (password, salt);
-		if (cp == NULL) {
+		if (NULL == cp) {
-			perror ("crypt");
+			fprintf (stderr,
-			exit (EXIT_FAILURE);
+			         _("%s: failed to crypt password with salt '%s': %s\n"),
+			         Prog, salt, strerror (errno));
+			return 1;
 		}
 		pwd->pw_passwd = cp;
 	}
+
+	return 0;
 }
 #endif				/* !USE_PAM */
 
@@ -435,8 +444,7 @@ static int add_passwd (struct passwd *pwd, const char *password)
 	 * harder since there are zillions of things to do ...
 	 */
 	if (!is_shadow) {
-		update_passwd (pwd, password);
+		return update_passwd (pwd, password);
-		return 0;
 	}
 #endif				/* USE_PAM */
 
@@ -455,9 +463,11 @@ static int add_passwd (struct passwd *pwd, const char *password)
 			const char *salt = crypt_make_salt (crypt_method,
 			                                    crypt_arg);
 			cp = pw_encrypt (password, salt);
-			if (cp == NULL) {
+			if (NULL == cp) {
-				perror ("crypt");
+				fprintf (stderr,
-				exit (EXIT_FAILURE);
+				         _("%s: failed to crypt password with salt '%s': %s\n"),
+				         Prog, salt, strerror (errno));
+				return 1;
 			}
 			spent.sp_pwdp = cp;
 		}
@@ -477,8 +487,7 @@ static int add_passwd (struct passwd *pwd, const char *password)
 	 * the password set someplace else.
 	 */
 	if (strcmp (pwd->pw_passwd, "x") != 0) {
-		update_passwd (pwd, password);
+		return update_passwd (pwd, password);
-		return 0;
 	}
 #else				/* USE_PAM */
 	/*
@@ -504,9 +513,11 @@ static int add_passwd (struct passwd *pwd, const char *password)
 	} else {
 		const char *salt = crypt_make_salt (crypt_method, crypt_arg);
 		cp = pw_encrypt (password, salt);
-		if (cp == NULL) {
+		if (NULL == cp) {
-			perror ("crypt");
+			fprintf (stderr,
-			exit (EXIT_FAILURE);
+			         _("%s: failed to crypt password with salt '%s': %s\n"),
+			         Prog, salt, strerror (errno));
+			return 1;
 		}
 		spent.sp_pwdp = cp;
 	}

src/passwd.c

@@ -218,6 +218,7 @@ static int new_password (const struct passwd *pw)
 {
 	char *clear;		/* Pointer to clear text */
 	char *cipher;		/* Pointer to cipher text */
+	const char *salt;	/* Pointer to new salt */
 	char *cp;		/* Pointer to getpass() response */
 	char orig[200];		/* Original password */
 	char pass[200];		/* New password */
@@ -242,7 +243,19 @@ static int new_password (const struct passwd *pw)
 		}
 
 		cipher = pw_encrypt (clear, crypt_passwd);
-		if ((cipher == NULL) || (strcmp (cipher, crypt_passwd) != 0)) {
+
+		if (NULL == cipher) {
+			strzero (clear);
+			fprintf (stderr,
+			         _("%s: failed to crypt password with previous salt: %s\n"),
+			         Prog, strerror (errno));
+			SYSLOG ((LOG_INFO,
+			         "Failed to crypt password with previous salt of user '%s'",
+			         pw->pw_name));
+			return -1;
+		}
+
+		if (strcmp (cipher, crypt_passwd) != 0) {
 			strzero (clear);
 			strzero (cipher);
 			SYSLOG ((LOG_WARN, "incorrect password for %s",
@@ -348,13 +361,17 @@ static int new_password (const struct passwd *pw)
 	/*
 	 * Encrypt the password, then wipe the cleartext password.
 	 */
-	cp = pw_encrypt (pass, crypt_make_salt (NULL, NULL));
+	salt = crypt_make_salt (NULL, NULL);
-	if (cp == NULL) {
+	cp = pw_encrypt (pass, salt);
-		perror ("crypt");
-		exit (EXIT_FAILURE);
-	}
 	memzero (pass, sizeof pass);
 
+	if (NULL == cp) {
+		fprintf (stderr,
+		         _("%s: failed to crypt password with salt '%s': %s\n"),
+		         Prog, salt, strerror (errno));
+		return -1;
+	}
+
 #ifdef HAVE_LIBCRACK_HIST
 	HistUpdate (pw->pw_name, crypt_passwd);
 #endif				/* HAVE_LIBCRACK_HIST */