Re: Unfortunate results from fake-super

[Dave Gordon via rsync]

On 05/02/18 23:03, Dave Gordon via rsync wrote:
> On 05/02/18 05:53, Wayne Davison wrote:
>> On Sat, Feb 3, 2018 at 5:20 AM, Dave Gordon via rsync
>> > wrote:
>>
>> [...fake-super symlink saved as a file...]
>>
>> This results in the copy being world-writable.
>>
>> Indeed. The file initially gets created as a mode-600 file, but the code
>> later tweaks the permissions to match the symlink, which is (as you
>> note) a bad thing.
>>
>> My first reaction is to change the code in set_stat_xattr()
>> (in xattrs.c) from:
>>
>>        if (fst.st_mode != mode)
>>                do_chmod(fname, mode);
>>
>> to:
>>
>>        if (fst.st_mode != mode && !S_ISLNK(file->mode))
>>                do_chmod(fname, mode);
>>
>> ..wayne.. 
> 
> That's certainly an improvement; from the safety point of view, leaving
> it as 0600 is a lot better than leaving it as 0777. I'm currently
> investigating a slightly more extensive fix to allow more control over
> how fake-symlink files end up, also to make fake-super work better with
> incoming-chmod for the daemon case.
> 
> .Dave.

The other part of this problem is that it doesn't at present seem
possible to satisfy all of three individually reasonable requirements of
a backup system at the same time:

1.  The backup receiver (daemon) need not run as root.
2.  The backup daemon should preserve all of the client's metadata.
3.  The backup daemon should have control over modes, ownership, etc
of the backup files.

1 & 2 can be satisfied today with the use of --fake-super, but even
apart from the issue with 0777 symlinks, the modes of the backup files
are based on those of the originals .. unless you use chmod.

1 & 3 can be satisfied together by using --fake-super with --chmod on
the receiving side or incoming-chmod in the daemon config. But this
actually loses information as it affects not only the modes of the
backup files, but also the state saved in user.rsync.%stat.

So my proposal would be for the receiving-side chmod to be applied (in
fake mode) *after* user.rsync.%stat has been saved, and likewise after
the conversion of symlinks (et al.) to plain files.

This would allow a daemon configuration containing both fake-super and
an absolute setting for incoming-chmod to fulfil all three of the above.

The logical flow for a push-to-fake-super-daemon would then be:
1.  client sends filespecs, applying any local --chmod on the way
2.  daemon receives filespecs, does *not* tweak_mode at this stage
3.  ... data transfer as needed ...
4.  daemon sets backup file attributes
4a. in fake mode, daemon saves user.rsync.%stat, then applies
tweak_mode() to the local file. Daemon modes take precedence
as expected.
4b. in non-fake mode, the daemon just applies the deferred
tweak_mode(). This may lose information, as at present
(because that's *also* useful).

The reverse flow (pull-from-fake-super-daemon) is:
1.  daemon sends filespecs, getting modes from user.rsync.%stat
(daemon would apply outgoing-chmod here, but I don't think it
would be set in this configuration).
2.  client receives filespecs, does *not* tweak_mode at this stage
3.  ... date transfer as needed ...
4.  client sets local file attributes
4b. (non-fake-mode) client applies any local --chmod

Any client-local --chmod is applied later in this flow than at present,
but that should make no difference. Note that the client didn't need to
know (and shouldn't be able to tell?) whether the daemon was super or
just faking it :)

.Dave.


-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

Re: [PATCH] Support running with an unknown current directory (bug 6422)

[Florian Weimer via rsync]

Sorry, I made a Git error and posted the wrong patch.  New patch attached.

Thanks,
Florian
>From dd609baac563e2367b9424f32eead35940f6bc33 Mon Sep 17 00:00:00 2001
From: Florian Weimer 
Date: Tue, 6 Feb 2018 14:43:35 +0100
Subject: [PATCH] Support running with an unknown current directory (bug 6422)

If the current directory is needed later, an error is reported.

Delayed error reporting is needed in more cases because after the fix
for CVE-2018-101 in getcwd

  https://sourceware.org/bugzilla/show_bug.cgi?id=22679

the glibc implementation will no longer return a made-up relative
pathname (as supplied by the Linux kernel) in case the current
directory has no name. Without this patch, rsync will fail to start in
such cases, but the command line arguments might all use absolute
pathnames, so that the current directory is never actually needed.
---
 exclude.c | 23 ++-
 flist.c   | 35 +++
 log.c |  1 +
 main.c| 12 +---
 util.c| 34 --
 5 files changed, 87 insertions(+), 18 deletions(-)

diff --git a/exclude.c b/exclude.c
index 7989fb3e..bbd3b263 100644
--- a/exclude.c
+++ b/exclude.c
@@ -381,6 +381,7 @@ void set_filter_dir(const char *dir, unsigned int dirlen)
 {
 	unsigned int len;
 	if (*dir != '/') {
+		need_curr_dir(dir);
 		memcpy(dirbuf, curr_dir, curr_dir_len);
 		dirbuf[curr_dir_len] = '/';
 		len = curr_dir_len + 1;
@@ -644,15 +645,19 @@ static int rule_matches(const char *fname, filter_rule *ex, int name_flags)
 		 * just match the name portion of the path. */
 		if ((p = strrchr(name,'/')) != NULL)
 			name = p+1;
-	} else if (ex->rflags & FILTRULE_ABS_PATH && *fname != '/'
-	&& curr_dir_len > module_dirlen + 1) {
-		/* If we're matching against an absolute-path pattern,
-		 * we need to prepend our full path info. */
-		strings[str_cnt++] = curr_dir + module_dirlen + 1;
-		strings[str_cnt++] = "/";
-	} else if (ex->rflags & FILTRULE_WILD2_PREFIX && *fname != '/') {
-		/* Allow "**"+"/" to match at the start of the string. */
-		strings[str_cnt++] = "/";
+	} else if (*fname != '/') {
+		need_curr_dir(fname);
+		if (ex->rflags & FILTRULE_ABS_PATH
+		&& curr_dir_len > module_dirlen + 1) {
+			/* If we're matching against an absolute-path pattern,
+			 * we need to prepend our full path info. */
+			strings[str_cnt++] = curr_dir + module_dirlen + 1;
+			strings[str_cnt++] = "/";
+		} else if (ex->rflags & FILTRULE_WILD2_PREFIX) {
+			/* Allow "**"+"/" to match at the start of the
+			 * string. */
+			strings[str_cnt++] = "/";
+		}
 	}
 	strings[str_cnt++] = name;
 	if (name_flags & NAME_IS_DIR) {
diff --git a/flist.c b/flist.c
index 499440cc..0ef6e9a3 100644
--- a/flist.c
+++ b/flist.c
@@ -77,6 +77,7 @@ extern char *filesfrom_host;
 extern char *usermap, *groupmap;
 
 extern char curr_dir[MAXPATHLEN];
+extern int curr_dir_error;
 
 extern struct chmod_mode_struct *chmod_modes;
 
@@ -278,8 +279,31 @@ static void send_directory(int f, struct file_list *flist,
 			   char *fbuf, int len, int flags);
 
 static const char *pathname, *orig_dir;
+static int orig_dir_error;
 static int pathname_len;
 
+/* Save orig_dir and its associated error code. */
+static void
+save_orig_dir(void)
+{
+	if (!orig_dir) {
+		orig_dir = strdup(curr_dir);
+		if (orig_dir == NULL)
+			out_of_memory("save_orig_dir");
+		orig_dir_error = curr_dir_error;
+	}
+}
+
+/* To be called before using orig_dir. */
+static void
+need_orig_dir(void)
+{
+	if (orig_dir_error != 0) {
+		curr_dir_error = orig_dir_error;
+		need_curr_dir(NULL);
+	}
+}
+
 /* Make sure flist can hold at least flist->used + extra entries. */
 static void flist_expand(struct file_list *flist, int extra)
 {
@@ -356,15 +380,19 @@ int change_pathname(struct file_struct *file, const char *dir, int dirlen)
 	pathname = dir;
 	pathname_len = dirlen;
 
-	if (!dir)
+	if (!dir) {
+		need_orig_dir();
 		dir = orig_dir;
+	}
 
 	if (!change_dir(dir, CD_NORMAL)) {
 	  chdir_error:
 		io_error |= IOERR_GENERAL;
 		rsyserr(FERROR_XFER, errno, "change_dir %s failed", full_fname(dir));
-		if (dir != orig_dir)
+		if (dir != orig_dir) {
+			need_orig_dir();
 			change_dir(orig_dir, CD_NORMAL);
+		}
 		pathname = NULL;
 		pathname_len = 0;
 		return 0;
@@ -2108,8 +2136,7 @@ struct file_list *send_file_list(int f, int argc, char *argv[])
 		use_ff_fd = 1;
 	}
 
-	if (!orig_dir)
-		orig_dir = strdup(curr_dir);
+	save_orig_dir();
 
 	while (1) {
 		char fbuf[MAXPATHLEN], *fn, name_type;
diff --git a/log.c b/log.c
index 6143349c..a1f85c3a 100644
--- a/log.c
+++ b/log.c
@@ -602,6 +602,7 @@ static void log_formatted(enum logcode code, const char *format, const char *op,
 } else
 	n = buf2;
 			} else if (am_daemon && *c != '/') {
+need_curr_dir(c);
 pathjoin(buf2, sizeof buf2,
 	 curr_dir + module_dirlen, c);
 clean_fname(buf2, 0);
diff --git a/main.c b/main.c
index ee9630fc..e1f1f705 100644
--- a/main.c
+++ b/main.c
@@ -714,7 

[Bug 6422] rsync needlessly aborts when getcwd() fails

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=6422

--- Comment #3 from Florian Weimer  ---
I posted a patch:
https://lists.samba.org/archive/rsync/2018-February/031476.html

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

[PATCH] Support running with an unknown current directory (bug 6422)

[Florian Weimer via rsync]

This hopefully addresses a glusterfs geo-replication regression after a 
recent security update to glibc.


I see that rsync does not assume that fchdir exists, so I didn't make 
any attempt to use it.


Thanks,
Florian
>From c17b53751fc7b72816b26959f07eb28f532b52e9 Mon Sep 17 00:00:00 2001
From: Florian Weimer 
Date: Tue, 6 Feb 2018 13:48:54 +0100
Subject: [PATCH] Support running with an unknown current directory (bug 6422)

If the current directory is needed later, an error is reported.

Delayed error reporting is needed in more cases because after the fix
for CVE-2018-101 in getcwd

  https://sourceware.org/bugzilla/show_bug.cgi?id=22679

the glibc implementation will no longer return a made-up relative
pathname (as supplied by the Linux kernel) in case the current
directory has no name. Without this patch, rsync will fail to start in
such cases, but the command line arguments might all use absolute
pathnames, so that the current directory is never actually needed.
---
 exclude.c | 23 ++-
 flist.c   | 35 +++
 log.c |  1 +
 main.c| 12 +---
 util.c| 34 --
 5 files changed, 87 insertions(+), 18 deletions(-)

diff --git a/exclude.c b/exclude.c
index 7989fb3e..bbd3b263 100644
--- a/exclude.c
+++ b/exclude.c
@@ -381,6 +381,7 @@ void set_filter_dir(const char *dir, unsigned int dirlen)
 {
 	unsigned int len;
 	if (*dir != '/') {
+		need_curr_dir(dir);
 		memcpy(dirbuf, curr_dir, curr_dir_len);
 		dirbuf[curr_dir_len] = '/';
 		len = curr_dir_len + 1;
@@ -644,15 +645,19 @@ static int rule_matches(const char *fname, filter_rule *ex, int name_flags)
 		 * just match the name portion of the path. */
 		if ((p = strrchr(name,'/')) != NULL)
 			name = p+1;
-	} else if (ex->rflags & FILTRULE_ABS_PATH && *fname != '/'
-	&& curr_dir_len > module_dirlen + 1) {
-		/* If we're matching against an absolute-path pattern,
-		 * we need to prepend our full path info. */
-		strings[str_cnt++] = curr_dir + module_dirlen + 1;
-		strings[str_cnt++] = "/";
-	} else if (ex->rflags & FILTRULE_WILD2_PREFIX && *fname != '/') {
-		/* Allow "**"+"/" to match at the start of the string. */
-		strings[str_cnt++] = "/";
+	} else if (*fname != '/') {
+		need_curr_dir(fname);
+		if (ex->rflags & FILTRULE_ABS_PATH
+		&& curr_dir_len > module_dirlen + 1) {
+			/* If we're matching against an absolute-path pattern,
+			 * we need to prepend our full path info. */
+			strings[str_cnt++] = curr_dir + module_dirlen + 1;
+			strings[str_cnt++] = "/";
+		} else if (ex->rflags & FILTRULE_WILD2_PREFIX) {
+			/* Allow "**"+"/" to match at the start of the
+			 * string. */
+			strings[str_cnt++] = "/";
+		}
 	}
 	strings[str_cnt++] = name;
 	if (name_flags & NAME_IS_DIR) {
diff --git a/flist.c b/flist.c
index 499440cc..0ef6e9a3 100644
--- a/flist.c
+++ b/flist.c
@@ -77,6 +77,7 @@ extern char *filesfrom_host;
 extern char *usermap, *groupmap;
 
 extern char curr_dir[MAXPATHLEN];
+extern int curr_dir_error;
 
 extern struct chmod_mode_struct *chmod_modes;
 
@@ -278,8 +279,31 @@ static void send_directory(int f, struct file_list *flist,
 			   char *fbuf, int len, int flags);
 
 static const char *pathname, *orig_dir;
+static int orig_dir_error;
 static int pathname_len;
 
+/* Save orig_dir and its associated error code. */
+static void
+save_orig_dir(void)
+{
+	if (!orig_dir) {
+		orig_dir = strdup(curr_dir);
+		if (orig_dir == NULL)
+			out_of_memory("save_orig_dir");
+		orig_dir_error = curr_dir_error;
+	}
+}
+
+/* To be called before using orig_dir. */
+static void
+need_orig_dir(void)
+{
+	if (orig_dir_error != 0) {
+		curr_dir_error = orig_dir_error;
+		need_curr_dir(NULL);
+	}
+}
+
 /* Make sure flist can hold at least flist->used + extra entries. */
 static void flist_expand(struct file_list *flist, int extra)
 {
@@ -356,15 +380,19 @@ int change_pathname(struct file_struct *file, const char *dir, int dirlen)
 	pathname = dir;
 	pathname_len = dirlen;
 
-	if (!dir)
+	if (!dir) {
+		need_orig_dir();
 		dir = orig_dir;
+	}
 
 	if (!change_dir(dir, CD_NORMAL)) {
 	  chdir_error:
 		io_error |= IOERR_GENERAL;
 		rsyserr(FERROR_XFER, errno, "change_dir %s failed", full_fname(dir));
-		if (dir != orig_dir)
+		if (dir != orig_dir) {
+			need_orig_dir();
 			change_dir(orig_dir, CD_NORMAL);
+		}
 		pathname = NULL;
 		pathname_len = 0;
 		return 0;
@@ -2108,8 +2136,7 @@ struct file_list *send_file_list(int f, int argc, char *argv[])
 		use_ff_fd = 1;
 	}
 
-	if (!orig_dir)
-		orig_dir = strdup(curr_dir);
+	save_orig_dir();
 
 	while (1) {
 		char fbuf[MAXPATHLEN], *fn, name_type;
diff --git a/log.c b/log.c
index 6143349c..a1f85c3a 100644
--- a/log.c
+++ b/log.c
@@ -602,6 +602,7 @@ static void log_formatted(enum logcode code, const char *format, const char *op,
 } else
 	n = buf2;
 			} else if (am_daemon && *c != '/') {
+need_curr_dir(c);
 pathjoin(buf2, sizeof buf2,
 	 curr_dir + module_dirlen, c);