rsync - Nov 2007 - Funny issue with chroot + symlink outside chroot

Hi,

The problem I found is not in what rsync, but on the error handling:

How to reproduce:
On server side I setup a tree and share using rsync + xinetd, of course, for 
security reason I use chroot option.

Now I push this symlink:
lrwxrwxrwx 1 root     root      4 nov 10 16:28 horsroot -> /bin/

Now I try to do:
./rsync -avPH  --copy-unsafe-links draco::test/ /tmp/rsyncdest/

(Of course the transfert will failed because /bin is outside the chroot.)

The first problem is the error message is not push to stderr but stdout:
---------------------------
[nanardon@draco rsync]$ rsync -avPH  --copy-unsafe-links 
virgo::test/ /tmp/rsyncdest/ 2>/dev/null
[...]
file has vanished: "/horsroot" (in test)
--------------------------

Nothing is sent to stderr whereas error code is set:
$ echo $?
24

I had a look to flist.c/sender.c, it seems this is expected behavior:
(flist.c:1037)
enum logcode c = am_daemon && protocol_version < 28
                    ? FERROR : FINFO;
and indeed the message is produce by daemon, and I am using proto 29 or 30.

Well, I also found the message is not the appropriate one, the file has not 
vanished (at least, we cannot know that, the file pointed is outside our 
tree).

I tried this to fix, but this part is not trivial, and I am not sure this does 
not have side effect. (The patch is partial, the same should be applied to 
sender.c).

diff -u -b -B -w -p -r1.468 flist.c
--- flist.c     6 Nov 2007 15:25:02 -0000       1.468
+++ flist.c     10 Nov 2007 17:21:51 -0000
@@ -1026,8 +1026,8 @@ struct file_struct *make_file(const char
                if (save_errno == ENOENT) {
 #ifdef SUPPORT_LINKS
                        /* Avoid "vanished" error if symlink points
nowhere.
*/
-                       if (copy_links && x_lstat(thisname, &st,
NULL) == 0
-                           && S_ISLNK(st.st_mode)) {
+                       if ((copy_links && x_lstat(thisname, &st,
NULL) == 0
+                           && S_ISLNK(st.st_mode)) || x_stat(thisname,
&st,
NULL) != 0) {
                                io_error |= IOERR_GENERAL;
                                rprintf(FERROR, "symlink has no 
referent: %s\n",
                                        full_fname(thisname));

Summary:
- should "vanished" message be report as INFO instead ERROR ?
- can we detected --copy-unsafe-symlink for file outside chroot ?

Regards.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
Url :
http://lists.samba.org/archive/rsync/attachments/20071110/2c5aef7b/attachment.bin

On Sat, 2007-11-10 at 18:36 +0100, Olivier Thauvin
wrote:
> How to reproduce:
> On server side I setup a tree and share using rsync + xinetd, of course,
for
> security reason I use chroot option.
> 
> Now I push this symlink:
> lrwxrwxrwx 1 root     root      4 nov 10 16:28 horsroot -> /bin/
> 
> Now I try to do:
> ./rsync -avPH  --copy-unsafe-links draco::test/ /tmp/rsyncdest/
> 
> (Of course the transfert will failed because /bin is outside the chroot.)
> 
> The first problem is the error message is not push to stderr but stdout:
> ---------------------------
> [nanardon@draco rsync]$ rsync -avPH  --copy-unsafe-links 
> virgo::test/ /tmp/rsyncdest/ 2>/dev/null
> [...]
> file has vanished: "/horsroot" (in test)
> --------------------------
> 
> Nothing is sent to stderr whereas error code is set:
> $ echo $?
> 24
> 
> I had a look to flist.c/sender.c, it seems this is expected behavior:
> (flist.c:1037)
> enum logcode c = am_daemon && protocol_version < 28
>                     ? FERROR : FINFO;
> and indeed the message is produce by daemon, and I am using proto 29 or 30.
> 
> Well, I also found the message is not the appropriate one, the file has not
> vanished (at least, we cannot know that, the file pointed is outside our 
> tree).
> 
> I tried this to fix, but this part is not trivial, and I am not sure this
does
> not have side effect. (The patch is partial, the same should be applied to 
> sender.c).
> 
> diff -u -b -B -w -p -r1.468 flist.c
> --- flist.c     6 Nov 2007 15:25:02 -0000       1.468
> +++ flist.c     10 Nov 2007 17:21:51 -0000
> @@ -1026,8 +1026,8 @@ struct file_struct *make_file(const char
>                 if (save_errno == ENOENT) {
>  #ifdef SUPPORT_LINKS
>                         /* Avoid "vanished" error if symlink
points nowhere.
> */
> -                       if (copy_links && x_lstat(thisname,
&st, NULL) == 0
> -                           && S_ISLNK(st.st_mode)) {
> +                       if ((copy_links && x_lstat(thisname,
&st, NULL) == 0
> +                           && S_ISLNK(st.st_mode)) ||
x_stat(thisname, &st,
> NULL) != 0) {
>                                 io_error |= IOERR_GENERAL;
>                                 rprintf(FERROR, "symlink has no 
> referent: %s\n",
>                                         full_fname(thisname));
The trouble is that the "symlink has no referent" message needs to be
extended to apply to symlinks followed because of --copy-unsafe-links as
well as --copy-links.  Your patch will trigger the message for *all*
vanishings regardless of whether a symlink is involved, which is wrong.
My proposed patch will follow.

Matt

---
Wayne, please check my comment to see whether I have understood the code
correctly.

 flist.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/flist.c b/flist.c
index 3616a8e..7b0fa13 100644
--- a/flist.c
+++ b/flist.c
@@ -1037,8 +1037,14 @@ struct file_struct *make_file(const char *fname, struct
file_list *flist,
 		}
 		if (save_errno == ENOENT) {
 #ifdef SUPPORT_LINKS
-			/* Avoid "vanished" error if symlink points nowhere. */
-			if (copy_links && x_lstat(thisname, &st, NULL) == 0
+			/* When our options tell us to follow a symlink that
+			 * points nowhere, state that instead of giving the
+			 * "vanished" message.  This case applies whenever the
+			 * symlink exists, but the symlink can only exist with
+			 * --copy-links or --copy-unsafe-links, so we avoid the
+			 * extra lstat when neither of those options is on. */
+			if ((copy_links || copy_unsafe_links)
+			    && x_lstat(thisname, &st, NULL) == 0
 			    && S_ISLNK(st.st_mode)) {
 				io_error |= IOERR_GENERAL;
 				rprintf(FERROR_XFER, "symlink has no referent: %s\n",
-- 
1.5.4.rc3.15.g4bbc

On Tue, 2008-01-29 at 21:06 -0500, Matt McCutchen wrote:
> The trouble is that the "symlink has no referent" message needs
to be
> extended to apply to symlinks followed because of --copy-unsafe-links as
> well as --copy-links.
Olivier,

Wayne has committed an improved version of my patch that extends the
"symlink has no referent" case to both --copy-unsafe-links and
--copy-dirlinks, so your case should be working now in the current
development rsync.

Matt