base/SOURCES/libuser-CVE-2015-3246.patch

2015-06-26  Miloslav Trmač  <mitr@redhat.com>

        * modules/files.c (open_and_copy_file): Replace and heavily modify  ...
        (lu_files_create_backup): ... this.
        (lock_file_handle_existing, lock_file_create, lock_file_remove)
        (struct editing, editing_open, replace_file_or_symlink)
        (editing_close): New functions.
        (generic_lookup, generic_is_locked, lu_files_enumerate)
        (lu_files_users_enumerate_by_group, lu_files_groups_enumerate_by_user)
        (lu_files_enumerate_full): Remove locking on read-only operations.
        (generic_add, generic_mod, generic_del, generic_lock)
        (generic_setpass): Use struct editing instead of dealing with locking,
        backups, SELinux individually.

        * lib/user_private.h (lu_util_lock_obtain, lu_util_lock_free): Mark
        as deprecated.

        * lib/util.c (lu_util_field_write): Fail on an incomplete write().

2015-06-25  Miloslav Trmač  <mitr@redhat.com>

        * modules/files.c (format_generic, generic_setpass): Refuse to write
        field values which contain \n.
        * tests/files_test.py (Tests.testUserAdd9, Tests.testUserMod8)
        (tests.testUserSetpass5, tests.testGroupAdd6, tests.testGroupMod7)
        (tests.testGroupSetpass4): New tests.

diff -up libuser-0.60/lib/user_private.h.CVE-2015-3246 libuser-0.60/lib/user_private.h
--- libuser-0.60/lib/user_private.h.CVE-2015-3246	2013-10-12 23:56:07.000000000 +0200
+++ libuser-0.60/lib/user_private.h	2015-07-08 15:15:14.060544103 +0200
@@ -330,9 +330,11 @@ typedef char lu_security_context_t; /* "
   ((void)(PATH), (void)(MODE), (void)(ERROR), TRUE)
 #endif

-/* Lock a file. */
+#ifndef LU_DISABLE_DEPRECATED
+/* Lock a file. Deprecated. */
 gpointer lu_util_lock_obtain(int fd, struct lu_error **error);
 void lu_util_lock_free(gpointer lock);
+#endif

 /* Manipulate a colon-delimited flat text file. */
 char *lu_util_line_get_matching1(int fd, const char *firstpart,
diff -up libuser-0.60/lib/util.c.CVE-2015-3246 libuser-0.60/lib/util.c
--- libuser-0.60/lib/util.c.CVE-2015-3246	2013-10-12 23:56:07.000000000 +0200
+++ libuser-0.60/lib/util.c	2015-07-08 15:15:14.060544103 +0200
@@ -632,7 +632,7 @@ lu_util_field_write(int fd, const char *
		goto err;
	}
	len = strlen(buf);
-	if (write(fd, buf, len) == -1) {
+	if (write(fd, buf, len) != len) {
		lu_error_new(error, lu_error_write, NULL);
		ret = FALSE;
		goto err;
diff -up libuser-0.60/modules/files.c.CVE-2015-3246 libuser-0.60/modules/files.c
--- libuser-0.60/modules/files.c.CVE-2015-3246	2013-10-12 23:56:07.000000000 +0200
+++ libuser-0.60/modules/files.c	2015-07-08 15:16:41.014981429 +0200
@@ -25,6 +25,7 @@
 #include <fcntl.h>
 #include <fnmatch.h>
 #include <limits.h>
+#include <shadow.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -101,82 +102,79 @@ module_filename(struct lu_module *module
	return g_strconcat(dir, file_suffix, NULL);
 }

-/* Create a backup copy of "filename" named "filename-". */
-static gboolean
-lu_files_create_backup(const char *filename,
-		       struct lu_error **error)
+/* Copy contents of INPUT_FILENAME to OUTPUT_FILENAME, exclusively creating it
+ * if EXCLUSIVE.
+ * Return the file descriptor for OUTPUT_FILENAME, open for reading and writing,
+ * or -1 on error.
+ * Note that this does no locking and assumes the directories hosting the files
+ * are not being manipulated by an attacker. */
+static int
+open_and_copy_file(const char *input_filename, const char *output_filename,
+		   gboolean exclusive, struct lu_error **error)
 {
	int ifd, ofd;
-	gpointer ilock, olock;
-	char *backupname;
-	struct stat ist, ost;
-	off_t offset;
-	gboolean res = FALSE;
+	struct stat st;
+	int res = -1;
+	int flags;

-	g_assert(filename != NULL);
-	g_assert(strlen(filename) > 0);
+	g_assert(input_filename != NULL);
+	g_assert(strlen(input_filename) > 0);
+	g_assert(output_filename != NULL);
+	g_assert(strlen(output_filename) > 0);

-	/* Open the original file. */
-	ifd = open(filename, O_RDONLY);
+	/* Open the input file. */
+	ifd = open(input_filename, O_RDONLY);
	if (ifd == -1) {
		lu_error_new(error, lu_error_open,
-			     _("couldn't open `%s': %s"), filename,
+			     _("couldn't open `%s': %s"), input_filename,
			     strerror(errno));
		goto err;
	}

-	/* Lock the input file. */
-	if ((ilock = lu_util_lock_obtain(ifd, error)) == NULL)
-		goto err_ifd;
-
	/* Read the input file's size. */
-	if (fstat(ifd, &ist) == -1) {
+	if (fstat(ifd, &st) == -1) {
		lu_error_new(error, lu_error_stat,
-			     _("couldn't stat `%s': %s"), filename,
+			     _("couldn't stat `%s': %s"), input_filename,
			     strerror(errno));
-		goto err_ilock;
+		goto err_ifd;
	}

-	/* Generate the backup file's name and open it, creating it if it
-	 * doesn't already exist. */
-	backupname = g_strconcat(filename, "-", NULL);
-	ofd = open(backupname, O_WRONLY | O_CREAT, ist.st_mode);
+	/* We only need O_WRONLY, but the caller needs RDWR if ofd will be
+	 * used as e->new_fd. */
+	flags = O_RDWR | O_CREAT;
+	if (exclusive) {
+		/* This ensures that if there is a concurrent writer which is
+		 * not doing locking for some reason, we will not truncate their
+		 * temporary file. Still, the other writer may truncate our
+		 * file, and ultimately the rename() committing the changes will
+		 * lose one or the other set of changes. */
+		(void)unlink(output_filename);
+		flags |= O_EXCL;
+	} else
+		flags |= O_TRUNC;
+	/* Start with absolutely restrictive permissions to make sure nobody
+	 * can get a file descriptor for this file until we are done resetting
+	 * ownership. */
+	ofd = open(output_filename, flags, 0);
	if (ofd == -1) {
		lu_error_new(error, lu_error_open,
-			     _("error creating `%s': %s"), backupname,
+			     _("error creating `%s': %s"), output_filename,
			     strerror(errno));
-		goto err_backupname;
-	}
-
-	/* If we can't read its type, or it's not a normal file, bail. */
-	if (fstat(ofd, &ost) == -1) {
-		lu_error_new(error, lu_error_stat, _("couldn't stat `%s': %s"),
-			     backupname, strerror(errno));
-		goto err_ofd;
-	}
-	if (!S_ISREG(ost.st_mode)) {
-		lu_error_new(error, lu_error_open,
-			     _("backup file `%s' exists and is not a regular file"),
-			     backupname);
-		goto err_ofd;
+		goto err_ifd;
	}

-	/* Now lock the output file. */
-	if ((olock = lu_util_lock_obtain(ofd, error)) == NULL)
-		goto err_ofd;
-
	/* Set the permissions on the new file to match the old one. */
-	if (fchown(ofd, ist.st_uid, ist.st_gid) == -1 && errno != EPERM) {
+	if (fchown(ofd, st.st_uid, st.st_gid) == -1 && errno != EPERM) {
		lu_error_new(error, lu_error_generic,
-			     _("Error changing owner of `%s': %s"), backupname,
-			     strerror(errno));
-		goto err_olock;
+			     _("Error changing owner of `%s': %s"),
+			     output_filename, strerror(errno));
+		goto err_ofd;
	}
-	if (fchmod(ofd, ist.st_mode) == -1) {
+	if (fchmod(ofd, st.st_mode) == -1) {
		lu_error_new(error, lu_error_generic,
-			     _("Error changing mode of `%s': %s"), backupname,
-			     strerror(errno));
-		goto err_olock;
+			     _("Error changing mode of `%s': %s"),
+			     output_filename, strerror(errno));
+		goto err_ofd;
	}

	/* Copy the data, block by block. */
@@ -190,9 +188,9 @@ lu_files_create_backup(const char *filen
			if (errno == EINTR)
				continue;
			lu_error_new(error, lu_error_read,
-				     _("Error reading `%s': %s"), filename,
-				     strerror(errno));
-			goto err_olock;
+				     _("Error reading `%s': %s"),
+				     input_filename, strerror(errno));
+			goto err_ofd;
		}
		if (left == 0)
			break;
@@ -206,55 +204,297 @@ lu_files_create_backup(const char *filen
					continue;
				lu_error_new(error, lu_error_write,
					     _("Error writing `%s': %s"),
-					     backupname, strerror(errno));
-				goto err_olock;
+					     output_filename, strerror(errno));
+				goto err_ofd;
			}
			p += out;
			left -= out;
		}
	}

-	/* Flush data to disk, and truncate at the current offset.  This is
-	 * necessary if the file existed before we opened it. */
-	fsync(ofd);
-	offset = lseek(ofd, 0, SEEK_CUR);
-	if (offset == -1 || ftruncate(ofd, offset) == -1) {
-		lu_error_new(error, lu_error_generic,
-			     _("Error writing `%s': %s"), backupname,
-			     strerror(errno));
-		goto err_olock;
-	}
-
-	/* Re-read data about the output file. */
-	if (fstat(ofd, &ost) == -1) {
-		lu_error_new(error, lu_error_stat,
-			     _("couldn't stat `%s': %s"), backupname,
-			     strerror(errno));
-		goto err_olock;
-	}
-
-	/* Complain if the files are somehow not the same. */
-	if (ist.st_size != ost.st_size) {
-		lu_error_new(error, lu_error_generic,
-			     _("backup file size mismatch"));
-		goto err_olock;
+	/* Flush data to disk. */
+	if (fsync(ofd) != 0 || lseek(ofd, 0, SEEK_SET) == -1) {
+		lu_error_new(error, lu_error_write, _("Error writing `%s': %s"),
+			     output_filename, strerror(errno));
+		goto err_ofd;
	}
-	res = TRUE;
+	res = ofd;
+	goto err_ifd; /* Do not close ofd */

- err_olock:
-	lu_util_lock_free(olock);
  err_ofd:
	close(ofd);
- err_backupname:
-	g_free(backupname);
- err_ilock:
-	lu_util_lock_free(ilock);
  err_ifd:
	close(ifd);
  err:
	return res;
 }

+/* Deal with an existing LOCK_FILENAME.
+ * Return TRUE if the caller should try again. */
+static gboolean
+lock_file_handle_existing(const char *lock_filename, struct lu_error **error)
+{
+	gchar *lock_contents;
+	GError *gerror;
+	gboolean ret = FALSE;
+	uintmax_t pid;
+	char *p;
+
+	gerror = NULL;
+	if (g_file_get_contents(lock_filename, &lock_contents, NULL, &gerror)
+	    == FALSE) {
+		lu_error_new(error, lu_error_read,
+			     _("couldn't read from `%s': %s"), lock_filename,
+			     gerror->message);
+		g_error_free(gerror);
+		goto err;
+	}
+	errno = 0;
+	pid = strtoumax(lock_contents, &p, 10);
+	if (errno != 0 || *p != 0 || p == lock_contents || (pid_t)pid != pid) {
+		lu_error_new(error, lu_error_lock,
+			     _("Invalid contents of lock `%s'"), lock_filename);
+		goto err_lock_contents;
+	}
+	if (kill(pid, 0) == 0 || errno != ESRCH) {
+		lu_error_new(error, lu_error_lock,
+			     _("The lock %s is held by process %ju"),
+			     lock_filename, pid);
+		goto err_lock_contents;
+	}
+	/* This is unfixably racy, but that should matter only if a genuine
+	 * lock owner crashes. */
+	if (unlink(lock_filename) != 0) {
+		lu_error_new(error, lu_error_lock,
+		     _("Error removing stale lock `%s': %s"), lock_filename,
+		     strerror(errno));
+		goto err_lock_contents;
+	}
+	ret = TRUE;
+	/* Fall through */
+
+err_lock_contents:
+	g_free(lock_contents);
+err:
+	return ret;
+}
+
+/* Create a lock file for FILENAME. */
+static gboolean
+lock_file_create(const char *filename, struct lu_error **error)
+{
+	char *lock_filename, *tmp_filename;
+	char pid_string[sizeof (pid_t) * CHAR_BIT + 1];
+	int fd;
+	gboolean ret = FALSE;
+
+	lock_filename = g_strconcat(filename, ".lock", NULL);
+	tmp_filename = g_strdup_printf("%s.lock.XXXXXX", filename);
+
+	fd = mkstemp(tmp_filename);
+	if (fd == -1) {
+		lu_error_new(error, lu_error_open,
+			     _("error opening temporary file for `%s': %s"),
+			     lock_filename, strerror(errno));
+		goto err_tmp_filename;
+	}
+	if (snprintf(pid_string, sizeof(pid_string), "%ju", (uintmax_t)getpid())
+	    >= sizeof(pid_string))
+		g_assert_not_reached();
+	if (write(fd, pid_string, strlen(pid_string)) != strlen(pid_string)) {
+		lu_error_new(error, lu_error_write, _("Error writing `%s': %s"),
+			     tmp_filename, strerror(errno));
+		close(fd);
+		goto err_tmp_file;
+	}
+	close(fd);
+
+	if (link(tmp_filename, lock_filename) != 0) {
+		if (errno == EEXIST) {
+			if (lock_file_handle_existing(lock_filename, error)
+			    == FALSE)
+				goto err_tmp_file;
+			if (link(tmp_filename, lock_filename) == 0)
+				goto got_link;
+		}
+		lu_error_new(error, lu_error_lock,
+			     _("Cannot obtain lock `%s': %s"), lock_filename,
+			     strerror(errno));
+		goto err_tmp_file;
+	}
+got_link:
+	ret = TRUE;
+	/* Fall through */
+
+err_tmp_file:
+	(void)unlink(tmp_filename);
+err_tmp_filename:
+	g_free(tmp_filename);
+	g_free(lock_filename);
+	return ret;
+}
+
+/* Remove the lock file for FILENAME. */
+static void
+lock_file_remove(const char *filename)
+{
+	char *lock_file;
+
+	lock_file = g_strconcat(filename, ".lock", NULL);
+	(void)unlink(lock_file);
+	g_free(lock_file);
+}
+
+/* State related to a file currently open for editing. */
+struct editing {
+	char *filename;
+	lu_security_context_t fscreate;
+	char *new_filename;
+	int new_fd;
+};
+
+/* Open and lock FILE_SUFFIX in MODULE for editing.
+ * Return editing state, or NULL on error. */
+static struct editing *
+editing_open(struct lu_module *module, const char *file_suffix,
+	     struct lu_error **error)
+{
+	struct editing *e;
+	char *backup_name;
+	int fd;
+
+	e = g_malloc0(sizeof (*e));
+	e->filename = module_filename(module, file_suffix);
+	/* Make sure this all works if e->filename is a symbolic link, at least
+	 * as long as it points to the same file system. */
+
+	if (geteuid() == 0) {
+		if (lckpwdf() != 0) {
+			lu_error_new(error, lu_error_lock,
+				     _("error locking file: %s"),
+				     strerror(errno));
+			goto err_filename;
+		}
+	}
+	if (lock_file_create(e->filename, error) == FALSE)
+		goto err_lckpwdf;
+
+	if (!lu_util_fscreate_save(&e->fscreate, error))
+		goto err_locked;
+	if (!lu_util_fscreate_from_file(e->filename, error))
+		goto err_fscreate;
+
+	backup_name = g_strconcat(e->filename, "-", NULL);
+	fd = open_and_copy_file(e->filename, backup_name, FALSE, error);
+	g_free (backup_name);
+	close(fd);
+	if (fd == -1)
+		goto err_fscreate;
+
+	e->new_filename = g_strconcat(e->filename, "+", NULL);
+	e->new_fd = open_and_copy_file(e->filename, e->new_filename, TRUE,
+			       	       error);
+	if (e->new_fd == -1)
+		goto err_new_filename;
+
+	return e;
+
+err_new_filename:
+ 	g_free(e->new_filename);
+err_fscreate:
+	lu_util_fscreate_restore(e->fscreate);
+
+err_locked:
+	(void)lock_file_remove(e->filename);
+err_lckpwdf:
+	if (geteuid() == 0)
+		(void)ulckpwdf();
+
+err_filename:
+ 	g_free(e->filename);
+ 	g_free(e);
+ 	return NULL;
+}
+
+
+/* Replace DESTINATION with SOURCE, even if DESTINATION is a symbolic link. */
+static gboolean
+replace_file_or_symlink(const char *source, const char *destination,
+		        struct lu_error **error)
+{
+	struct stat st;
+	char *tmp;
+	gboolean ret = FALSE;
+
+	tmp = NULL;
+	if (lstat(destination, &st) == 0 && S_ISLNK(st.st_mode)) {
+		tmp = realpath(destination, NULL);
+		if (tmp == NULL) {
+			lu_error_new(error, lu_error_generic,
+				     _("Error resolving `%s': %s"), destination,
+				     strerror(errno));
+			goto err;
+		}
+		destination = tmp;
+	}
+	if (rename(source, destination) != 0) {
+		lu_error_new(error, lu_error_write,
+			     _("Error replacing `%s': %s"), destination,
+			     strerror(errno));
+		goto err;
+	}
+	ret = TRUE;
+	/* Fall through */
+
+err:
+	free(tmp);
+	return ret;
+}
+
+/* Finish editing E, commit edits if COMMIT.
+ * Return true only if RET_INPUT and everything went OK; suggested usage is
+ *  ret = editing_close(e, commit, ret, error); */
+static gboolean
+editing_close(struct editing *e, gboolean commit, gboolean ret_input,
+	      struct lu_error **error)
+{
+	gboolean ret = FALSE;
+	gboolean unlink_new_filename = TRUE;
+
+	g_assert(e != NULL);
+
+	if (commit && fsync(e->new_fd) != 0) {
+		lu_error_new(error, lu_error_write, _("Error writing `%s': %s"),
+			     e->new_filename, strerror(errno));
+		close(e->new_fd);
+		goto err;
+	}
+	close(e->new_fd);
+
+	if (commit) {
+		if (replace_file_or_symlink(e->new_filename, e->filename,
+					    error) == FALSE)
+			goto err;
+		unlink_new_filename = FALSE;
+	}
+	ret = ret_input;
+
+err:
+	if (unlink_new_filename)
+		(void)unlink(e->new_filename);
+	g_free(e->new_filename);
+	lu_util_fscreate_restore(e->fscreate);
+
+	(void)lock_file_remove(e->filename);
+	if (geteuid() == 0)
+		(void)ulckpwdf();
+
+	g_free(e->filename);
+	g_free(e);
+	return ret;
+}
+
+
 /* Read a line from the file, no matter how long it is, and return it as a
  * newly-allocated string, with the terminator intact. */
 static char *
@@ -435,7 +675,6 @@ generic_lookup(struct lu_module *module,
 {
	gboolean ret;
	int fd = -1;
-	gpointer lock;
	char *line, *filename;

	g_assert(module != NULL);
@@ -446,7 +685,7 @@ generic_lookup(struct lu_module *module,

	filename = module_filename(module, file_suffix);

-	/* Open the file and lock it. */
+	/* Open the file. */
	fd = open(filename, O_RDONLY);
	if (fd == -1) {
		lu_error_new(error, lu_error_open,
@@ -457,15 +696,9 @@ generic_lookup(struct lu_module *module,
	}
	g_free(filename);

-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
-		close(fd);
-		return FALSE;
-	}
-
	/* Search for the entry in this file. */
	line = lu_util_line_get_matchingx(fd, name, field, error);
	if (line == NULL) {
-		lu_util_lock_free(lock);
		close(fd);
		return FALSE;
	}
@@ -473,7 +706,6 @@ generic_lookup(struct lu_module *module,
	/* If we found data, parse it and then free the data. */
	ret = parser(line, ent);
	g_free(line);
-	lu_util_lock_free(lock);
	close(fd);

	return ret;
@@ -666,6 +898,13 @@ format_generic(struct lu_ent *ent, const
		char *field;

		field = format_field(ent, formats + i);
+		if (strchr(field, '\n') != NULL) {
+			lu_error_new(error, lu_error_invalid_attribute_value,
+				     _("%s value `%s': `\\n' not allowed"),
+				     formats[i].attribute, field);
+			g_free(field);
+			goto err;
+		}
		if (i != format_count - 1 && strchr(field, ':') != NULL) {
			lu_error_new(error, lu_error_invalid_attribute_value,
				     _("%s value `%s': `:' not allowed"),
@@ -729,11 +968,9 @@ generic_add(struct lu_module *module, co
	    const struct format_specifier *formats, size_t format_count,
	    struct lu_ent *ent, struct lu_error **error)
 {
-	lu_security_context_t fscreate;
-	char *line, *filename, *contents;
-	int fd;
+	struct editing *e;
+	char *line, *contents;
	ssize_t r;
-	gpointer lock;
	struct stat st;
	off_t offset;
	gboolean ret = FALSE;
@@ -743,50 +980,30 @@ generic_add(struct lu_module *module, co
	g_assert(format_count > 0);
	g_assert(ent != NULL);

-	filename = module_filename(module, file_suffix);
-
	line = format_generic(ent, formats, format_count, error);
	if (line == NULL)
-		goto err_filename;
+		goto err;

-	if (!lu_util_fscreate_save(&fscreate, error))
+	e = editing_open(module, file_suffix, error);
+	if (e == NULL)
		goto err_line;
-	if (!lu_util_fscreate_from_file(filename, error))
-		goto err_fscreate;
-
-	/* Create a backup copy of the file we're about to modify. */
-	if (lu_files_create_backup(filename, error) == FALSE)
-		goto err_fscreate;
-
-	/* Open the file. */
-	fd = open(filename, O_RDWR);
-	if (fd == -1) {
-		lu_error_new(error, lu_error_open,
-			     _("couldn't open `%s': %s"), filename,
-			     strerror(errno));
-		goto err_fscreate;
-	}
-
-	/* Lock the file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
-		goto err_fd;

	/* Read the file's size. */
-	if (fstat(fd, &st) == -1) {
+	if (fstat(e->new_fd, &st) == -1) {
		lu_error_new(error, lu_error_stat,
-			     _("couldn't stat `%s': %s"), filename,
+			     _("couldn't stat `%s': %s"), e->new_filename,
			     strerror(errno));
-		goto err_lock;
+		goto err_editing;
	}

	/* Read the entire file in.  There's some room for improvement here,
	 * but at least we still have the lock, so it's not going to get
	 * funky on us. */
	contents = g_malloc0(st.st_size + 1);
-	if (read(fd, contents, st.st_size) != st.st_size) {
+	if (read(e->new_fd, contents, st.st_size) != st.st_size) {
		lu_error_new(error, lu_error_read,
			     _("couldn't read from `%s': %s"),
-			     filename, strerror(errno));
+			     e->new_filename, strerror(errno));
		goto err_contents;
	}

@@ -798,54 +1015,43 @@ generic_add(struct lu_module *module, co
		goto err_contents;
	}
	/* Hooray, we can add this entry at the end of the file. */
-	offset = lseek(fd, 0, SEEK_END);
+	offset = lseek(e->new_fd, 0, SEEK_END);
	if (offset == -1) {
		lu_error_new(error, lu_error_write,
			     _("couldn't write to `%s': %s"),
-			     filename, strerror(errno));
+			     e->new_filename, strerror(errno));
		goto err_contents;
	}
	/* If the last byte in the file isn't a newline, add one, and silently
	 * curse people who use text editors (which shall remain unnamed) which
	 * allow saving of the file without a final line terminator. */
	if ((st.st_size > 0) && (contents[st.st_size - 1] != '\n')) {
-		if (write(fd, "\n", 1) != 1) {
+		if (write(e->new_fd, "\n", 1) != 1) {
			lu_error_new(error, lu_error_write,
				     _("couldn't write to `%s': %s"),
-				     filename,
-				     strerror(errno));
+				     e->new_filename, strerror(errno));
			goto err_contents;
		}
	}
	/* Attempt to write the entire line to the end. */
-	r = write(fd, line, strlen(line));
+	r = write(e->new_fd, line, strlen(line));
	if ((size_t)r != strlen(line)) {
		/* Oh, come on! */
		lu_error_new(error, lu_error_write,
-			     _("couldn't write to `%s': %s"),
-			     filename,
+			     _("couldn't write to `%s': %s"), e->new_filename,
			     strerror(errno));
-		/* Truncate off whatever we actually managed to write and
-		 * give up. */
-		(void)ftruncate(fd, offset);
		goto err_contents;
	}
-	/* Hey, it succeeded. */
	ret = TRUE;
	/* Fall through */

 err_contents:
	g_free(contents);
-err_lock:
-	lu_util_lock_free(lock);
-err_fd:
-	close(fd);
-err_fscreate:
-	lu_util_fscreate_restore(fscreate);
+err_editing:
+	ret = editing_close(e, ret, ret, error); /* Commit/rollback happens here. */
 err_line:
	g_free(line);
-err_filename:
-	g_free(filename);
+err:
	return ret;
 }

@@ -938,11 +1144,9 @@ generic_mod(struct lu_module *module, co
	    const struct format_specifier *formats, size_t format_count,
	    struct lu_ent *ent, struct lu_error **error)
 {
-	lu_security_context_t fscreate;
-	char *filename, *new_line, *contents, *line, *rest;
+	struct editing *e;
+	char *new_line, *contents, *line, *rest;
	char *current_name, *fragment;
-	int fd;
-	gpointer lock;
	const char *name_attribute;
	gboolean ret = FALSE;
	struct stat st;
@@ -971,43 +1175,24 @@ generic_mod(struct lu_module *module, co
		return FALSE;
	}

-	filename = module_filename(module, file_suffix);
-
	new_line = format_generic(ent, formats, format_count, error);
	if (new_line == NULL)
-		goto err_filename;
+		goto err_current_name;

-	if (!lu_util_fscreate_save(&fscreate, error))
+	e = editing_open(module, file_suffix, error);
+	if (e == NULL)
		goto err_new_line;
-	if (!lu_util_fscreate_from_file(filename, error))
-		goto err_fscreate;
-	/* Create a backup file. */
-	if (lu_files_create_backup(filename, error) == FALSE)
-		goto err_fscreate;

-	/* Open the file to be modified. */
-	fd = open(filename, O_RDWR);
-	if (fd == -1) {
-		lu_error_new(error, lu_error_open,
-			     _("couldn't open `%s': %s"), filename,
-			     strerror(errno));
-		goto err_fscreate;
-	}
-
-	/* Lock the file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
-		goto err_fd;
-
-	if (fstat(fd, &st) == -1) {
+	if (fstat(e->new_fd, &st) == -1) {
		lu_error_new(error, lu_error_stat, _("couldn't stat `%s': %s"),
-			     filename, strerror(errno));
-		goto err_lock;
+			     e->new_filename, strerror(errno));
+		goto err_editing;
	}

	contents = g_malloc(st.st_size + 1 + strlen(new_line));
-	if (read(fd, contents, st.st_size) != st.st_size) {
+	if (read(e->new_fd, contents, st.st_size) != st.st_size) {
		lu_error_new(error, lu_error_read,
-			     _("couldn't read from `%s': %s"), filename,
+			     _("couldn't read from `%s': %s"), e->new_filename,
			     strerror(errno));
		goto err_contents;
	}
@@ -1045,16 +1230,16 @@ generic_mod(struct lu_module *module, co
	memmove(line + strlen(new_line), rest,
		contents + st.st_size + 1 - rest);
	memcpy(line, new_line, strlen(new_line));
-	if (lseek(fd, line - contents, SEEK_SET) == -1) {
+	if (lseek(e->new_fd, line - contents, SEEK_SET) == -1) {
		lu_error_new(error, lu_error_write, NULL);
		goto err_contents;
	}
	len = strlen(line);
-	if ((size_t)write(fd, line, len) != len) {
+	if ((size_t)write(e->new_fd, line, len) != len) {
		lu_error_new(error, lu_error_write, NULL);
		goto err_contents;
	}
-	if (ftruncate(fd, (line - contents) + len) != 0) {
+	if (ftruncate(e->new_fd, (line - contents) + len) != 0) {
		lu_error_new(error, lu_error_write, NULL);
		goto err_contents;
	}
@@ -1063,16 +1248,11 @@ generic_mod(struct lu_module *module, co

 err_contents:
	g_free(contents);
-err_lock:
-	lu_util_lock_free(lock);
-err_fd:
-	close(fd);
-err_fscreate:
-	lu_util_fscreate_restore(fscreate);
+err_editing:
+	ret = editing_close(e, ret, ret, error); /* Commit/rollback happens here. */
 err_new_line:
	g_free(new_line);
-err_filename:
-	g_free(filename);
+err_current_name:
	g_free(current_name);
	return ret;
 }
@@ -1118,16 +1298,14 @@ static gboolean
 generic_del(struct lu_module *module, const char *file_suffix,
	    struct lu_ent *ent, struct lu_error **error)
 {
-	lu_security_context_t fscreate;
+	struct editing *e;
	char *name;
-	char *contents, *filename;
+	char *contents;
	char *fragment2;
	struct stat st;
	size_t len;
-	int fd;
-        gboolean ret = FALSE;
+        gboolean commit = FALSE, ret = FALSE;
	gboolean found;
-	gpointer lock;

	/* Get the entity's current name. */
	if (ent->type == lu_user)
@@ -1141,42 +1319,23 @@ generic_del(struct lu_module *module, co
	g_assert(module != NULL);
	g_assert(ent != NULL);

-	filename = module_filename(module, file_suffix);
-
-	if (!lu_util_fscreate_save(&fscreate, error))
-		goto err_filename;
-	if (!lu_util_fscreate_from_file(filename, error))
-		goto err_fscreate;
-	/* Create a backup of that file. */
-	if (lu_files_create_backup(filename, error) == FALSE)
-		goto err_fscreate;
-
-	/* Open the file to be modified. */
-	fd = open(filename, O_RDWR);
-	if (fd == -1) {
-		lu_error_new(error, lu_error_open,
-			     _("couldn't open `%s': %s"), filename,
-			     strerror(errno));
-		goto err_fscreate;
-	}
-
-	/* Lock the file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
-		goto err_fd;
+	e = editing_open(module, file_suffix, error);
+	if (e == NULL)
+		goto err_name;

	/* Determine the file's size. */
-	if (fstat(fd, &st) == -1) {
+	if (fstat(e->new_fd, &st) == -1) {
		lu_error_new(error, lu_error_stat,
-			     _("couldn't stat `%s': %s"), filename,
+			     _("couldn't stat `%s': %s"), e->new_filename,
			     strerror(errno));
-		goto err_lock;
+		goto err_editing;
	}

	/* Allocate space to hold the file and read it all in. */
	contents = g_malloc(st.st_size + 1);
-	if (read(fd, contents, st.st_size) != st.st_size) {
+	if (read(e->new_fd, contents, st.st_size) != st.st_size) {
		lu_error_new(error, lu_error_read,
-			     _("couldn't read from `%s': %s"), filename,
+			     _("couldn't read from `%s': %s"), e->new_filename,
			     strerror(errno));
		goto err_contents;
	}
@@ -1229,41 +1388,38 @@ generic_del(struct lu_module *module, co

	/* Otherwise we need to write the new data to the file.  Jump back to
	 * the beginning of the file. */
-	if (lseek(fd, 0, SEEK_SET) == -1) {
+	if (lseek(e->new_fd, 0, SEEK_SET) == -1) {
		lu_error_new(error, lu_error_write,
-			     _("couldn't write to `%s': %s"), filename,
+			     _("couldn't write to `%s': %s"), e->new_filename,
			     strerror(errno));
		goto err_contents;
	}

	/* Write the new contents out. */
-	if ((size_t)write(fd, contents, len) != len) {
+	if ((size_t)write(e->new_fd, contents, len) != len) {
		lu_error_new(error, lu_error_write,
-			     _("couldn't write to `%s': %s"), filename,
+			     _("couldn't write to `%s': %s"), e->new_filename,
			     strerror(errno));
		goto err_contents;
	}

	/* Truncate the file to the new (certainly shorter) length. */
-	if (ftruncate(fd, len) == -1) {
+	if (ftruncate(e->new_fd, len) == -1) {
		lu_error_new(error, lu_error_generic,
-			     _("couldn't write to `%s': %s"), filename,
+			     _("couldn't write to `%s': %s"), e->new_filename,
			     strerror(errno));
		goto err_contents;
	}
+	commit = TRUE;
	ret = TRUE;
	/* Fall through */

  err_contents:
	g_free(contents);
- err_lock:
-	lu_util_lock_free(lock);
- err_fd:
-	close(fd);
-err_fscreate:
-	lu_util_fscreate_restore(fscreate);
- err_filename:
-	g_free(filename);
+err_editing:
+	/* Commit/rollback happens here. */
+	ret = editing_close(e, commit, ret, error);
+err_name:
	g_free(name);
	return ret;
 }
@@ -1344,12 +1500,9 @@ static gboolean
 generic_lock(struct lu_module *module, const char *file_suffix, int field,
	     struct lu_ent *ent, enum lock_op op, struct lu_error **error)
 {
-	lu_security_context_t fscreate;
-	char *filename;
+	struct editing *e;
	char *value, *new_value, *name;
-	int fd;
-	gpointer lock;
-	gboolean ret = FALSE;
+	gboolean commit = FALSE, ret = FALSE;

	/* Get the name which keys the entries of interest in the file. */
	g_assert((ent->type == lu_user) || (ent->type == lu_group));
@@ -1362,33 +1515,14 @@ generic_lock(struct lu_module *module, c
	g_assert(module != NULL);
	g_assert(ent != NULL);

-	filename = module_filename(module, file_suffix);
-
-	if (!lu_util_fscreate_save(&fscreate, error))
-		goto err_filename;
-	if (!lu_util_fscreate_from_file(filename, error))
-		goto err_fscreate;
-	/* Create a backup of the file. */
-	if (lu_files_create_backup(filename, error) == FALSE)
-		goto err_fscreate;
-
-	/* Open the file. */
-	fd = open(filename, O_RDWR);
-	if (fd == -1) {
-		lu_error_new(error, lu_error_open,
-			     _("couldn't open `%s': %s"), filename,
-			     strerror(errno));
-		goto err_fscreate;
-	}
-
-	/* Lock the file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
-		goto err_fd;
+	e = editing_open(module, file_suffix, error);
+	if (e == NULL)
+		goto err_name;

	/* Read the old value from the file. */
-	value = lu_util_field_read(fd, name, field, error);
+	value = lu_util_field_read(e->new_fd, name, field, error);
	if (value == NULL)
-		goto err_lock;
+		goto err_editing;

	/* Check that we actually care about this.  If there's a non-empty,
	 * not locked string in there, but it's too short to be a hash, then
@@ -1396,27 +1530,27 @@ generic_lock(struct lu_module *module, c
	if (LU_CRYPT_INVALID(value)) {
		g_free(value);
		ret = TRUE;
-		goto err_lock;
+		goto err_editing;
	}

	/* Generate a new value for the file. */
	new_value = lock_process(value, op, ent, error);
	g_free(value);
	if (new_value == NULL)
-		goto err_lock;
+		goto err_editing;

	/* Make the change. */
-	ret = lu_util_field_write(fd, name, field, new_value, error);
+	if (lu_util_field_write(e->new_fd, name, field, new_value, error)
+	    == FALSE)
+		goto err_editing;
+	commit = TRUE;
+	ret = TRUE;
	/* Fall through */

-err_lock:
-	lu_util_lock_free(lock);
- err_fd:
-	close(fd);
-err_fscreate:
-	lu_util_fscreate_restore(fscreate);
- err_filename:
-	g_free(filename);
+err_editing:
+	/* Commit/rollback happens here. */
+	ret = editing_close(e, commit, ret, error);
+err_name:
	g_free(name);
	return ret;
 }
@@ -1429,7 +1563,6 @@ generic_is_locked(struct lu_module *modu
	char *filename;
	char *value, *name;
	int fd;
-	gpointer lock;
	gboolean ret = FALSE;

	/* Get the name of this account. */
@@ -1454,22 +1587,16 @@ generic_is_locked(struct lu_module *modu
		goto err_filename;
	}

-	/* Lock the file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
-		goto err_fd;
-
	/* Read the value. */
	value = lu_util_field_read(fd, name, field, error);
	if (value == NULL)
-		goto err_lock;
+		goto err_fd;

	/* It all comes down to this. */
	ret = value[0] == '!';
	g_free(value);
	/* Fall through */

-err_lock:
-	lu_util_lock_free(lock);
 err_fd:
	close(fd);
 err_filename:
@@ -1624,10 +1751,8 @@ generic_setpass(struct lu_module *module
		struct lu_ent *ent, const char *password, gboolean is_shadow,
		struct lu_error **error)
 {
-	lu_security_context_t fscreate;
-	char *filename, *value, *name;
-	int fd;
-	gpointer lock;
+	struct editing *e;
+	char *value, *name;
	gboolean ret = FALSE;

	/* Get the name of this account. */
@@ -1641,34 +1766,14 @@ generic_setpass(struct lu_module *module
	g_assert(module != NULL);
	g_assert(ent != NULL);

-	filename = module_filename(module, file_suffix);
-
-	if (!lu_util_fscreate_save(&fscreate, error))
-		goto err_filename;
-	if (!lu_util_fscreate_from_file(filename, error))
-		goto err_fscreate;
-
-	/* Create a backup of the file. */
-	if (lu_files_create_backup(filename, error) == FALSE)
-		goto err_filename;
-
-	/* Open the file. */
-	fd = open(filename, O_RDWR);
-	if (fd == -1) {
-		lu_error_new(error, lu_error_open,
-			     _("couldn't open `%s': %s"), filename,
-			     strerror(errno));
-		goto err_fscreate;
-	}
-
-	/* Lock the file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
-		goto err_fd;
+	e = editing_open(module, file_suffix, error);
+	if (e == NULL)
+		goto err_name;

	/* Read the current contents of the field. */
-	value = lu_util_field_read(fd, name, field, error);
+	value = lu_util_field_read(e->new_fd, name, field, error);
	if (value == NULL)
-		goto err_lock;
+		goto err_editing;

	/* pam_unix uses shadow passwords only if pw_passwd is "x"
	   (or ##${username}).  Make sure to preserve the shadow marker
@@ -1693,9 +1798,9 @@ generic_setpass(struct lu_module *module
	else if (g_ascii_strncasecmp(password, LU_CRYPTED, strlen(LU_CRYPTED))
		 == 0) {
		password = password + strlen(LU_CRYPTED);
-		if (strchr(password, ':') != NULL) {
+		if (strpbrk(password, ":\n") != NULL) {
			lu_error_new(error, lu_error_invalid_attribute_value,
-				     _("`:' not allowed in encrypted "
+				     _("`:' and `\\n' not allowed in encrypted "
				       "password"));
			goto err_value;
		}
@@ -1713,19 +1818,14 @@ generic_setpass(struct lu_module *module
	}

	/* Now write our changes to the file. */
-	ret = lu_util_field_write(fd, name, field, password, error);
+	ret = lu_util_field_write(e->new_fd, name, field, password, error);
	/* Fall through */

- err_value:
+err_value:
	g_free(value);
-err_lock:
-	lu_util_lock_free(lock);
- err_fd:
-	close(fd);
-err_fscreate:
-	lu_util_fscreate_restore(fscreate);
- err_filename:
-	g_free(filename);
+err_editing:
+	ret = editing_close(e, ret, ret, error); /* Commit/rollback happens here. */
+err_name:
	g_free(name);
	return ret;
 }
@@ -1802,7 +1902,6 @@ lu_files_enumerate(struct lu_module *mod
		   const char *pattern, struct lu_error **error)
 {
	int fd;
-	gpointer lock;
	GValueArray *ret;
	GValue value;
	char *buf;
@@ -1824,20 +1923,12 @@ lu_files_enumerate(struct lu_module *mod
		return NULL;
	}

-	/* Lock the file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
-		close(fd);
-		g_free(filename);
-		return NULL;
-	}
-
	/* Wrap the file for stdio operations. */
	fp = fdopen(fd, "r");
	if (fp == NULL) {
		lu_error_new(error, lu_error_open,
			     _("couldn't open `%s': %s"), filename,
			     strerror(errno));
-		lu_util_lock_free(lock);
		close(fd);
		g_free(filename);
		return NULL;
@@ -1873,7 +1964,6 @@ lu_files_enumerate(struct lu_module *mod

	/* Clean up. */
	g_value_unset(&value);
-	lu_util_lock_free(lock);
	fclose(fp);
	g_free(filename);

@@ -1902,7 +1992,6 @@ lu_files_users_enumerate_by_group(struct
				  struct lu_error **error)
 {
	int fd;
-	gpointer lock;
	GValueArray *ret;
	GValue value;
	char *buf, grp[CHUNK_SIZE];
@@ -1927,21 +2016,12 @@ lu_files_users_enumerate_by_group(struct
		return NULL;
	}

-	/* Lock the passwd file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
-		close(fd);
-		g_free(pwdfilename);
-		g_free(grpfilename);
-		return NULL;
-	}
-
	/* Wrap the descriptor in a stdio FILE. */
	fp = fdopen(fd, "r");
	if (fp == NULL) {
		lu_error_new(error, lu_error_open,
			     _("couldn't open `%s': %s"), pwdfilename,
			     strerror(errno));
-		lu_util_lock_free(lock);
		close(fd);
		g_free(pwdfilename);
		g_free(grpfilename);
@@ -2000,7 +2080,6 @@ lu_files_users_enumerate_by_group(struct
	}
	/* Close the file. */
	g_value_unset(&value);
-	lu_util_lock_free(lock);
	fclose(fp);

	/* Open the group file. */
@@ -2015,22 +2094,12 @@ lu_files_users_enumerate_by_group(struct
		return NULL;
	}

-	/* Lock the group file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
-		close(fd);
-		g_free(pwdfilename);
-		g_free(grpfilename);
-		g_value_array_free(ret);
-		return NULL;
-	}
-
	/* Wrap the group file in an stdio file. */
	fp = fdopen(fd, "r");
	if (fp == NULL) {
		lu_error_new(error, lu_error_open,
			     _("couldn't open `%s': %s"), grpfilename,
			     strerror(errno));
-		lu_util_lock_free(lock);
		close(fd);
		g_free(pwdfilename);
		g_free(grpfilename);
@@ -2085,7 +2154,6 @@ lu_files_users_enumerate_by_group(struct
	}

	/* Clean up. */
-	lu_util_lock_free(lock);
	fclose(fp);

	g_free(pwdfilename);
@@ -2102,7 +2170,6 @@ lu_files_groups_enumerate_by_user(struct
				  struct lu_error **error)
 {
	int fd;
-	gpointer lock;
	GValueArray *ret;
	GValue value;
	char *buf;
@@ -2126,19 +2193,12 @@ lu_files_groups_enumerate_by_user(struct
		goto err_pwdfilename;
	}

-	/* Lock it. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
-		close(fd);
-		goto err_pwdfilename;
-	}
-
	/* Open it so that we can use stdio. */
	fp = fdopen(fd, "r");
	if (fp == NULL) {
		lu_error_new(error, lu_error_open,
			     _("couldn't open `%s': %s"), pwdfilename,
			     strerror(errno));
-		lu_util_lock_free(lock);
		close(fd);
		goto err_pwdfilename;
	}
@@ -2186,7 +2246,6 @@ lu_files_groups_enumerate_by_user(struct
		}
		g_free(buf);
	}
-	lu_util_lock_free(lock);
	fclose(fp);

	/* Open the groups file. */
@@ -2198,19 +2257,12 @@ lu_files_groups_enumerate_by_user(struct
		goto err_key;
	}

-	/* Lock it. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
-		close(fd);
-		goto err_key;
-	}
-
	/* Open it so that we can use stdio. */
	fp = fdopen(fd, "r");
	if (fp == NULL) {
		lu_error_new(error, lu_error_open,
			     _("couldn't open `%s': %s"), grpfilename,
			     strerror(errno));
-		lu_util_lock_free(lock);
		close(fd);
		goto err_key;
	}
@@ -2267,7 +2319,6 @@ lu_files_groups_enumerate_by_user(struct
	g_free(key);
	g_value_unset(&value);

-	lu_util_lock_free(lock);
	fclose(fp);
	g_free(pwdfilename);
	g_free(grpfilename);
@@ -2291,7 +2342,6 @@ lu_files_enumerate_full(struct lu_module
			struct lu_error **error)
 {
	int fd;
-	gpointer lock;
	GPtrArray *ret = NULL;
	char *buf;
	char *key, *filename;
@@ -2311,19 +2361,12 @@ lu_files_enumerate_full(struct lu_module
		goto err_filename;
	}

-	/* Lock the file. */
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
-		close(fd);
-		goto err_filename;
-	}
-
	/* Wrap the file up in stdio. */
	fp = fdopen(fd, "r");
	if (fp == NULL) {
		lu_error_new(error, lu_error_open,
			     _("couldn't open `%s': %s"), filename,
			     strerror(errno));
-		lu_util_lock_free(lock);
		close(fd);
		goto err_filename;
	}
@@ -2358,7 +2401,6 @@ lu_files_enumerate_full(struct lu_module
		g_free(key);
	}

-	lu_util_lock_free(lock);
	fclose(fp);

  err_filename:
diff -up libuser-0.60/tests/files_test.py.CVE-2015-3246 libuser-0.60/tests/files_test.py
--- libuser-0.60/tests/files_test.py.CVE-2015-3246	2013-10-12 23:56:08.000000000 +0200
+++ libuser-0.60/tests/files_test.py	2015-07-08 15:15:14.061544074 +0200
@@ -262,6 +262,21 @@ class Tests(unittest.TestCase):
         e = self.a.initUser('user6_8')
         self.assertRaises(RuntimeError, self.a.addUser, e, False, False)

+    def testUserAdd9(self):
+        # '\n' in field values
+        # libuser.USERPASSWORD not tested because lu_shadow_user_add_prep()
+        # always replaces it by 'x'.  libuser.UIDNUMBER not tested because
+        # ent_has_name_and_id() interprets the value as a number.
+        for field in (libuser.USERNAME, libuser.GIDNUMBER, libuser.GECOS,
+                      libuser.HOMEDIRECTORY, libuser.LOGINSHELL,
+                      libuser.SHADOWPASSWORD, libuser.SHADOWLASTCHANGE,
+                      libuser.SHADOWMIN, libuser.SHADOWMAX,
+                      libuser.SHADOWWARNING, libuser.SHADOWINACTIVE,
+                      libuser.SHADOWEXPIRE, libuser.SHADOWFLAG):
+            e = self.a.initUser('user_6_9' + field)
+            e[field] = str(e[field][0]) + '\nx'
+            self.assertRaises(RuntimeError, self.a.addUser, e, False, False)
+
     def testUserMod1(self):
         # A minimal case
         e = self.a.initUser('user7_1')
@@ -421,6 +436,26 @@ class Tests(unittest.TestCase):
         e[libuser.USERNAME] = 'user7_7'
         self.assertRaises(RuntimeError, self.a.modifyUser, e, False)

+    def testUserMod8(self):
+        # '\n' in field values
+        # libuser.USERPASSWORD not tested because lu_shadow_user_add_prep()
+        # always replaces it by 'x'.  libuser.UIDNUMBER not tested because
+        # ent_has_name_and_id() interprets the value as a number.
+        for field in (libuser.USERNAME, libuser.USERPASSWORD, libuser.GIDNUMBER,
+                      libuser.GECOS, libuser.HOMEDIRECTORY, libuser.LOGINSHELL,
+                      libuser.SHADOWPASSWORD, libuser.SHADOWLASTCHANGE,
+                      libuser.SHADOWMIN, libuser.SHADOWMAX,
+                      libuser.SHADOWWARNING, libuser.SHADOWINACTIVE,
+                      libuser.SHADOWEXPIRE, libuser.SHADOWFLAG):
+            e = self.a.initUser('user7_9' + field)
+            self.a.addUser(e, False, False)
+            del e
+            e = self.a.lookupUserByName('user7_9' + field)
+            self.assertIsNotNone(e)
+            self.assertNotIn('\n', str(e[field][0]))
+            e[field] = str(e[field][0]) + '\nx'
+            self.assertRaises(RuntimeError, self.a.modifyUser, e, False)
+
     def testUserDel(self):
         e = self.a.initUser('user8_1')
         self.a.addUser(e, False, False)
@@ -619,6 +654,22 @@ class Tests(unittest.TestCase):
         crypted = crypt.crypt('a:b', e[libuser.SHADOWPASSWORD][0])
         self.assertEqual(e[libuser.SHADOWPASSWORD], [crypted])

+    def testUserSetpass5(self):
+        # '\n' in field value
+        e = self.a.initUser('user12_5')
+        self.a.addUser(e, False, False)
+        del e
+        e = self.a.lookupUserByName('user12_5')
+        self.assertNotIn('\n', e[libuser.SHADOWPASSWORD][0])
+        self.assertRaises(SystemError, self.a.setpassUser, e, 'a\nb', True)
+        self.a.setpassUser(e, 'a\nb', False)
+        del e
+        e = self.a.lookupUserByName('user12_5')
+        self.assertEqual(e[libuser.SHADOWPASSWORD][0][:3], '$1$')
+        self.assertNotIn('\n', e[libuser.SHADOWPASSWORD][0])
+        crypted = crypt.crypt('a\nb', e[libuser.SHADOWPASSWORD][0])
+        self.assertEqual(e[libuser.SHADOWPASSWORD], [crypted])
+
     def testUserRemovepass(self):
         e = self.a.initUser('user13_1')
         e[libuser.SHADOWPASSWORD] = '03dgZm5nZvqOc'
@@ -884,6 +935,20 @@ class Tests(unittest.TestCase):
         e = self.a.initGroup('group21_5')
         self.assertRaises(RuntimeError, self.a.addGroup, e)

+    def testGroupAdd6(self):
+        # '\n' in field values
+        # libuser.GROUPPASSWORD not tested because lu_shadow_group_add_prep()
+        # always replaces it by 'x'.  libuser.GIDNUMBER not tested because
+        # ent_has_name_and_id() interprets the value as a number.
+        for field in (libuser.GROUPNAME, libuser.SHADOWPASSWORD,
+                      libuser.MEMBERNAME, libuser.ADMINISTRATORNAME):
+            e = self.a.initGroup('group_21_6' + field)
+            if e.has_key(field):
+                e[field] = str(e[field][0]) + '\nx'
+            else:
+                e[field] = field + '\nx'
+            self.assertRaises(RuntimeError, self.a.addGroup, e)
+
     def testGroupMod1(self):
         # A minimal case
         e = self.a.initGroup('group22_1')
@@ -997,6 +1062,25 @@ class Tests(unittest.TestCase):
         e[libuser.GROUPNAME] = 'group22_6'
         self.assertRaises(RuntimeError, self.a.modifyGroup, e)

+    def testGroupMod7(self):
+        # '\n' in field values
+        # libuser.GIDNUMBER not tested because ent_has_name_and_id() interprets
+        # the value as a number.
+        for field in (libuser.GROUPNAME, libuser.GROUPPASSWORD,
+                      libuser.SHADOWPASSWORD, libuser.MEMBERNAME,
+                      libuser.ADMINISTRATORNAME):
+            e = self.a.initGroup('group22_8' + field)
+            self.a.addGroup(e)
+            del e
+            e = self.a.lookupGroupByName('group22_8' + field)
+            self.assertIsNotNone(e)
+            if e.has_key(field):
+                self.assertNotIn('\n', str(e[field][0]))
+                e[field] = str(e[field][0]) + '\nx'
+            else:
+                e[field] = field + '\nx'
+            self.assertRaises(RuntimeError, self.a.modifyGroup, e)
+
     def testGroupDel(self):
         e = self.a.initGroup('group23_1')
         self.a.addGroup(e)
@@ -1190,6 +1274,22 @@ class Tests(unittest.TestCase):
         crypted = crypt.crypt('a:b', e[libuser.SHADOWPASSWORD][0])
         self.assertEqual(e[libuser.SHADOWPASSWORD], [crypted])

+    def testGroupSetpass4(self):
+        # '\n' in field value
+        e = self.a.initGroup('group27_5')
+        self.a.addGroup(e)
+        del e
+        e = self.a.lookupGroupByName('group27_5')
+        self.assertNotIn('\n', e[libuser.SHADOWPASSWORD][0])
+        self.assertRaises(SystemError, self.a.setpassGroup, e, 'a\nb', True)
+        self.a.setpassGroup(e, 'a\nb', False)
+        del e
+        e = self.a.lookupGroupByName('group27_5')
+        self.assertEqual(e[libuser.SHADOWPASSWORD][0][:3], '$1$')
+        self.assertNotIn('\n', e[libuser.SHADOWPASSWORD][0])
+        crypted = crypt.crypt('a\nb', e[libuser.SHADOWPASSWORD][0])
+        self.assertEqual(e[libuser.SHADOWPASSWORD], [crypted])
+
     def testGroupRemovepass(self):
         e = self.a.initGroup('group28_1')
         e[libuser.SHADOWPASSWORD] = '07Js7N.eEhbgs'