Fix significant security holes with "use chroot = no" in an rsync daemon:

1. The file paths being sent and received were not "sanitized" to
	ensure that there weren't any ".." components that would escape the
	top level directory.  This can't happen with the standard rsync
	client, but it could be exploited on both read and write if someone
	modified an rsync client.  This fix sanitizes all incoming and
	outgoing paths when "use chroot = no".

    2. If a module is also "read only = no", clients could have created
	symbolic links with ".." components that would allow writing
	outside of the module.  This could happen with the standard rsync
	client.  This fix sanitizes all incoming symbolic link targets
	when "use chroot = no".

Previously, only top-level paths (anything passed in command line arguments)
were sanitized.  Sorry, I didn't think about the individual file paths
before now.
This commit is contained in:
David Dykstra
1999-07-09 15:49:46 +00:00
parent 0503f06089
commit cb13abfed0
3 changed files with 76 additions and 35 deletions

View File

@@ -25,6 +25,7 @@ extern int read_only;
extern int verbose;
extern int rsync_port;
char *auth_user;
int sanitize_paths = 0;
int start_socket_client(char *host, char *path, int argc, char *argv[])
{
@@ -221,6 +222,7 @@ static int rsync_module(int fd, int i)
io_printf(fd,"@ERROR: chdir failed\n");
return -1;
}
sanitize_paths = 1;
}
if (am_root) {
@@ -262,7 +264,7 @@ static int rsync_module(int fd, int i)
request = strdup(p);
start_glob++;
}
glob_expand(name, argv, &argc, MAX_ARGS, !use_chroot);
glob_expand(name, argv, &argc, MAX_ARGS);
} else {
argc++;
}
@@ -276,7 +278,7 @@ static int rsync_module(int fd, int i)
}
}
if (!use_chroot) {
if (sanitize_paths) {
/*
* Note that this is applied to all parameters, whether or not
* they are filenames, but no other legal parameters contain
@@ -285,7 +287,7 @@ static int rsync_module(int fd, int i)
* and which aren't.
*/
for (i = 1; i < argc; i++) {
sanitize_path(argv[i]);
sanitize_path(argv[i], NULL);
}
}

11
flist.c
View File

@@ -45,6 +45,7 @@ extern int copy_links;
extern int copy_unsafe_links;
extern int remote_version;
extern int io_error;
extern int sanitize_paths;
static char topsrcname[MAXPATHLEN];
@@ -309,6 +310,10 @@ static void receive_file_entry(struct file_struct **fptr,
clean_fname(thisname);
if (sanitize_paths) {
sanitize_path(thisname, NULL);
}
if ((p = strrchr(thisname,'/'))) {
static char *lastdir;
*p = 0;
@@ -343,6 +348,9 @@ static void receive_file_entry(struct file_struct **fptr,
file->link = (char *)malloc(l+1);
if (!file->link) out_of_memory("receive_file_entry 2");
read_sbuf(f,file->link,l);
if (sanitize_paths) {
sanitize_path(file->link, file->dirname);
}
}
#if SUPPORT_HARD_LINKS
@@ -414,6 +422,9 @@ static struct file_struct *make_file(int f, char *fname)
strlcpy(cleaned_name, fname, MAXPATHLEN);
cleaned_name[MAXPATHLEN-1] = 0;
clean_fname(cleaned_name);
if (sanitize_paths) {
sanitize_path(cleaned_name, NULL);
}
fname = cleaned_name;
memset(sum,0,SUM_LENGTH);

54
util.c
View File

@@ -477,7 +477,7 @@ int lock_range(int fd, int offset, int len)
}
static void glob_expand_one(char *s, char **argv, int *argc, int maxargs, int sanitize_paths)
static void glob_expand_one(char *s, char **argv, int *argc, int maxargs)
{
#if !(defined(HAVE_GLOB) && defined(HAVE_GLOB_H))
if (!*s) s = ".";
@@ -485,14 +485,16 @@ static void glob_expand_one(char *s, char **argv, int *argc, int maxargs, int sa
(*argc)++;
return;
#else
extern int sanitize_paths;
glob_t globbuf;
int i;
if (!*s) s = ".";
s = strdup(s);
sanitize_path(s);
argv[*argc] = s;
argv[*argc] = strdup(s);
if (sanitize_paths) {
sanitize_path(argv[*argc], NULL);
}
memset(&globbuf, 0, sizeof(globbuf));
glob(argv[*argc], 0, NULL, &globbuf);
@@ -511,7 +513,7 @@ static void glob_expand_one(char *s, char **argv, int *argc, int maxargs, int sa
#endif
}
void glob_expand(char *base1, char **argv, int *argc, int maxargs, int sanitize_paths)
void glob_expand(char *base1, char **argv, int *argc, int maxargs)
{
char *s = argv[*argc];
char *p, *q;
@@ -535,11 +537,11 @@ void glob_expand(char *base1, char **argv, int *argc, int maxargs, int sanitize_
while ((p = strstr(q,base)) && ((*argc) < maxargs)) {
/* split it at this point */
*p = 0;
glob_expand_one(q, argv, argc, maxargs, sanitize_paths);
glob_expand_one(q, argv, argc, maxargs);
q = p+strlen(base);
}
if (*q && (*argc < maxargs)) glob_expand_one(q, argv, argc, maxargs, sanitize_paths);
if (*q && (*argc < maxargs)) glob_expand_one(q, argv, argc, maxargs);
free(s);
free(base);
@@ -635,8 +637,11 @@ void clean_fname(char *name)
/*
* Make path appear as if a chroot had occurred:
* 1. remove leading "/" (or replace with "." if at end)
* 2. remove leading ".." components
* 2. remove leading ".." components (except those allowed by "reldir")
* 3. delete any other "<dir>/.." (recursively)
* If "reldir" is non-null, it is a sanitized directory that the path will be
* relative to, so allow as many ".." at the beginning of the path as
* there are components in reldir.
* While we're at it, remove double slashes and "." components like
* clean_fname does(), but DON'T remove a trailing slash because that
* is sometimes significant on command line arguments.
@@ -644,10 +649,20 @@ void clean_fname(char *name)
* Contributed by Dave Dykstra <dwd@bell-labs.com>
*/
void sanitize_path(char *p)
void sanitize_path(char *p, char *reldir)
{
char *start, *sanp;
int depth = 0;
int allowdotdot = 0;
if (reldir) {
depth++;
while (*reldir) {
if (*reldir++ == '/') {
depth++;
}
}
}
start = p;
sanp = p;
while (*p == '/') {
@@ -665,9 +680,17 @@ void sanitize_path(char *p)
/* skip following slashes */
;
}
} else if ((*p == '.') && (*(p+1) == '.') &&
continue;
}
allowdotdot = 0;
if ((*p == '.') && (*(p+1) == '.') &&
((*(p+2) == '/') || (*(p+2) == '\0'))) {
/* skip ".." component followed by slash or end */
/* ".." component followed by slash or end */
if ((depth > 0) && (sanp == start)) {
/* allow depth levels of .. at the beginning */
--depth;
allowdotdot = 1;
} else {
p += 2;
if (*p == '/')
p++;
@@ -679,7 +702,9 @@ void sanitize_path(char *p)
sanp--;
}
}
} else {
continue;
}
}
while (1) {
/* copy one component through next slash */
*sanp++ = *p++;
@@ -691,9 +716,12 @@ void sanitize_path(char *p)
break;
}
}
if (allowdotdot) {
/* move the virtual beginning to leave the .. alone */
start = sanp;
}
}
if (sanp == start) {
if ((sanp == start) && !allowdotdot) {
/* ended up with nothing, so put in "." component */
*sanp++ = '.';
}