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

cfe9be
2015-06-26  Miloslav Trmač  <mitr@redhat.com>
cfe9be
cfe9be
        * modules/files.c (open_and_copy_file): Replace and heavily modify  ...
cfe9be
        (lu_files_create_backup): ... this.
cfe9be
        (lock_file_handle_existing, lock_file_create, lock_file_remove)
cfe9be
        (struct editing, editing_open, replace_file_or_symlink)
cfe9be
        (editing_close): New functions.
cfe9be
        (generic_lookup, generic_is_locked, lu_files_enumerate)
cfe9be
        (lu_files_users_enumerate_by_group, lu_files_groups_enumerate_by_user)
cfe9be
        (lu_files_enumerate_full): Remove locking on read-only operations.
cfe9be
        (generic_add, generic_mod, generic_del, generic_lock)
cfe9be
        (generic_setpass): Use struct editing instead of dealing with locking,
cfe9be
        backups, SELinux individually.
cfe9be
cfe9be
        * lib/user_private.h (lu_util_lock_obtain, lu_util_lock_free): Mark
cfe9be
        as deprecated.
cfe9be
cfe9be
        * lib/util.c (lu_util_field_write): Fail on an incomplete write().
cfe9be
cfe9be
2015-06-25  Miloslav Trmač  <mitr@redhat.com>
cfe9be
cfe9be
        * modules/files.c (format_generic, generic_setpass): Refuse to write
cfe9be
        field values which contain \n.
cfe9be
        * tests/files_test.py (Tests.testUserAdd9, Tests.testUserMod8)
cfe9be
        (tests.testUserSetpass5, tests.testGroupAdd6, tests.testGroupMod7)
cfe9be
        (tests.testGroupSetpass4): New tests.
cfe9be
cfe9be
diff -up libuser-0.60/lib/user_private.h.CVE-2015-3246 libuser-0.60/lib/user_private.h
cfe9be
--- libuser-0.60/lib/user_private.h.CVE-2015-3246	2013-10-12 23:56:07.000000000 +0200
cfe9be
+++ libuser-0.60/lib/user_private.h	2015-07-08 15:15:14.060544103 +0200
cfe9be
@@ -330,9 +330,11 @@ typedef char lu_security_context_t; /* "
cfe9be
   ((void)(PATH), (void)(MODE), (void)(ERROR), TRUE)
cfe9be
 #endif
cfe9be
 
cfe9be
-/* Lock a file. */
cfe9be
+#ifndef LU_DISABLE_DEPRECATED
cfe9be
+/* Lock a file. Deprecated. */
cfe9be
 gpointer lu_util_lock_obtain(int fd, struct lu_error **error);
cfe9be
 void lu_util_lock_free(gpointer lock);
cfe9be
+#endif
cfe9be
 
cfe9be
 /* Manipulate a colon-delimited flat text file. */
cfe9be
 char *lu_util_line_get_matching1(int fd, const char *firstpart,
cfe9be
diff -up libuser-0.60/lib/util.c.CVE-2015-3246 libuser-0.60/lib/util.c
cfe9be
--- libuser-0.60/lib/util.c.CVE-2015-3246	2013-10-12 23:56:07.000000000 +0200
cfe9be
+++ libuser-0.60/lib/util.c	2015-07-08 15:15:14.060544103 +0200
cfe9be
@@ -632,7 +632,7 @@ lu_util_field_write(int fd, const char *
cfe9be
 		goto err;
cfe9be
 	}
cfe9be
 	len = strlen(buf);
cfe9be
-	if (write(fd, buf, len) == -1) {
cfe9be
+	if (write(fd, buf, len) != len) {
cfe9be
 		lu_error_new(error, lu_error_write, NULL);
cfe9be
 		ret = FALSE;
cfe9be
 		goto err;
cfe9be
diff -up libuser-0.60/modules/files.c.CVE-2015-3246 libuser-0.60/modules/files.c
cfe9be
--- libuser-0.60/modules/files.c.CVE-2015-3246	2013-10-12 23:56:07.000000000 +0200
cfe9be
+++ libuser-0.60/modules/files.c	2015-07-08 15:16:41.014981429 +0200
cfe9be
@@ -25,6 +25,7 @@
cfe9be
 #include <fcntl.h>
cfe9be
 #include <fnmatch.h>
cfe9be
 #include <limits.h>
cfe9be
+#include <shadow.h>
cfe9be
 #include <stdio.h>
cfe9be
 #include <stdlib.h>
cfe9be
 #include <string.h>
cfe9be
@@ -101,82 +102,79 @@ module_filename(struct lu_module *module
cfe9be
 	return g_strconcat(dir, file_suffix, NULL);
cfe9be
 }
cfe9be
 
cfe9be
-/* Create a backup copy of "filename" named "filename-". */
cfe9be
-static gboolean
cfe9be
-lu_files_create_backup(const char *filename,
cfe9be
-		       struct lu_error **error)
cfe9be
+/* Copy contents of INPUT_FILENAME to OUTPUT_FILENAME, exclusively creating it
cfe9be
+ * if EXCLUSIVE.
cfe9be
+ * Return the file descriptor for OUTPUT_FILENAME, open for reading and writing,
cfe9be
+ * or -1 on error.
cfe9be
+ * Note that this does no locking and assumes the directories hosting the files
cfe9be
+ * are not being manipulated by an attacker. */
cfe9be
+static int
cfe9be
+open_and_copy_file(const char *input_filename, const char *output_filename,
cfe9be
+		   gboolean exclusive, struct lu_error **error)
cfe9be
 {
cfe9be
 	int ifd, ofd;
cfe9be
-	gpointer ilock, olock;
cfe9be
-	char *backupname;
cfe9be
-	struct stat ist, ost;
cfe9be
-	off_t offset;
cfe9be
-	gboolean res = FALSE;
cfe9be
+	struct stat st;
cfe9be
+	int res = -1;
cfe9be
+	int flags;
cfe9be
 
cfe9be
-	g_assert(filename != NULL);
cfe9be
-	g_assert(strlen(filename) > 0);
cfe9be
+	g_assert(input_filename != NULL);
cfe9be
+	g_assert(strlen(input_filename) > 0);
cfe9be
+	g_assert(output_filename != NULL);
cfe9be
+	g_assert(strlen(output_filename) > 0);
cfe9be
 
cfe9be
-	/* Open the original file. */
cfe9be
-	ifd = open(filename, O_RDONLY);
cfe9be
+	/* Open the input file. */
cfe9be
+	ifd = open(input_filename, O_RDONLY);
cfe9be
 	if (ifd == -1) {
cfe9be
 		lu_error_new(error, lu_error_open,
cfe9be
-			     _("couldn't open `%s': %s"), filename,
cfe9be
+			     _("couldn't open `%s': %s"), input_filename,
cfe9be
 			     strerror(errno));
cfe9be
 		goto err;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Lock the input file. */
cfe9be
-	if ((ilock = lu_util_lock_obtain(ifd, error)) == NULL)
cfe9be
-		goto err_ifd;
cfe9be
-
cfe9be
 	/* Read the input file's size. */
cfe9be
-	if (fstat(ifd, &ist) == -1) {
cfe9be
+	if (fstat(ifd, &st) == -1) {
cfe9be
 		lu_error_new(error, lu_error_stat,
cfe9be
-			     _("couldn't stat `%s': %s"), filename,
cfe9be
+			     _("couldn't stat `%s': %s"), input_filename,
cfe9be
 			     strerror(errno));
cfe9be
-		goto err_ilock;
cfe9be
+		goto err_ifd;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Generate the backup file's name and open it, creating it if it
cfe9be
-	 * doesn't already exist. */
cfe9be
-	backupname = g_strconcat(filename, "-", NULL);
cfe9be
-	ofd = open(backupname, O_WRONLY | O_CREAT, ist.st_mode);
cfe9be
+	/* We only need O_WRONLY, but the caller needs RDWR if ofd will be
cfe9be
+	 * used as e->new_fd. */
cfe9be
+	flags = O_RDWR | O_CREAT;
cfe9be
+	if (exclusive) {
cfe9be
+		/* This ensures that if there is a concurrent writer which is
cfe9be
+		 * not doing locking for some reason, we will not truncate their
cfe9be
+		 * temporary file. Still, the other writer may truncate our
cfe9be
+		 * file, and ultimately the rename() committing the changes will
cfe9be
+		 * lose one or the other set of changes. */
cfe9be
+		(void)unlink(output_filename);
cfe9be
+		flags |= O_EXCL;
cfe9be
+	} else
cfe9be
+		flags |= O_TRUNC;
cfe9be
+	/* Start with absolutely restrictive permissions to make sure nobody
cfe9be
+	 * can get a file descriptor for this file until we are done resetting
cfe9be
+	 * ownership. */
cfe9be
+	ofd = open(output_filename, flags, 0);
cfe9be
 	if (ofd == -1) {
cfe9be
 		lu_error_new(error, lu_error_open,
cfe9be
-			     _("error creating `%s': %s"), backupname,
cfe9be
+			     _("error creating `%s': %s"), output_filename,
cfe9be
 			     strerror(errno));
cfe9be
-		goto err_backupname;
cfe9be
-	}
cfe9be
-
cfe9be
-	/* If we can't read its type, or it's not a normal file, bail. */
cfe9be
-	if (fstat(ofd, &ost) == -1) {
cfe9be
-		lu_error_new(error, lu_error_stat, _("couldn't stat `%s': %s"),
cfe9be
-			     backupname, strerror(errno));
cfe9be
-		goto err_ofd;
cfe9be
-	}
cfe9be
-	if (!S_ISREG(ost.st_mode)) {
cfe9be
-		lu_error_new(error, lu_error_open,
cfe9be
-			     _("backup file `%s' exists and is not a regular file"),
cfe9be
-			     backupname);
cfe9be
-		goto err_ofd;
cfe9be
+		goto err_ifd;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Now lock the output file. */
cfe9be
-	if ((olock = lu_util_lock_obtain(ofd, error)) == NULL)
cfe9be
-		goto err_ofd;
cfe9be
-
cfe9be
 	/* Set the permissions on the new file to match the old one. */
cfe9be
-	if (fchown(ofd, ist.st_uid, ist.st_gid) == -1 && errno != EPERM) {
cfe9be
+	if (fchown(ofd, st.st_uid, st.st_gid) == -1 && errno != EPERM) {
cfe9be
 		lu_error_new(error, lu_error_generic,
cfe9be
-			     _("Error changing owner of `%s': %s"), backupname,
cfe9be
-			     strerror(errno));
cfe9be
-		goto err_olock;
cfe9be
+			     _("Error changing owner of `%s': %s"),
cfe9be
+			     output_filename, strerror(errno));
cfe9be
+		goto err_ofd;
cfe9be
 	}
cfe9be
-	if (fchmod(ofd, ist.st_mode) == -1) {
cfe9be
+	if (fchmod(ofd, st.st_mode) == -1) {
cfe9be
 		lu_error_new(error, lu_error_generic,
cfe9be
-			     _("Error changing mode of `%s': %s"), backupname,
cfe9be
-			     strerror(errno));
cfe9be
-		goto err_olock;
cfe9be
+			     _("Error changing mode of `%s': %s"),
cfe9be
+			     output_filename, strerror(errno));
cfe9be
+		goto err_ofd;
cfe9be
 	}
cfe9be
 
cfe9be
 	/* Copy the data, block by block. */
cfe9be
@@ -190,9 +188,9 @@ lu_files_create_backup(const char *filen
cfe9be
 			if (errno == EINTR)
cfe9be
 				continue;
cfe9be
 			lu_error_new(error, lu_error_read,
cfe9be
-				     _("Error reading `%s': %s"), filename,
cfe9be
-				     strerror(errno));
cfe9be
-			goto err_olock;
cfe9be
+				     _("Error reading `%s': %s"),
cfe9be
+				     input_filename, strerror(errno));
cfe9be
+			goto err_ofd;
cfe9be
 		}
cfe9be
 		if (left == 0)
cfe9be
 			break;
cfe9be
@@ -206,55 +204,297 @@ lu_files_create_backup(const char *filen
cfe9be
 					continue;
cfe9be
 				lu_error_new(error, lu_error_write,
cfe9be
 					     _("Error writing `%s': %s"),
cfe9be
-					     backupname, strerror(errno));
cfe9be
-				goto err_olock;
cfe9be
+					     output_filename, strerror(errno));
cfe9be
+				goto err_ofd;
cfe9be
 			}
cfe9be
 			p += out;
cfe9be
 			left -= out;
cfe9be
 		}
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Flush data to disk, and truncate at the current offset.  This is
cfe9be
-	 * necessary if the file existed before we opened it. */
cfe9be
-	fsync(ofd);
cfe9be
-	offset = lseek(ofd, 0, SEEK_CUR);
cfe9be
-	if (offset == -1 || ftruncate(ofd, offset) == -1) {
cfe9be
-		lu_error_new(error, lu_error_generic,
cfe9be
-			     _("Error writing `%s': %s"), backupname,
cfe9be
-			     strerror(errno));
cfe9be
-		goto err_olock;
cfe9be
-	}
cfe9be
-
cfe9be
-	/* Re-read data about the output file. */
cfe9be
-	if (fstat(ofd, &ost) == -1) {
cfe9be
-		lu_error_new(error, lu_error_stat,
cfe9be
-			     _("couldn't stat `%s': %s"), backupname,
cfe9be
-			     strerror(errno));
cfe9be
-		goto err_olock;
cfe9be
-	}
cfe9be
-
cfe9be
-	/* Complain if the files are somehow not the same. */
cfe9be
-	if (ist.st_size != ost.st_size) {
cfe9be
-		lu_error_new(error, lu_error_generic,
cfe9be
-			     _("backup file size mismatch"));
cfe9be
-		goto err_olock;
cfe9be
+	/* Flush data to disk. */
cfe9be
+	if (fsync(ofd) != 0 || lseek(ofd, 0, SEEK_SET) == -1) {
cfe9be
+		lu_error_new(error, lu_error_write, _("Error writing `%s': %s"),
cfe9be
+			     output_filename, strerror(errno));
cfe9be
+		goto err_ofd;
cfe9be
 	}
cfe9be
-	res = TRUE;
cfe9be
+	res = ofd;
cfe9be
+	goto err_ifd; /* Do not close ofd */
cfe9be
 
cfe9be
- err_olock:
cfe9be
-	lu_util_lock_free(olock);
cfe9be
  err_ofd:
cfe9be
 	close(ofd);
cfe9be
- err_backupname:
cfe9be
-	g_free(backupname);
cfe9be
- err_ilock:
cfe9be
-	lu_util_lock_free(ilock);
cfe9be
  err_ifd:
cfe9be
 	close(ifd);
cfe9be
  err:
cfe9be
 	return res;
cfe9be
 }
cfe9be
 
cfe9be
+/* Deal with an existing LOCK_FILENAME.
cfe9be
+ * Return TRUE if the caller should try again. */
cfe9be
+static gboolean
cfe9be
+lock_file_handle_existing(const char *lock_filename, struct lu_error **error)
cfe9be
+{
cfe9be
+	gchar *lock_contents;
cfe9be
+	GError *gerror;
cfe9be
+	gboolean ret = FALSE;
cfe9be
+	uintmax_t pid;
cfe9be
+	char *p;
cfe9be
+
cfe9be
+	gerror = NULL;
cfe9be
+	if (g_file_get_contents(lock_filename, &lock_contents, NULL, &gerror)
cfe9be
+	    == FALSE) {
cfe9be
+		lu_error_new(error, lu_error_read,
cfe9be
+			     _("couldn't read from `%s': %s"), lock_filename,
cfe9be
+			     gerror->message);
cfe9be
+		g_error_free(gerror);
cfe9be
+		goto err;
cfe9be
+	}
cfe9be
+	errno = 0;
cfe9be
+	pid = strtoumax(lock_contents, &p, 10);
cfe9be
+	if (errno != 0 || *p != 0 || p == lock_contents || (pid_t)pid != pid) {
cfe9be
+		lu_error_new(error, lu_error_lock,
cfe9be
+			     _("Invalid contents of lock `%s'"), lock_filename);
cfe9be
+		goto err_lock_contents;
cfe9be
+	}
cfe9be
+	if (kill(pid, 0) == 0 || errno != ESRCH) {
cfe9be
+		lu_error_new(error, lu_error_lock,
cfe9be
+			     _("The lock %s is held by process %ju"),
cfe9be
+			     lock_filename, pid);
cfe9be
+		goto err_lock_contents;
cfe9be
+	}
cfe9be
+	/* This is unfixably racy, but that should matter only if a genuine
cfe9be
+	 * lock owner crashes. */
cfe9be
+	if (unlink(lock_filename) != 0) {
cfe9be
+		lu_error_new(error, lu_error_lock,
cfe9be
+		     _("Error removing stale lock `%s': %s"), lock_filename,
cfe9be
+		     strerror(errno));
cfe9be
+		goto err_lock_contents;
cfe9be
+	}
cfe9be
+	ret = TRUE;
cfe9be
+	/* Fall through */
cfe9be
+
cfe9be
+err_lock_contents:
cfe9be
+	g_free(lock_contents);
cfe9be
+err:
cfe9be
+	return ret;
cfe9be
+}
cfe9be
+
cfe9be
+/* Create a lock file for FILENAME. */
cfe9be
+static gboolean
cfe9be
+lock_file_create(const char *filename, struct lu_error **error)
cfe9be
+{
cfe9be
+	char *lock_filename, *tmp_filename;
cfe9be
+	char pid_string[sizeof (pid_t) * CHAR_BIT + 1];
cfe9be
+	int fd;
cfe9be
+	gboolean ret = FALSE;
cfe9be
+
cfe9be
+	lock_filename = g_strconcat(filename, ".lock", NULL);
cfe9be
+	tmp_filename = g_strdup_printf("%s.lock.XXXXXX", filename);
cfe9be
+
cfe9be
+	fd = mkstemp(tmp_filename);
cfe9be
+	if (fd == -1) {
cfe9be
+		lu_error_new(error, lu_error_open,
cfe9be
+			     _("error opening temporary file for `%s': %s"),
cfe9be
+			     lock_filename, strerror(errno));
cfe9be
+		goto err_tmp_filename;
cfe9be
+	}
cfe9be
+	if (snprintf(pid_string, sizeof(pid_string), "%ju", (uintmax_t)getpid())
cfe9be
+	    >= sizeof(pid_string))
cfe9be
+		g_assert_not_reached();
cfe9be
+	if (write(fd, pid_string, strlen(pid_string)) != strlen(pid_string)) {
cfe9be
+		lu_error_new(error, lu_error_write, _("Error writing `%s': %s"),
cfe9be
+			     tmp_filename, strerror(errno));
cfe9be
+		close(fd);
cfe9be
+		goto err_tmp_file;
cfe9be
+	}
cfe9be
+	close(fd);
cfe9be
+
cfe9be
+	if (link(tmp_filename, lock_filename) != 0) {
cfe9be
+		if (errno == EEXIST) {
cfe9be
+			if (lock_file_handle_existing(lock_filename, error)
cfe9be
+			    == FALSE)
cfe9be
+				goto err_tmp_file;
cfe9be
+			if (link(tmp_filename, lock_filename) == 0)
cfe9be
+				goto got_link;
cfe9be
+		}
cfe9be
+		lu_error_new(error, lu_error_lock,
cfe9be
+			     _("Cannot obtain lock `%s': %s"), lock_filename,
cfe9be
+			     strerror(errno));
cfe9be
+		goto err_tmp_file;
cfe9be
+	}
cfe9be
+got_link:
cfe9be
+	ret = TRUE;
cfe9be
+	/* Fall through */
cfe9be
+
cfe9be
+err_tmp_file:
cfe9be
+	(void)unlink(tmp_filename);
cfe9be
+err_tmp_filename:
cfe9be
+	g_free(tmp_filename);
cfe9be
+	g_free(lock_filename);
cfe9be
+	return ret;
cfe9be
+}
cfe9be
+
cfe9be
+/* Remove the lock file for FILENAME. */
cfe9be
+static void
cfe9be
+lock_file_remove(const char *filename)
cfe9be
+{
cfe9be
+	char *lock_file;
cfe9be
+
cfe9be
+	lock_file = g_strconcat(filename, ".lock", NULL);
cfe9be
+	(void)unlink(lock_file);
cfe9be
+	g_free(lock_file);
cfe9be
+}
cfe9be
+
cfe9be
+/* State related to a file currently open for editing. */
cfe9be
+struct editing {
cfe9be
+	char *filename;
cfe9be
+	lu_security_context_t fscreate;
cfe9be
+	char *new_filename;
cfe9be
+	int new_fd;
cfe9be
+};
cfe9be
+
cfe9be
+/* Open and lock FILE_SUFFIX in MODULE for editing.
cfe9be
+ * Return editing state, or NULL on error. */
cfe9be
+static struct editing *
cfe9be
+editing_open(struct lu_module *module, const char *file_suffix,
cfe9be
+	     struct lu_error **error)
cfe9be
+{
cfe9be
+	struct editing *e;
cfe9be
+	char *backup_name;
cfe9be
+	int fd;
cfe9be
+
cfe9be
+	e = g_malloc0(sizeof (*e));
cfe9be
+	e->filename = module_filename(module, file_suffix);
cfe9be
+	/* Make sure this all works if e->filename is a symbolic link, at least
cfe9be
+	 * as long as it points to the same file system. */
cfe9be
+
cfe9be
+	if (geteuid() == 0) {
cfe9be
+		if (lckpwdf() != 0) {
cfe9be
+			lu_error_new(error, lu_error_lock,
cfe9be
+				     _("error locking file: %s"),
cfe9be
+				     strerror(errno));
cfe9be
+			goto err_filename;
cfe9be
+		}
cfe9be
+	}
cfe9be
+	if (lock_file_create(e->filename, error) == FALSE)
cfe9be
+		goto err_lckpwdf;
cfe9be
+
cfe9be
+	if (!lu_util_fscreate_save(&e->fscreate, error))
cfe9be
+		goto err_locked;
cfe9be
+	if (!lu_util_fscreate_from_file(e->filename, error))
cfe9be
+		goto err_fscreate;
cfe9be
+
cfe9be
+	backup_name = g_strconcat(e->filename, "-", NULL);
cfe9be
+	fd = open_and_copy_file(e->filename, backup_name, FALSE, error);
cfe9be
+	g_free (backup_name);
cfe9be
+	close(fd);
cfe9be
+	if (fd == -1)
cfe9be
+		goto err_fscreate;
cfe9be
+
cfe9be
+	e->new_filename = g_strconcat(e->filename, "+", NULL);
cfe9be
+	e->new_fd = open_and_copy_file(e->filename, e->new_filename, TRUE,
cfe9be
+			       	       error);
cfe9be
+	if (e->new_fd == -1)
cfe9be
+		goto err_new_filename;
cfe9be
+
cfe9be
+	return e;
cfe9be
+
cfe9be
+err_new_filename:
cfe9be
+ 	g_free(e->new_filename);
cfe9be
+err_fscreate:
cfe9be
+	lu_util_fscreate_restore(e->fscreate);
cfe9be
+
cfe9be
+err_locked:
cfe9be
+	(void)lock_file_remove(e->filename);
cfe9be
+err_lckpwdf:
cfe9be
+	if (geteuid() == 0)
cfe9be
+		(void)ulckpwdf();
cfe9be
+
cfe9be
+err_filename:
cfe9be
+ 	g_free(e->filename);
cfe9be
+ 	g_free(e);
cfe9be
+ 	return NULL;
cfe9be
+}
cfe9be
+
cfe9be
+
cfe9be
+/* Replace DESTINATION with SOURCE, even if DESTINATION is a symbolic link. */
cfe9be
+static gboolean
cfe9be
+replace_file_or_symlink(const char *source, const char *destination,
cfe9be
+		        struct lu_error **error)
cfe9be
+{
cfe9be
+	struct stat st;
cfe9be
+	char *tmp;
cfe9be
+	gboolean ret = FALSE;
cfe9be
+
cfe9be
+	tmp = NULL;
cfe9be
+	if (lstat(destination, &st) == 0 && S_ISLNK(st.st_mode)) {
cfe9be
+		tmp = realpath(destination, NULL);
cfe9be
+		if (tmp == NULL) {
cfe9be
+			lu_error_new(error, lu_error_generic,
cfe9be
+				     _("Error resolving `%s': %s"), destination,
cfe9be
+				     strerror(errno));
cfe9be
+			goto err;
cfe9be
+		}
cfe9be
+		destination = tmp;
cfe9be
+	}
cfe9be
+	if (rename(source, destination) != 0) {
cfe9be
+		lu_error_new(error, lu_error_write,
cfe9be
+			     _("Error replacing `%s': %s"), destination,
cfe9be
+			     strerror(errno));
cfe9be
+		goto err;
cfe9be
+	}
cfe9be
+	ret = TRUE;
cfe9be
+	/* Fall through */
cfe9be
+
cfe9be
+err:
cfe9be
+	free(tmp);
cfe9be
+	return ret;
cfe9be
+}
cfe9be
+
cfe9be
+/* Finish editing E, commit edits if COMMIT.
cfe9be
+ * Return true only if RET_INPUT and everything went OK; suggested usage is
cfe9be
+ *  ret = editing_close(e, commit, ret, error); */
cfe9be
+static gboolean
cfe9be
+editing_close(struct editing *e, gboolean commit, gboolean ret_input,
cfe9be
+	      struct lu_error **error)
cfe9be
+{
cfe9be
+	gboolean ret = FALSE;
cfe9be
+	gboolean unlink_new_filename = TRUE;
cfe9be
+
cfe9be
+	g_assert(e != NULL);
cfe9be
+
cfe9be
+	if (commit && fsync(e->new_fd) != 0) {
cfe9be
+		lu_error_new(error, lu_error_write, _("Error writing `%s': %s"),
cfe9be
+			     e->new_filename, strerror(errno));
cfe9be
+		close(e->new_fd);
cfe9be
+		goto err;
cfe9be
+	}
cfe9be
+	close(e->new_fd);
cfe9be
+
cfe9be
+	if (commit) {
cfe9be
+		if (replace_file_or_symlink(e->new_filename, e->filename,
cfe9be
+					    error) == FALSE)
cfe9be
+			goto err;
cfe9be
+		unlink_new_filename = FALSE;
cfe9be
+	}
cfe9be
+	ret = ret_input;
cfe9be
+
cfe9be
+err:
cfe9be
+	if (unlink_new_filename)
cfe9be
+		(void)unlink(e->new_filename);
cfe9be
+	g_free(e->new_filename);
cfe9be
+	lu_util_fscreate_restore(e->fscreate);
cfe9be
+
cfe9be
+	(void)lock_file_remove(e->filename);
cfe9be
+	if (geteuid() == 0)
cfe9be
+		(void)ulckpwdf();
cfe9be
+
cfe9be
+	g_free(e->filename);
cfe9be
+	g_free(e);
cfe9be
+	return ret;
cfe9be
+}
cfe9be
+
cfe9be
+
cfe9be
 /* Read a line from the file, no matter how long it is, and return it as a
cfe9be
  * newly-allocated string, with the terminator intact. */
cfe9be
 static char *
cfe9be
@@ -435,7 +675,6 @@ generic_lookup(struct lu_module *module,
cfe9be
 {
cfe9be
 	gboolean ret;
cfe9be
 	int fd = -1;
cfe9be
-	gpointer lock;
cfe9be
 	char *line, *filename;
cfe9be
 
cfe9be
 	g_assert(module != NULL);
cfe9be
@@ -446,7 +685,7 @@ generic_lookup(struct lu_module *module,
cfe9be
 
cfe9be
 	filename = module_filename(module, file_suffix);
cfe9be
 
cfe9be
-	/* Open the file and lock it. */
cfe9be
+	/* Open the file. */
cfe9be
 	fd = open(filename, O_RDONLY);
cfe9be
 	if (fd == -1) {
cfe9be
 		lu_error_new(error, lu_error_open,
cfe9be
@@ -457,15 +696,9 @@ generic_lookup(struct lu_module *module,
cfe9be
 	}
cfe9be
 	g_free(filename);
cfe9be
 
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
cfe9be
-		close(fd);
cfe9be
-		return FALSE;
cfe9be
-	}
cfe9be
-
cfe9be
 	/* Search for the entry in this file. */
cfe9be
 	line = lu_util_line_get_matchingx(fd, name, field, error);
cfe9be
 	if (line == NULL) {
cfe9be
-		lu_util_lock_free(lock);
cfe9be
 		close(fd);
cfe9be
 		return FALSE;
cfe9be
 	}
cfe9be
@@ -473,7 +706,6 @@ generic_lookup(struct lu_module *module,
cfe9be
 	/* If we found data, parse it and then free the data. */
cfe9be
 	ret = parser(line, ent);
cfe9be
 	g_free(line);
cfe9be
-	lu_util_lock_free(lock);
cfe9be
 	close(fd);
cfe9be
 
cfe9be
 	return ret;
cfe9be
@@ -666,6 +898,13 @@ format_generic(struct lu_ent *ent, const
cfe9be
 		char *field;
cfe9be
 
cfe9be
 		field = format_field(ent, formats + i);
cfe9be
+		if (strchr(field, '\n') != NULL) {
cfe9be
+			lu_error_new(error, lu_error_invalid_attribute_value,
cfe9be
+				     _("%s value `%s': `\\n' not allowed"),
cfe9be
+				     formats[i].attribute, field);
cfe9be
+			g_free(field);
cfe9be
+			goto err;
cfe9be
+		}
cfe9be
 		if (i != format_count - 1 && strchr(field, ':') != NULL) {
cfe9be
 			lu_error_new(error, lu_error_invalid_attribute_value,
cfe9be
 				     _("%s value `%s': `:' not allowed"),
cfe9be
@@ -729,11 +968,9 @@ generic_add(struct lu_module *module, co
cfe9be
 	    const struct format_specifier *formats, size_t format_count,
cfe9be
 	    struct lu_ent *ent, struct lu_error **error)
cfe9be
 {
cfe9be
-	lu_security_context_t fscreate;
cfe9be
-	char *line, *filename, *contents;
cfe9be
-	int fd;
cfe9be
+	struct editing *e;
cfe9be
+	char *line, *contents;
cfe9be
 	ssize_t r;
cfe9be
-	gpointer lock;
cfe9be
 	struct stat st;
cfe9be
 	off_t offset;
cfe9be
 	gboolean ret = FALSE;
cfe9be
@@ -743,50 +980,30 @@ generic_add(struct lu_module *module, co
cfe9be
 	g_assert(format_count > 0);
cfe9be
 	g_assert(ent != NULL);
cfe9be
 
cfe9be
-	filename = module_filename(module, file_suffix);
cfe9be
-
cfe9be
 	line = format_generic(ent, formats, format_count, error);
cfe9be
 	if (line == NULL)
cfe9be
-		goto err_filename;
cfe9be
+		goto err;
cfe9be
 
cfe9be
-	if (!lu_util_fscreate_save(&fscreate, error))
cfe9be
+	e = editing_open(module, file_suffix, error);
cfe9be
+	if (e == NULL)
cfe9be
 		goto err_line;
cfe9be
-	if (!lu_util_fscreate_from_file(filename, error))
cfe9be
-		goto err_fscreate;
cfe9be
-
cfe9be
-	/* Create a backup copy of the file we're about to modify. */
cfe9be
-	if (lu_files_create_backup(filename, error) == FALSE)
cfe9be
-		goto err_fscreate;
cfe9be
-
cfe9be
-	/* Open the file. */
cfe9be
-	fd = open(filename, O_RDWR);
cfe9be
-	if (fd == -1) {
cfe9be
-		lu_error_new(error, lu_error_open,
cfe9be
-			     _("couldn't open `%s': %s"), filename,
cfe9be
-			     strerror(errno));
cfe9be
-		goto err_fscreate;
cfe9be
-	}
cfe9be
-
cfe9be
-	/* Lock the file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
cfe9be
-		goto err_fd;
cfe9be
 
cfe9be
 	/* Read the file's size. */
cfe9be
-	if (fstat(fd, &st) == -1) {
cfe9be
+	if (fstat(e->new_fd, &st) == -1) {
cfe9be
 		lu_error_new(error, lu_error_stat,
cfe9be
-			     _("couldn't stat `%s': %s"), filename,
cfe9be
+			     _("couldn't stat `%s': %s"), e->new_filename,
cfe9be
 			     strerror(errno));
cfe9be
-		goto err_lock;
cfe9be
+		goto err_editing;
cfe9be
 	}
cfe9be
 
cfe9be
 	/* Read the entire file in.  There's some room for improvement here,
cfe9be
 	 * but at least we still have the lock, so it's not going to get
cfe9be
 	 * funky on us. */
cfe9be
 	contents = g_malloc0(st.st_size + 1);
cfe9be
-	if (read(fd, contents, st.st_size) != st.st_size) {
cfe9be
+	if (read(e->new_fd, contents, st.st_size) != st.st_size) {
cfe9be
 		lu_error_new(error, lu_error_read,
cfe9be
 			     _("couldn't read from `%s': %s"),
cfe9be
-			     filename, strerror(errno));
cfe9be
+			     e->new_filename, strerror(errno));
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
 
cfe9be
@@ -798,54 +1015,43 @@ generic_add(struct lu_module *module, co
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
 	/* Hooray, we can add this entry at the end of the file. */
cfe9be
-	offset = lseek(fd, 0, SEEK_END);
cfe9be
+	offset = lseek(e->new_fd, 0, SEEK_END);
cfe9be
 	if (offset == -1) {
cfe9be
 		lu_error_new(error, lu_error_write,
cfe9be
 			     _("couldn't write to `%s': %s"),
cfe9be
-			     filename, strerror(errno));
cfe9be
+			     e->new_filename, strerror(errno));
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
 	/* If the last byte in the file isn't a newline, add one, and silently
cfe9be
 	 * curse people who use text editors (which shall remain unnamed) which
cfe9be
 	 * allow saving of the file without a final line terminator. */
cfe9be
 	if ((st.st_size > 0) && (contents[st.st_size - 1] != '\n')) {
cfe9be
-		if (write(fd, "\n", 1) != 1) {
cfe9be
+		if (write(e->new_fd, "\n", 1) != 1) {
cfe9be
 			lu_error_new(error, lu_error_write,
cfe9be
 				     _("couldn't write to `%s': %s"),
cfe9be
-				     filename,
cfe9be
-				     strerror(errno));
cfe9be
+				     e->new_filename, strerror(errno));
cfe9be
 			goto err_contents;
cfe9be
 		}
cfe9be
 	}
cfe9be
 	/* Attempt to write the entire line to the end. */
cfe9be
-	r = write(fd, line, strlen(line));
cfe9be
+	r = write(e->new_fd, line, strlen(line));
cfe9be
 	if ((size_t)r != strlen(line)) {
cfe9be
 		/* Oh, come on! */
cfe9be
 		lu_error_new(error, lu_error_write,
cfe9be
-			     _("couldn't write to `%s': %s"),
cfe9be
-			     filename,
cfe9be
+			     _("couldn't write to `%s': %s"), e->new_filename,
cfe9be
 			     strerror(errno));
cfe9be
-		/* Truncate off whatever we actually managed to write and
cfe9be
-		 * give up. */
cfe9be
-		(void)ftruncate(fd, offset);
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
-	/* Hey, it succeeded. */
cfe9be
 	ret = TRUE;
cfe9be
 	/* Fall through */
cfe9be
 
cfe9be
 err_contents:
cfe9be
 	g_free(contents);
cfe9be
-err_lock:
cfe9be
-	lu_util_lock_free(lock);
cfe9be
-err_fd:
cfe9be
-	close(fd);
cfe9be
-err_fscreate:
cfe9be
-	lu_util_fscreate_restore(fscreate);
cfe9be
+err_editing:
cfe9be
+	ret = editing_close(e, ret, ret, error); /* Commit/rollback happens here. */
cfe9be
 err_line:
cfe9be
 	g_free(line);
cfe9be
-err_filename:
cfe9be
-	g_free(filename);
cfe9be
+err:
cfe9be
 	return ret;
cfe9be
 }
cfe9be
 
cfe9be
@@ -938,11 +1144,9 @@ generic_mod(struct lu_module *module, co
cfe9be
 	    const struct format_specifier *formats, size_t format_count,
cfe9be
 	    struct lu_ent *ent, struct lu_error **error)
cfe9be
 {
cfe9be
-	lu_security_context_t fscreate;
cfe9be
-	char *filename, *new_line, *contents, *line, *rest;
cfe9be
+	struct editing *e;
cfe9be
+	char *new_line, *contents, *line, *rest;
cfe9be
 	char *current_name, *fragment;
cfe9be
-	int fd;
cfe9be
-	gpointer lock;
cfe9be
 	const char *name_attribute;
cfe9be
 	gboolean ret = FALSE;
cfe9be
 	struct stat st;
cfe9be
@@ -971,43 +1175,24 @@ generic_mod(struct lu_module *module, co
cfe9be
 		return FALSE;
cfe9be
 	}
cfe9be
 
cfe9be
-	filename = module_filename(module, file_suffix);
cfe9be
-
cfe9be
 	new_line = format_generic(ent, formats, format_count, error);
cfe9be
 	if (new_line == NULL)
cfe9be
-		goto err_filename;
cfe9be
+		goto err_current_name;
cfe9be
 
cfe9be
-	if (!lu_util_fscreate_save(&fscreate, error))
cfe9be
+	e = editing_open(module, file_suffix, error);
cfe9be
+	if (e == NULL)
cfe9be
 		goto err_new_line;
cfe9be
-	if (!lu_util_fscreate_from_file(filename, error))
cfe9be
-		goto err_fscreate;
cfe9be
-	/* Create a backup file. */
cfe9be
-	if (lu_files_create_backup(filename, error) == FALSE)
cfe9be
-		goto err_fscreate;
cfe9be
 
cfe9be
-	/* Open the file to be modified. */
cfe9be
-	fd = open(filename, O_RDWR);
cfe9be
-	if (fd == -1) {
cfe9be
-		lu_error_new(error, lu_error_open,
cfe9be
-			     _("couldn't open `%s': %s"), filename,
cfe9be
-			     strerror(errno));
cfe9be
-		goto err_fscreate;
cfe9be
-	}
cfe9be
-
cfe9be
-	/* Lock the file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
cfe9be
-		goto err_fd;
cfe9be
-
cfe9be
-	if (fstat(fd, &st) == -1) {
cfe9be
+	if (fstat(e->new_fd, &st) == -1) {
cfe9be
 		lu_error_new(error, lu_error_stat, _("couldn't stat `%s': %s"),
cfe9be
-			     filename, strerror(errno));
cfe9be
-		goto err_lock;
cfe9be
+			     e->new_filename, strerror(errno));
cfe9be
+		goto err_editing;
cfe9be
 	}
cfe9be
 
cfe9be
 	contents = g_malloc(st.st_size + 1 + strlen(new_line));
cfe9be
-	if (read(fd, contents, st.st_size) != st.st_size) {
cfe9be
+	if (read(e->new_fd, contents, st.st_size) != st.st_size) {
cfe9be
 		lu_error_new(error, lu_error_read,
cfe9be
-			     _("couldn't read from `%s': %s"), filename,
cfe9be
+			     _("couldn't read from `%s': %s"), e->new_filename,
cfe9be
 			     strerror(errno));
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
@@ -1045,16 +1230,16 @@ generic_mod(struct lu_module *module, co
cfe9be
 	memmove(line + strlen(new_line), rest,
cfe9be
 		contents + st.st_size + 1 - rest);
cfe9be
 	memcpy(line, new_line, strlen(new_line));
cfe9be
-	if (lseek(fd, line - contents, SEEK_SET) == -1) {
cfe9be
+	if (lseek(e->new_fd, line - contents, SEEK_SET) == -1) {
cfe9be
 		lu_error_new(error, lu_error_write, NULL);
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
 	len = strlen(line);
cfe9be
-	if ((size_t)write(fd, line, len) != len) {
cfe9be
+	if ((size_t)write(e->new_fd, line, len) != len) {
cfe9be
 		lu_error_new(error, lu_error_write, NULL);
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
-	if (ftruncate(fd, (line - contents) + len) != 0) {
cfe9be
+	if (ftruncate(e->new_fd, (line - contents) + len) != 0) {
cfe9be
 		lu_error_new(error, lu_error_write, NULL);
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
@@ -1063,16 +1248,11 @@ generic_mod(struct lu_module *module, co
cfe9be
 
cfe9be
 err_contents:
cfe9be
 	g_free(contents);
cfe9be
-err_lock:
cfe9be
-	lu_util_lock_free(lock);
cfe9be
-err_fd:
cfe9be
-	close(fd);
cfe9be
-err_fscreate:
cfe9be
-	lu_util_fscreate_restore(fscreate);
cfe9be
+err_editing:
cfe9be
+	ret = editing_close(e, ret, ret, error); /* Commit/rollback happens here. */
cfe9be
 err_new_line:
cfe9be
 	g_free(new_line);
cfe9be
-err_filename:
cfe9be
-	g_free(filename);
cfe9be
+err_current_name:
cfe9be
 	g_free(current_name);
cfe9be
 	return ret;
cfe9be
 }
cfe9be
@@ -1118,16 +1298,14 @@ static gboolean
cfe9be
 generic_del(struct lu_module *module, const char *file_suffix,
cfe9be
 	    struct lu_ent *ent, struct lu_error **error)
cfe9be
 {
cfe9be
-	lu_security_context_t fscreate;
cfe9be
+	struct editing *e;
cfe9be
 	char *name;
cfe9be
-	char *contents, *filename;
cfe9be
+	char *contents;
cfe9be
 	char *fragment2;
cfe9be
 	struct stat st;
cfe9be
 	size_t len;
cfe9be
-	int fd;
cfe9be
-        gboolean ret = FALSE;
cfe9be
+        gboolean commit = FALSE, ret = FALSE;
cfe9be
 	gboolean found;
cfe9be
-	gpointer lock;
cfe9be
 
cfe9be
 	/* Get the entity's current name. */
cfe9be
 	if (ent->type == lu_user)
cfe9be
@@ -1141,42 +1319,23 @@ generic_del(struct lu_module *module, co
cfe9be
 	g_assert(module != NULL);
cfe9be
 	g_assert(ent != NULL);
cfe9be
 
cfe9be
-	filename = module_filename(module, file_suffix);
cfe9be
-
cfe9be
-	if (!lu_util_fscreate_save(&fscreate, error))
cfe9be
-		goto err_filename;
cfe9be
-	if (!lu_util_fscreate_from_file(filename, error))
cfe9be
-		goto err_fscreate;
cfe9be
-	/* Create a backup of that file. */
cfe9be
-	if (lu_files_create_backup(filename, error) == FALSE)
cfe9be
-		goto err_fscreate;
cfe9be
-
cfe9be
-	/* Open the file to be modified. */
cfe9be
-	fd = open(filename, O_RDWR);
cfe9be
-	if (fd == -1) {
cfe9be
-		lu_error_new(error, lu_error_open,
cfe9be
-			     _("couldn't open `%s': %s"), filename,
cfe9be
-			     strerror(errno));
cfe9be
-		goto err_fscreate;
cfe9be
-	}
cfe9be
-
cfe9be
-	/* Lock the file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
cfe9be
-		goto err_fd;
cfe9be
+	e = editing_open(module, file_suffix, error);
cfe9be
+	if (e == NULL)
cfe9be
+		goto err_name;
cfe9be
 
cfe9be
 	/* Determine the file's size. */
cfe9be
-	if (fstat(fd, &st) == -1) {
cfe9be
+	if (fstat(e->new_fd, &st) == -1) {
cfe9be
 		lu_error_new(error, lu_error_stat,
cfe9be
-			     _("couldn't stat `%s': %s"), filename,
cfe9be
+			     _("couldn't stat `%s': %s"), e->new_filename,
cfe9be
 			     strerror(errno));
cfe9be
-		goto err_lock;
cfe9be
+		goto err_editing;
cfe9be
 	}
cfe9be
 
cfe9be
 	/* Allocate space to hold the file and read it all in. */
cfe9be
 	contents = g_malloc(st.st_size + 1);
cfe9be
-	if (read(fd, contents, st.st_size) != st.st_size) {
cfe9be
+	if (read(e->new_fd, contents, st.st_size) != st.st_size) {
cfe9be
 		lu_error_new(error, lu_error_read,
cfe9be
-			     _("couldn't read from `%s': %s"), filename,
cfe9be
+			     _("couldn't read from `%s': %s"), e->new_filename,
cfe9be
 			     strerror(errno));
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
@@ -1229,41 +1388,38 @@ generic_del(struct lu_module *module, co
cfe9be
 
cfe9be
 	/* Otherwise we need to write the new data to the file.  Jump back to
cfe9be
 	 * the beginning of the file. */
cfe9be
-	if (lseek(fd, 0, SEEK_SET) == -1) {
cfe9be
+	if (lseek(e->new_fd, 0, SEEK_SET) == -1) {
cfe9be
 		lu_error_new(error, lu_error_write,
cfe9be
-			     _("couldn't write to `%s': %s"), filename,
cfe9be
+			     _("couldn't write to `%s': %s"), e->new_filename,
cfe9be
 			     strerror(errno));
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
 
cfe9be
 	/* Write the new contents out. */
cfe9be
-	if ((size_t)write(fd, contents, len) != len) {
cfe9be
+	if ((size_t)write(e->new_fd, contents, len) != len) {
cfe9be
 		lu_error_new(error, lu_error_write,
cfe9be
-			     _("couldn't write to `%s': %s"), filename,
cfe9be
+			     _("couldn't write to `%s': %s"), e->new_filename,
cfe9be
 			     strerror(errno));
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
 
cfe9be
 	/* Truncate the file to the new (certainly shorter) length. */
cfe9be
-	if (ftruncate(fd, len) == -1) {
cfe9be
+	if (ftruncate(e->new_fd, len) == -1) {
cfe9be
 		lu_error_new(error, lu_error_generic,
cfe9be
-			     _("couldn't write to `%s': %s"), filename,
cfe9be
+			     _("couldn't write to `%s': %s"), e->new_filename,
cfe9be
 			     strerror(errno));
cfe9be
 		goto err_contents;
cfe9be
 	}
cfe9be
+	commit = TRUE;
cfe9be
 	ret = TRUE;
cfe9be
 	/* Fall through */
cfe9be
 
cfe9be
  err_contents:
cfe9be
 	g_free(contents);
cfe9be
- err_lock:
cfe9be
-	lu_util_lock_free(lock);
cfe9be
- err_fd:
cfe9be
-	close(fd);
cfe9be
-err_fscreate:
cfe9be
-	lu_util_fscreate_restore(fscreate);
cfe9be
- err_filename:
cfe9be
-	g_free(filename);
cfe9be
+err_editing:
cfe9be
+	/* Commit/rollback happens here. */
cfe9be
+	ret = editing_close(e, commit, ret, error);
cfe9be
+err_name:
cfe9be
 	g_free(name);
cfe9be
 	return ret;
cfe9be
 }
cfe9be
@@ -1344,12 +1500,9 @@ static gboolean
cfe9be
 generic_lock(struct lu_module *module, const char *file_suffix, int field,
cfe9be
 	     struct lu_ent *ent, enum lock_op op, struct lu_error **error)
cfe9be
 {
cfe9be
-	lu_security_context_t fscreate;
cfe9be
-	char *filename;
cfe9be
+	struct editing *e;
cfe9be
 	char *value, *new_value, *name;
cfe9be
-	int fd;
cfe9be
-	gpointer lock;
cfe9be
-	gboolean ret = FALSE;
cfe9be
+	gboolean commit = FALSE, ret = FALSE;
cfe9be
 
cfe9be
 	/* Get the name which keys the entries of interest in the file. */
cfe9be
 	g_assert((ent->type == lu_user) || (ent->type == lu_group));
cfe9be
@@ -1362,33 +1515,14 @@ generic_lock(struct lu_module *module, c
cfe9be
 	g_assert(module != NULL);
cfe9be
 	g_assert(ent != NULL);
cfe9be
 
cfe9be
-	filename = module_filename(module, file_suffix);
cfe9be
-
cfe9be
-	if (!lu_util_fscreate_save(&fscreate, error))
cfe9be
-		goto err_filename;
cfe9be
-	if (!lu_util_fscreate_from_file(filename, error))
cfe9be
-		goto err_fscreate;
cfe9be
-	/* Create a backup of the file. */
cfe9be
-	if (lu_files_create_backup(filename, error) == FALSE)
cfe9be
-		goto err_fscreate;
cfe9be
-
cfe9be
-	/* Open the file. */
cfe9be
-	fd = open(filename, O_RDWR);
cfe9be
-	if (fd == -1) {
cfe9be
-		lu_error_new(error, lu_error_open,
cfe9be
-			     _("couldn't open `%s': %s"), filename,
cfe9be
-			     strerror(errno));
cfe9be
-		goto err_fscreate;
cfe9be
-	}
cfe9be
-
cfe9be
-	/* Lock the file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
cfe9be
-		goto err_fd;
cfe9be
+	e = editing_open(module, file_suffix, error);
cfe9be
+	if (e == NULL)
cfe9be
+		goto err_name;
cfe9be
 
cfe9be
 	/* Read the old value from the file. */
cfe9be
-	value = lu_util_field_read(fd, name, field, error);
cfe9be
+	value = lu_util_field_read(e->new_fd, name, field, error);
cfe9be
 	if (value == NULL)
cfe9be
-		goto err_lock;
cfe9be
+		goto err_editing;
cfe9be
 
cfe9be
 	/* Check that we actually care about this.  If there's a non-empty,
cfe9be
 	 * not locked string in there, but it's too short to be a hash, then
cfe9be
@@ -1396,27 +1530,27 @@ generic_lock(struct lu_module *module, c
cfe9be
 	if (LU_CRYPT_INVALID(value)) {
cfe9be
 		g_free(value);
cfe9be
 		ret = TRUE;
cfe9be
-		goto err_lock;
cfe9be
+		goto err_editing;
cfe9be
 	}
cfe9be
 
cfe9be
 	/* Generate a new value for the file. */
cfe9be
 	new_value = lock_process(value, op, ent, error);
cfe9be
 	g_free(value);
cfe9be
 	if (new_value == NULL)
cfe9be
-		goto err_lock;
cfe9be
+		goto err_editing;
cfe9be
 
cfe9be
 	/* Make the change. */
cfe9be
-	ret = lu_util_field_write(fd, name, field, new_value, error);
cfe9be
+	if (lu_util_field_write(e->new_fd, name, field, new_value, error)
cfe9be
+	    == FALSE)
cfe9be
+		goto err_editing;
cfe9be
+	commit = TRUE;
cfe9be
+	ret = TRUE;
cfe9be
 	/* Fall through */
cfe9be
 
cfe9be
-err_lock:
cfe9be
-	lu_util_lock_free(lock);
cfe9be
- err_fd:
cfe9be
-	close(fd);
cfe9be
-err_fscreate:
cfe9be
-	lu_util_fscreate_restore(fscreate);
cfe9be
- err_filename:
cfe9be
-	g_free(filename);
cfe9be
+err_editing:
cfe9be
+	/* Commit/rollback happens here. */
cfe9be
+	ret = editing_close(e, commit, ret, error);
cfe9be
+err_name:
cfe9be
 	g_free(name);
cfe9be
 	return ret;
cfe9be
 }
cfe9be
@@ -1429,7 +1563,6 @@ generic_is_locked(struct lu_module *modu
cfe9be
 	char *filename;
cfe9be
 	char *value, *name;
cfe9be
 	int fd;
cfe9be
-	gpointer lock;
cfe9be
 	gboolean ret = FALSE;
cfe9be
 
cfe9be
 	/* Get the name of this account. */
cfe9be
@@ -1454,22 +1587,16 @@ generic_is_locked(struct lu_module *modu
cfe9be
 		goto err_filename;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Lock the file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
cfe9be
-		goto err_fd;
cfe9be
-
cfe9be
 	/* Read the value. */
cfe9be
 	value = lu_util_field_read(fd, name, field, error);
cfe9be
 	if (value == NULL)
cfe9be
-		goto err_lock;
cfe9be
+		goto err_fd;
cfe9be
 
cfe9be
 	/* It all comes down to this. */
cfe9be
 	ret = value[0] == '!';
cfe9be
 	g_free(value);
cfe9be
 	/* Fall through */
cfe9be
 
cfe9be
-err_lock:
cfe9be
-	lu_util_lock_free(lock);
cfe9be
 err_fd:
cfe9be
 	close(fd);
cfe9be
 err_filename:
cfe9be
@@ -1624,10 +1751,8 @@ generic_setpass(struct lu_module *module
cfe9be
 		struct lu_ent *ent, const char *password, gboolean is_shadow,
cfe9be
 		struct lu_error **error)
cfe9be
 {
cfe9be
-	lu_security_context_t fscreate;
cfe9be
-	char *filename, *value, *name;
cfe9be
-	int fd;
cfe9be
-	gpointer lock;
cfe9be
+	struct editing *e;
cfe9be
+	char *value, *name;
cfe9be
 	gboolean ret = FALSE;
cfe9be
 
cfe9be
 	/* Get the name of this account. */
cfe9be
@@ -1641,34 +1766,14 @@ generic_setpass(struct lu_module *module
cfe9be
 	g_assert(module != NULL);
cfe9be
 	g_assert(ent != NULL);
cfe9be
 
cfe9be
-	filename = module_filename(module, file_suffix);
cfe9be
-
cfe9be
-	if (!lu_util_fscreate_save(&fscreate, error))
cfe9be
-		goto err_filename;
cfe9be
-	if (!lu_util_fscreate_from_file(filename, error))
cfe9be
-		goto err_fscreate;
cfe9be
-
cfe9be
-	/* Create a backup of the file. */
cfe9be
-	if (lu_files_create_backup(filename, error) == FALSE)
cfe9be
-		goto err_filename;
cfe9be
-
cfe9be
-	/* Open the file. */
cfe9be
-	fd = open(filename, O_RDWR);
cfe9be
-	if (fd == -1) {
cfe9be
-		lu_error_new(error, lu_error_open,
cfe9be
-			     _("couldn't open `%s': %s"), filename,
cfe9be
-			     strerror(errno));
cfe9be
-		goto err_fscreate;
cfe9be
-	}
cfe9be
-
cfe9be
-	/* Lock the file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL)
cfe9be
-		goto err_fd;
cfe9be
+	e = editing_open(module, file_suffix, error);
cfe9be
+	if (e == NULL)
cfe9be
+		goto err_name;
cfe9be
 
cfe9be
 	/* Read the current contents of the field. */
cfe9be
-	value = lu_util_field_read(fd, name, field, error);
cfe9be
+	value = lu_util_field_read(e->new_fd, name, field, error);
cfe9be
 	if (value == NULL)
cfe9be
-		goto err_lock;
cfe9be
+		goto err_editing;
cfe9be
 
cfe9be
 	/* pam_unix uses shadow passwords only if pw_passwd is "x"
cfe9be
 	   (or ##${username}).  Make sure to preserve the shadow marker
cfe9be
@@ -1693,9 +1798,9 @@ generic_setpass(struct lu_module *module
cfe9be
 	else if (g_ascii_strncasecmp(password, LU_CRYPTED, strlen(LU_CRYPTED))
cfe9be
 		 == 0) {
cfe9be
 		password = password + strlen(LU_CRYPTED);
cfe9be
-		if (strchr(password, ':') != NULL) {
cfe9be
+		if (strpbrk(password, ":\n") != NULL) {
cfe9be
 			lu_error_new(error, lu_error_invalid_attribute_value,
cfe9be
-				     _("`:' not allowed in encrypted "
cfe9be
+				     _("`:' and `\\n' not allowed in encrypted "
cfe9be
 				       "password"));
cfe9be
 			goto err_value;
cfe9be
 		}
cfe9be
@@ -1713,19 +1818,14 @@ generic_setpass(struct lu_module *module
cfe9be
 	}
cfe9be
 
cfe9be
 	/* Now write our changes to the file. */
cfe9be
-	ret = lu_util_field_write(fd, name, field, password, error);
cfe9be
+	ret = lu_util_field_write(e->new_fd, name, field, password, error);
cfe9be
 	/* Fall through */
cfe9be
 
cfe9be
- err_value:
cfe9be
+err_value:
cfe9be
 	g_free(value);
cfe9be
-err_lock:
cfe9be
-	lu_util_lock_free(lock);
cfe9be
- err_fd:
cfe9be
-	close(fd);
cfe9be
-err_fscreate:
cfe9be
-	lu_util_fscreate_restore(fscreate);
cfe9be
- err_filename:
cfe9be
-	g_free(filename);
cfe9be
+err_editing:
cfe9be
+	ret = editing_close(e, ret, ret, error); /* Commit/rollback happens here. */
cfe9be
+err_name:
cfe9be
 	g_free(name);
cfe9be
 	return ret;
cfe9be
 }
cfe9be
@@ -1802,7 +1902,6 @@ lu_files_enumerate(struct lu_module *mod
cfe9be
 		   const char *pattern, struct lu_error **error)
cfe9be
 {
cfe9be
 	int fd;
cfe9be
-	gpointer lock;
cfe9be
 	GValueArray *ret;
cfe9be
 	GValue value;
cfe9be
 	char *buf;
cfe9be
@@ -1824,20 +1923,12 @@ lu_files_enumerate(struct lu_module *mod
cfe9be
 		return NULL;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Lock the file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
cfe9be
-		close(fd);
cfe9be
-		g_free(filename);
cfe9be
-		return NULL;
cfe9be
-	}
cfe9be
-
cfe9be
 	/* Wrap the file for stdio operations. */
cfe9be
 	fp = fdopen(fd, "r");
cfe9be
 	if (fp == NULL) {
cfe9be
 		lu_error_new(error, lu_error_open,
cfe9be
 			     _("couldn't open `%s': %s"), filename,
cfe9be
 			     strerror(errno));
cfe9be
-		lu_util_lock_free(lock);
cfe9be
 		close(fd);
cfe9be
 		g_free(filename);
cfe9be
 		return NULL;
cfe9be
@@ -1873,7 +1964,6 @@ lu_files_enumerate(struct lu_module *mod
cfe9be
 
cfe9be
 	/* Clean up. */
cfe9be
 	g_value_unset(&value);
cfe9be
-	lu_util_lock_free(lock);
cfe9be
 	fclose(fp);
cfe9be
 	g_free(filename);
cfe9be
 
cfe9be
@@ -1902,7 +1992,6 @@ lu_files_users_enumerate_by_group(struct
cfe9be
 				  struct lu_error **error)
cfe9be
 {
cfe9be
 	int fd;
cfe9be
-	gpointer lock;
cfe9be
 	GValueArray *ret;
cfe9be
 	GValue value;
cfe9be
 	char *buf, grp[CHUNK_SIZE];
cfe9be
@@ -1927,21 +2016,12 @@ lu_files_users_enumerate_by_group(struct
cfe9be
 		return NULL;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Lock the passwd file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
cfe9be
-		close(fd);
cfe9be
-		g_free(pwdfilename);
cfe9be
-		g_free(grpfilename);
cfe9be
-		return NULL;
cfe9be
-	}
cfe9be
-
cfe9be
 	/* Wrap the descriptor in a stdio FILE. */
cfe9be
 	fp = fdopen(fd, "r");
cfe9be
 	if (fp == NULL) {
cfe9be
 		lu_error_new(error, lu_error_open,
cfe9be
 			     _("couldn't open `%s': %s"), pwdfilename,
cfe9be
 			     strerror(errno));
cfe9be
-		lu_util_lock_free(lock);
cfe9be
 		close(fd);
cfe9be
 		g_free(pwdfilename);
cfe9be
 		g_free(grpfilename);
cfe9be
@@ -2000,7 +2080,6 @@ lu_files_users_enumerate_by_group(struct
cfe9be
 	}
cfe9be
 	/* Close the file. */
cfe9be
 	g_value_unset(&value);
cfe9be
-	lu_util_lock_free(lock);
cfe9be
 	fclose(fp);
cfe9be
 
cfe9be
 	/* Open the group file. */
cfe9be
@@ -2015,22 +2094,12 @@ lu_files_users_enumerate_by_group(struct
cfe9be
 		return NULL;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Lock the group file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
cfe9be
-		close(fd);
cfe9be
-		g_free(pwdfilename);
cfe9be
-		g_free(grpfilename);
cfe9be
-		g_value_array_free(ret);
cfe9be
-		return NULL;
cfe9be
-	}
cfe9be
-
cfe9be
 	/* Wrap the group file in an stdio file. */
cfe9be
 	fp = fdopen(fd, "r");
cfe9be
 	if (fp == NULL) {
cfe9be
 		lu_error_new(error, lu_error_open,
cfe9be
 			     _("couldn't open `%s': %s"), grpfilename,
cfe9be
 			     strerror(errno));
cfe9be
-		lu_util_lock_free(lock);
cfe9be
 		close(fd);
cfe9be
 		g_free(pwdfilename);
cfe9be
 		g_free(grpfilename);
cfe9be
@@ -2085,7 +2154,6 @@ lu_files_users_enumerate_by_group(struct
cfe9be
 	}
cfe9be
 
cfe9be
 	/* Clean up. */
cfe9be
-	lu_util_lock_free(lock);
cfe9be
 	fclose(fp);
cfe9be
 
cfe9be
 	g_free(pwdfilename);
cfe9be
@@ -2102,7 +2170,6 @@ lu_files_groups_enumerate_by_user(struct
cfe9be
 				  struct lu_error **error)
cfe9be
 {
cfe9be
 	int fd;
cfe9be
-	gpointer lock;
cfe9be
 	GValueArray *ret;
cfe9be
 	GValue value;
cfe9be
 	char *buf;
cfe9be
@@ -2126,19 +2193,12 @@ lu_files_groups_enumerate_by_user(struct
cfe9be
 		goto err_pwdfilename;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Lock it. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
cfe9be
-		close(fd);
cfe9be
-		goto err_pwdfilename;
cfe9be
-	}
cfe9be
-
cfe9be
 	/* Open it so that we can use stdio. */
cfe9be
 	fp = fdopen(fd, "r");
cfe9be
 	if (fp == NULL) {
cfe9be
 		lu_error_new(error, lu_error_open,
cfe9be
 			     _("couldn't open `%s': %s"), pwdfilename,
cfe9be
 			     strerror(errno));
cfe9be
-		lu_util_lock_free(lock);
cfe9be
 		close(fd);
cfe9be
 		goto err_pwdfilename;
cfe9be
 	}
cfe9be
@@ -2186,7 +2246,6 @@ lu_files_groups_enumerate_by_user(struct
cfe9be
 		}
cfe9be
 		g_free(buf);
cfe9be
 	}
cfe9be
-	lu_util_lock_free(lock);
cfe9be
 	fclose(fp);
cfe9be
 
cfe9be
 	/* Open the groups file. */
cfe9be
@@ -2198,19 +2257,12 @@ lu_files_groups_enumerate_by_user(struct
cfe9be
 		goto err_key;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Lock it. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
cfe9be
-		close(fd);
cfe9be
-		goto err_key;
cfe9be
-	}
cfe9be
-
cfe9be
 	/* Open it so that we can use stdio. */
cfe9be
 	fp = fdopen(fd, "r");
cfe9be
 	if (fp == NULL) {
cfe9be
 		lu_error_new(error, lu_error_open,
cfe9be
 			     _("couldn't open `%s': %s"), grpfilename,
cfe9be
 			     strerror(errno));
cfe9be
-		lu_util_lock_free(lock);
cfe9be
 		close(fd);
cfe9be
 		goto err_key;
cfe9be
 	}
cfe9be
@@ -2267,7 +2319,6 @@ lu_files_groups_enumerate_by_user(struct
cfe9be
 	g_free(key);
cfe9be
 	g_value_unset(&value);
cfe9be
 
cfe9be
-	lu_util_lock_free(lock);
cfe9be
 	fclose(fp);
cfe9be
 	g_free(pwdfilename);
cfe9be
 	g_free(grpfilename);
cfe9be
@@ -2291,7 +2342,6 @@ lu_files_enumerate_full(struct lu_module
cfe9be
 			struct lu_error **error)
cfe9be
 {
cfe9be
 	int fd;
cfe9be
-	gpointer lock;
cfe9be
 	GPtrArray *ret = NULL;
cfe9be
 	char *buf;
cfe9be
 	char *key, *filename;
cfe9be
@@ -2311,19 +2361,12 @@ lu_files_enumerate_full(struct lu_module
cfe9be
 		goto err_filename;
cfe9be
 	}
cfe9be
 
cfe9be
-	/* Lock the file. */
cfe9be
-	if ((lock = lu_util_lock_obtain(fd, error)) == NULL) {
cfe9be
-		close(fd);
cfe9be
-		goto err_filename;
cfe9be
-	}
cfe9be
-
cfe9be
 	/* Wrap the file up in stdio. */
cfe9be
 	fp = fdopen(fd, "r");
cfe9be
 	if (fp == NULL) {
cfe9be
 		lu_error_new(error, lu_error_open,
cfe9be
 			     _("couldn't open `%s': %s"), filename,
cfe9be
 			     strerror(errno));
cfe9be
-		lu_util_lock_free(lock);
cfe9be
 		close(fd);
cfe9be
 		goto err_filename;
cfe9be
 	}
cfe9be
@@ -2358,7 +2401,6 @@ lu_files_enumerate_full(struct lu_module
cfe9be
 		g_free(key);
cfe9be
 	}
cfe9be
 
cfe9be
-	lu_util_lock_free(lock);
cfe9be
 	fclose(fp);
cfe9be
 
cfe9be
  err_filename:
cfe9be
diff -up libuser-0.60/tests/files_test.py.CVE-2015-3246 libuser-0.60/tests/files_test.py
cfe9be
--- libuser-0.60/tests/files_test.py.CVE-2015-3246	2013-10-12 23:56:08.000000000 +0200
cfe9be
+++ libuser-0.60/tests/files_test.py	2015-07-08 15:15:14.061544074 +0200
cfe9be
@@ -262,6 +262,21 @@ class Tests(unittest.TestCase):
cfe9be
         e = self.a.initUser('user6_8')
cfe9be
         self.assertRaises(RuntimeError, self.a.addUser, e, False, False)
cfe9be
 
cfe9be
+    def testUserAdd9(self):
cfe9be
+        # '\n' in field values
cfe9be
+        # libuser.USERPASSWORD not tested because lu_shadow_user_add_prep()
cfe9be
+        # always replaces it by 'x'.  libuser.UIDNUMBER not tested because
cfe9be
+        # ent_has_name_and_id() interprets the value as a number.
cfe9be
+        for field in (libuser.USERNAME, libuser.GIDNUMBER, libuser.GECOS,
cfe9be
+                      libuser.HOMEDIRECTORY, libuser.LOGINSHELL,
cfe9be
+                      libuser.SHADOWPASSWORD, libuser.SHADOWLASTCHANGE, 
cfe9be
+                      libuser.SHADOWMIN, libuser.SHADOWMAX, 
cfe9be
+                      libuser.SHADOWWARNING, libuser.SHADOWINACTIVE, 
cfe9be
+                      libuser.SHADOWEXPIRE, libuser.SHADOWFLAG):
cfe9be
+            e = self.a.initUser('user_6_9' + field)
cfe9be
+            e[field] = str(e[field][0]) + '\nx'
cfe9be
+            self.assertRaises(RuntimeError, self.a.addUser, e, False, False)
cfe9be
+
cfe9be
     def testUserMod1(self):
cfe9be
         # A minimal case
cfe9be
         e = self.a.initUser('user7_1')
cfe9be
@@ -421,6 +436,26 @@ class Tests(unittest.TestCase):
cfe9be
         e[libuser.USERNAME] = 'user7_7'
cfe9be
         self.assertRaises(RuntimeError, self.a.modifyUser, e, False)
cfe9be
 
cfe9be
+    def testUserMod8(self):
cfe9be
+        # '\n' in field values
cfe9be
+        # libuser.USERPASSWORD not tested because lu_shadow_user_add_prep()
cfe9be
+        # always replaces it by 'x'.  libuser.UIDNUMBER not tested because
cfe9be
+        # ent_has_name_and_id() interprets the value as a number.
cfe9be
+        for field in (libuser.USERNAME, libuser.USERPASSWORD, libuser.GIDNUMBER,
cfe9be
+                      libuser.GECOS, libuser.HOMEDIRECTORY, libuser.LOGINSHELL,
cfe9be
+                      libuser.SHADOWPASSWORD, libuser.SHADOWLASTCHANGE,
cfe9be
+                      libuser.SHADOWMIN, libuser.SHADOWMAX,
cfe9be
+                      libuser.SHADOWWARNING, libuser.SHADOWINACTIVE,
cfe9be
+                      libuser.SHADOWEXPIRE, libuser.SHADOWFLAG):
cfe9be
+            e = self.a.initUser('user7_9' + field)
cfe9be
+            self.a.addUser(e, False, False)
cfe9be
+            del e
cfe9be
+            e = self.a.lookupUserByName('user7_9' + field)
cfe9be
+            self.assertIsNotNone(e)
cfe9be
+            self.assertNotIn('\n', str(e[field][0]))
cfe9be
+            e[field] = str(e[field][0]) + '\nx'
cfe9be
+            self.assertRaises(RuntimeError, self.a.modifyUser, e, False)
cfe9be
+
cfe9be
     def testUserDel(self):
cfe9be
         e = self.a.initUser('user8_1')
cfe9be
         self.a.addUser(e, False, False)
cfe9be
@@ -619,6 +654,22 @@ class Tests(unittest.TestCase):
cfe9be
         crypted = crypt.crypt('a:b', e[libuser.SHADOWPASSWORD][0])
cfe9be
         self.assertEqual(e[libuser.SHADOWPASSWORD], [crypted])
cfe9be
 
cfe9be
+    def testUserSetpass5(self):
cfe9be
+        # '\n' in field value
cfe9be
+        e = self.a.initUser('user12_5')
cfe9be
+        self.a.addUser(e, False, False)
cfe9be
+        del e
cfe9be
+        e = self.a.lookupUserByName('user12_5')
cfe9be
+        self.assertNotIn('\n', e[libuser.SHADOWPASSWORD][0])
cfe9be
+        self.assertRaises(SystemError, self.a.setpassUser, e, 'a\nb', True)
cfe9be
+        self.a.setpassUser(e, 'a\nb', False)
cfe9be
+        del e
cfe9be
+        e = self.a.lookupUserByName('user12_5')
cfe9be
+        self.assertEqual(e[libuser.SHADOWPASSWORD][0][:3], '$1$')
cfe9be
+        self.assertNotIn('\n', e[libuser.SHADOWPASSWORD][0])
cfe9be
+        crypted = crypt.crypt('a\nb', e[libuser.SHADOWPASSWORD][0])
cfe9be
+        self.assertEqual(e[libuser.SHADOWPASSWORD], [crypted])
cfe9be
+
cfe9be
     def testUserRemovepass(self):
cfe9be
         e = self.a.initUser('user13_1')
cfe9be
         e[libuser.SHADOWPASSWORD] = '03dgZm5nZvqOc'
cfe9be
@@ -884,6 +935,20 @@ class Tests(unittest.TestCase):
cfe9be
         e = self.a.initGroup('group21_5')
cfe9be
         self.assertRaises(RuntimeError, self.a.addGroup, e)
cfe9be
 
cfe9be
+    def testGroupAdd6(self):
cfe9be
+        # '\n' in field values
cfe9be
+        # libuser.GROUPPASSWORD not tested because lu_shadow_group_add_prep()
cfe9be
+        # always replaces it by 'x'.  libuser.GIDNUMBER not tested because
cfe9be
+        # ent_has_name_and_id() interprets the value as a number.
cfe9be
+        for field in (libuser.GROUPNAME, libuser.SHADOWPASSWORD,
cfe9be
+                      libuser.MEMBERNAME, libuser.ADMINISTRATORNAME):
cfe9be
+            e = self.a.initGroup('group_21_6' + field)
cfe9be
+            if e.has_key(field):
cfe9be
+                e[field] = str(e[field][0]) + '\nx'
cfe9be
+            else:
cfe9be
+                e[field] = field + '\nx'
cfe9be
+            self.assertRaises(RuntimeError, self.a.addGroup, e)
cfe9be
+
cfe9be
     def testGroupMod1(self):
cfe9be
         # A minimal case
cfe9be
         e = self.a.initGroup('group22_1')
cfe9be
@@ -997,6 +1062,25 @@ class Tests(unittest.TestCase):
cfe9be
         e[libuser.GROUPNAME] = 'group22_6'
cfe9be
         self.assertRaises(RuntimeError, self.a.modifyGroup, e)
cfe9be
 
cfe9be
+    def testGroupMod7(self):
cfe9be
+        # '\n' in field values
cfe9be
+        # libuser.GIDNUMBER not tested because ent_has_name_and_id() interprets
cfe9be
+        # the value as a number.
cfe9be
+        for field in (libuser.GROUPNAME, libuser.GROUPPASSWORD,
cfe9be
+                      libuser.SHADOWPASSWORD, libuser.MEMBERNAME,
cfe9be
+                      libuser.ADMINISTRATORNAME):
cfe9be
+            e = self.a.initGroup('group22_8' + field)
cfe9be
+            self.a.addGroup(e)
cfe9be
+            del e
cfe9be
+            e = self.a.lookupGroupByName('group22_8' + field)
cfe9be
+            self.assertIsNotNone(e)
cfe9be
+            if e.has_key(field):
cfe9be
+                self.assertNotIn('\n', str(e[field][0]))
cfe9be
+                e[field] = str(e[field][0]) + '\nx'
cfe9be
+            else:
cfe9be
+                e[field] = field + '\nx'
cfe9be
+            self.assertRaises(RuntimeError, self.a.modifyGroup, e)
cfe9be
+
cfe9be
     def testGroupDel(self):
cfe9be
         e = self.a.initGroup('group23_1')
cfe9be
         self.a.addGroup(e)
cfe9be
@@ -1190,6 +1274,22 @@ class Tests(unittest.TestCase):
cfe9be
         crypted = crypt.crypt('a:b', e[libuser.SHADOWPASSWORD][0])
cfe9be
         self.assertEqual(e[libuser.SHADOWPASSWORD], [crypted])
cfe9be
 
cfe9be
+    def testGroupSetpass4(self):
cfe9be
+        # '\n' in field value
cfe9be
+        e = self.a.initGroup('group27_5')
cfe9be
+        self.a.addGroup(e)
cfe9be
+        del e
cfe9be
+        e = self.a.lookupGroupByName('group27_5')
cfe9be
+        self.assertNotIn('\n', e[libuser.SHADOWPASSWORD][0])
cfe9be
+        self.assertRaises(SystemError, self.a.setpassGroup, e, 'a\nb', True)
cfe9be
+        self.a.setpassGroup(e, 'a\nb', False)
cfe9be
+        del e
cfe9be
+        e = self.a.lookupGroupByName('group27_5')
cfe9be
+        self.assertEqual(e[libuser.SHADOWPASSWORD][0][:3], '$1$')
cfe9be
+        self.assertNotIn('\n', e[libuser.SHADOWPASSWORD][0])
cfe9be
+        crypted = crypt.crypt('a\nb', e[libuser.SHADOWPASSWORD][0])
cfe9be
+        self.assertEqual(e[libuser.SHADOWPASSWORD], [crypted])
cfe9be
+
cfe9be
     def testGroupRemovepass(self):
cfe9be
         e = self.a.initGroup('group28_1')
cfe9be
         e[libuser.SHADOWPASSWORD] = '07Js7N.eEhbgs'