From 1a7f3d99c5d4bcb5f38e2143bfb99fdf571fab69 Mon Sep 17 00:00:00 2001 From: Wayne Davison Date: Thu, 12 Oct 2006 03:01:18 +0000 Subject: [PATCH] Removed the changes in symlink handling in non-chroot daemon mode as they were not yet safe (I'll consider similar changes for the next release). --- flist.c | 14 ++---- generator.c | 2 +- loadparm.c | 4 -- main.c | 16 +----- options.c | 12 ++--- rsyncd.conf.yo | 26 +++------- testsuite/daemon-gzip-upload.test | 6 +-- util.c | 82 ------------------------------- 8 files changed, 19 insertions(+), 143 deletions(-) diff --git a/flist.c b/flist.c index 16c95aba..0366d9eb 100644 --- a/flist.c +++ b/flist.c @@ -172,7 +172,7 @@ static int readlink_stat(const char *path, STRUCT_STAT *stp, char *linkbuf) rprintf(FINFO,"copying unsafe symlink \"%s\" -> \"%s\"\n", path, linkbuf); } - return safe_stat(path, stp); + return do_stat(path, stp); } } return 0; @@ -185,12 +185,12 @@ int link_stat(const char *path, STRUCT_STAT *stp, int follow_dirlinks) { #ifdef SUPPORT_LINKS if (copy_links) - return safe_stat(path, stp); + return do_stat(path, stp); if (do_lstat(path, stp) < 0) return -1; if (follow_dirlinks && S_ISLNK(stp->st_mode)) { STRUCT_STAT st; - if (safe_stat(path, &st) == 0 && S_ISDIR(st.st_mode)) + if (do_stat(path, &st) == 0 && S_ISDIR(st.st_mode)) *stp = st; } return 0; @@ -655,7 +655,7 @@ static struct file_struct *receive_file_entry(struct file_list *flist, if (linkname_len) { file->u.link = bp; read_sbuf(f, bp, linkname_len - 1); - if (lp_munge_symlinks(module_id)) + if (sanitize_paths) sanitize_path(bp, bp, "", lastdir_depth, NULL); bp += linkname_len; } @@ -922,7 +922,7 @@ struct file_struct *make_file(char *fname, struct file_list *flist, int save_mode = file->mode; file->mode = S_IFDIR; /* Find a directory with our name. */ if (flist_find(the_file_list, file) >= 0 - && safe_stat(thisname, &st2) == 0 && S_ISDIR(st2.st_mode)) { + && do_stat(thisname, &st2) == 0 && S_ISDIR(st2.st_mode)) { file->modtime = st2.st_mtime; file->length = st2.st_size; file->mode = st2.st_mode; @@ -1071,8 +1071,6 @@ struct file_list *send_file_list(int f, int argc, char *argv[]) io_start_buffering_out(); if (filesfrom_fd >= 0) { - if (sanitize_paths) - die_on_unsafe_path(argv[0], 0); if (argv[0] && !push_dir(argv[0], 0)) { rsyserr(FERROR, errno, "push_dir %s failed", full_fname(argv[0])); @@ -1126,8 +1124,6 @@ struct file_list *send_file_list(int f, int argc, char *argv[]) && (len == 1 || fbuf[len-2] == '/'); } - if (sanitize_paths) - die_on_unsafe_path(fbuf, 1); if (link_stat(fbuf, &st, copy_dirlinks) != 0) { io_error |= IOERR_GENERAL; rsyserr(FERROR, errno, "link_stat %s failed", diff --git a/generator.c b/generator.c index 1122c4f2..84801d16 100644 --- a/generator.c +++ b/generator.c @@ -856,7 +856,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, char *dn = file->dirname ? file->dirname : "."; if (parent_dirname != dn && strcmp(parent_dirname, dn) != 0) { if (relative_paths && !implied_dirs - && safe_stat(dn, &st) < 0 + && do_stat(dn, &st) < 0 && create_directory_path(fname) < 0) { rsyserr(FERROR, errno, "recv_generator: mkdir %s failed", diff --git a/loadparm.c b/loadparm.c index dc8ddbe7..24fd3562 100644 --- a/loadparm.c +++ b/loadparm.c @@ -153,7 +153,6 @@ typedef struct BOOL ignore_errors; BOOL ignore_nonreadable; BOOL list; - BOOL munge_symlinks; BOOL read_only; BOOL strict_modes; BOOL transfer_logging; @@ -201,7 +200,6 @@ static service sDefault = /* ignore_errors; */ False, /* ignore_nonreadable; */ False, /* list; */ True, - /* munge_symlinks; */ False, /* read_only; */ True, /* strict_modes; */ True, /* transfer_logging; */ False, @@ -315,7 +313,6 @@ static struct parm_struct parm_table[] = {"log format", P_STRING, P_LOCAL, &sDefault.log_format, NULL,0}, {"max connections", P_INTEGER,P_LOCAL, &sDefault.max_connections, NULL,0}, {"max verbosity", P_INTEGER,P_LOCAL, &sDefault.max_verbosity, NULL,0}, - {"munge symlinks", P_BOOL, P_LOCAL, &sDefault.munge_symlinks, NULL,0}, {"name", P_STRING, P_LOCAL, &sDefault.name, NULL,0}, {"outgoing chmod", P_STRING, P_LOCAL, &sDefault.outgoing_chmod, NULL,0}, {"path", P_PATH, P_LOCAL, &sDefault.path, NULL,0}, @@ -418,7 +415,6 @@ FN_LOCAL_INTEGER(lp_timeout, timeout) FN_LOCAL_BOOL(lp_ignore_errors, ignore_errors) FN_LOCAL_BOOL(lp_ignore_nonreadable, ignore_nonreadable) FN_LOCAL_BOOL(lp_list, list) -FN_LOCAL_BOOL(lp_munge_symlinks, munge_symlinks) FN_LOCAL_BOOL(lp_read_only, read_only) FN_LOCAL_BOOL(lp_strict_modes, strict_modes) FN_LOCAL_BOOL(lp_transfer_logging, transfer_logging) diff --git a/main.c b/main.c index 841d4389..52ec58e2 100644 --- a/main.c +++ b/main.c @@ -477,11 +477,9 @@ static char *get_local_name(struct file_list *flist, char *dest_path) return NULL; /* See what currently exists at the destination. */ - if ((statret = safe_stat(dest_path, &st)) == 0) { + if ((statret = do_stat(dest_path, &st)) == 0) { /* If the destination is a dir, enter it and use mode 1. */ if (S_ISDIR(st.st_mode)) { - if (sanitize_paths) - die_on_unsafe_path(dest_path, 0); if (!push_dir(dest_path, 0)) { rsyserr(FERROR, errno, "push_dir#1 %s failed", full_fname(dest_path)); @@ -489,8 +487,6 @@ static char *get_local_name(struct file_list *flist, char *dest_path) } return NULL; } - if (sanitize_paths && S_ISLNK(st.st_mode)) - die_on_unsafe_path(dest_path, 0); if (flist->count > 1) { rprintf(FERROR, "ERROR: destination must be a directory when" @@ -543,8 +539,6 @@ static char *get_local_name(struct file_list *flist, char *dest_path) dry_run++; } - if (sanitize_paths) - die_on_unsafe_path(dest_path, 0); if (!push_dir(dest_path, dry_run > 1)) { rsyserr(FERROR, errno, "push_dir#2 %s failed", full_fname(dest_path)); @@ -565,8 +559,6 @@ static char *get_local_name(struct file_list *flist, char *dest_path) dest_path = "/"; *cp = '\0'; - if (sanitize_paths) - die_on_unsafe_path(dest_path, 0); if (!push_dir(dest_path, 0)) { rsyserr(FERROR, errno, "push_dir#3 %s failed", full_fname(dest_path)); @@ -658,8 +650,6 @@ static void do_server_sender(int f_in, int f_out, int argc, char *argv[]) } if (!relative_paths) { - if (sanitize_paths) - die_on_unsafe_path(dir, 0); if (!push_dir(dir, 0)) { rsyserr(FERROR, errno, "push_dir#3 %s failed", full_fname(dir)); @@ -854,13 +844,9 @@ static void do_server_recv(int f_in, int f_out, int argc,char *argv[]) char **dir; for (dir = basis_dir; *dir; dir++) { *dir = sanitize_path(NULL, *dir, NULL, curr_dir_depth, NULL); - die_on_unsafe_path(*dir, 0); } if (partial_dir) { partial_dir = sanitize_path(NULL, partial_dir, NULL, curr_dir_depth, NULL); - /* A relative path gets this checked at every dir change. */ - if (*partial_dir == '/') - die_on_unsafe_path(partial_dir, 0); } } fix_basis_dirs(); diff --git a/options.c b/options.c index 74a42790..a568643f 100644 --- a/options.c +++ b/options.c @@ -902,10 +902,8 @@ int parse_arguments(int *argc, const char ***argv, int frommain) case OPT_EXCLUDE_FROM: case OPT_INCLUDE_FROM: arg = poptGetOptArg(pc); - if (sanitize_paths) { + if (sanitize_paths) arg = sanitize_path(NULL, arg, NULL, 0, NULL); - die_on_unsafe_path((char*)arg, 0); - } if (server_filter_list.head) { char *cp = strdup(arg); if (!cp) @@ -1223,14 +1221,10 @@ int parse_arguments(int *argc, const char ***argv, int frommain) int i; for (i = *argc; i-- > 0; ) (*argv)[i] = sanitize_path(NULL, (*argv)[i], "", 0, NULL); - if (tmpdir) { + if (tmpdir) tmpdir = sanitize_path(NULL, tmpdir, NULL, 0, NULL); - die_on_unsafe_path(tmpdir, 0); - } - if (backup_dir) { + if (backup_dir) backup_dir = sanitize_path(NULL, backup_dir, NULL, 0, NULL); - die_on_unsafe_path(backup_dir, 0); - } } if (server_filter_list.head && !am_sender) { struct filter_list_struct *elp = &server_filter_list; diff --git a/rsyncd.conf.yo b/rsyncd.conf.yo index f6075d08..c174654a 100644 --- a/rsyncd.conf.yo +++ b/rsyncd.conf.yo @@ -126,12 +126,14 @@ for each module in tt(rsyncd.conf). dit(bf(use chroot)) If "use chroot" is true, the rsync daemon will chroot to the "path" before starting the file transfer with the client. This has the advantage of extra protection against possible implementation security -holes, but it has the disadvantages of requiring super-user privileges -and of complicating the preservation of usernames and groups -(see below). When "use chroot" is false, rsync takes extra steps to -manually process symlinks in an attempt to make them behave the same -way as when "use chroot" is true (this behavior is new for version -2.6.9). +holes, but it has the disadvantages of requiring super-user privileges, +of not being able to follow symbolic links that are either absolute or outside +of the new root path, and of complicating the preservation of usernames and groups +(see below). When "use chroot" is false, for security reasons, +symlinks may only be relative paths pointing to other files within the root +path, and leading slashes are removed from most absolute paths (options +such as bf(--backup-dir), bf(--compare-dest), etc. interpret an absolute path as +rooted in the module's "path" dir, just as if chroot was specified). The default for "use chroot" is true. In order to preserve usernames and groupnames, rsync needs to be able to @@ -162,18 +164,6 @@ Any clients connecting when the maximum has been reached will receive a message telling them to try later. The default is 0 which means no limit. See also the "lock file" option. -dit(bf(munge symlinks)) The "munge symlinks" option tells rsync to not -allow absolute symlinks (any leading slashes are stripped) and to trim -parent-dir references ("../") if they attempt to move beyond the root of -the transfer. Use this option if you need to ensure that other processes -(besides a daemon rsync) don't ever see a module-created symlink that can -point outside the module, or perhaps if you value safety over preserving -symlink data. - -Prior to rsync 2.6.9, symlink munging was always enabled when "use chroot" -was off, and always disabled when it was on. Starting with 2.6.9, this -symlink-munging is totally controlled by the setting of this option. - dit(bf(log file)) When the "log file" option is set to a non-empty string, the rsync daemon will log messages to the indicated file rather than using syslog. This is particularly useful on systems (such as AIX) diff --git a/testsuite/daemon-gzip-upload.test b/testsuite/daemon-gzip-upload.test index 1b327b44..49a7b1ae 100644 --- a/testsuite/daemon-gzip-upload.test +++ b/testsuite/daemon-gzip-upload.test @@ -22,14 +22,10 @@ export RSYNC_CONNECT_PROG hands_setup -umask 0 -ln -s ../to/nolf "$fromdir/to-nolf-symlink" -umask 022 - # Build chkdir with a normal rsync and an --exclude. $RSYNC -av --exclude=foobar.baz "$fromdir/" "$chkdir/" -checkit "$RSYNC -avvvvz \"$fromdir/\" localhost::test-scratch/to/" "$chkdir" "$todir" +checkit "$RSYNC -avvvvz \"$fromdir/\" localhost::test-to/" "$chkdir" "$todir" # The script would have aborted on error, so getting here means we've won. exit 0 diff --git a/util.c b/util.c index 72e08597..dc37c04a 100644 --- a/util.c +++ b/util.c @@ -861,86 +861,6 @@ char *sanitize_path(char *dest, const char *p, const char *rootdir, int depth, return dest; } -/* If sanitize_paths is not set, this works exactly the same as do_stat(). - * Otherwise, we verify that no symlink takes us outside the module path. - * If we encounter an escape attempt, we return a symlink's stat info! */ -int safe_stat(const char *fname, STRUCT_STAT *stp) -{ -#ifdef SUPPORT_LINKS - char tmpbuf[MAXPATHLEN], linkbuf[MAXPATHLEN], *mod_path; - int i, llen, mod_path_len; - - if (!sanitize_paths) - return do_stat(fname, stp); - - mod_path = lp_path(module_id); - mod_path_len = strlen(mod_path); - - for (i = 0; i < 16; i++) { -#ifdef DEBUG - if (*fname == '/') - assert(strncmp(fname, mod_path, mod_path_len) == 0 && fname[mod_path_len] == '/'); -#endif - if (do_lstat(fname, stp) < 0) - return -1; - if (!S_ISLNK(stp->st_mode)) - return 0; - if ((llen = readlink(fname, linkbuf, sizeof linkbuf - 1)) < 0) - return -1; - linkbuf[llen] = '\0'; - if (*fname == '/') - fname += mod_path_len; - if (!(fname = sanitize_path(tmpbuf, fname, mod_path, curr_dir_depth, linkbuf))) - break; - } - - return 0; /* Leave *stp set to the last symlink. */ -#else - return do_stat(fname, stp); -#endif -} - -void die_on_unsafe_path(char *path, int strip_filename) -{ -#ifdef SUPPORT_LINKS - char *final_slash, *p; - STRUCT_STAT st; - - if (!path) - return; - if (strip_filename) { - if (!(final_slash = strrchr(path, '/'))) - return; - *final_slash = '\0'; - } else - final_slash = NULL; - - p = path; - if (*p == '/') - p += module_dirlen + 1; - while (*p) { - if ((p = strchr(p, '/')) != NULL) - *p = '\0'; - if (safe_stat(path, &st) < 0) { - if (p) - *p = '/'; - goto done; - } - if (S_ISLNK(st.st_mode)) { - rprintf(FERROR, "Unsafe path: %s\n", path); - exit_cleanup(RERR_SYNTAX); - } - if (!p) - break; - *p++ = '/'; - } - - done: - if (final_slash) - *final_slash = '/'; -#endif -} - /* Like chdir(), but it keeps track of the current directory (in the * global "curr_dir"), and ensures that the path size doesn't overflow. * Also cleans the path using the clean_fname() function. */ @@ -1091,8 +1011,6 @@ int handle_partial_dir(const char *fname, int create) if (create) { STRUCT_STAT st; int statret = do_lstat(dir, &st); - if (sanitize_paths && *partial_dir != '/') - die_on_unsafe_path(dir, 1); /* lstat handles last element */ if (statret == 0 && !S_ISDIR(st.st_mode)) { if (do_unlink(dir) < 0) return 0; -- 2.34.1