Skip to content

Commit

Permalink
Merge pull request #37 from Castaglia/root-path-filtering-issue1491
Browse files Browse the repository at this point in the history 
Address a subtle issue in the handling of paths whose names match tha…
  • Loading branch information
@Castaglia
Castaglia authored Aug 3, 2022
2 parents 5676f51 + b231f7e commit 2335d4b
Show file tree
Hide file tree
Showing 4 changed files with 419 additions and 43 deletions.
4 changes: 2 additions & 2 deletions mod_vroot.h.in
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
* ProFTPD - mod_vroot
* Copyright (c) 2016-2019 TJ Saunders
* Copyright (c) 2016-2022 TJ Saunders
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
Expand All @@ -27,7 +27,7 @@

#include "conf.h"

#define MOD_VROOT_VERSION "mod_vroot/0.9.9"
#define MOD_VROOT_VERSION "mod_vroot/0.9.10"

/* Make sure the version of proftpd is as necessary. */
#if PROFTPD_VERSION_NUMBER < 0x0001030602
Expand Down
72 changes: 48 additions & 24 deletions path.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,31 +246,49 @@ char *vroot_realpath(pool *p, const char *path, int flags) {
return real_path;
}

int vroot_path_lookup(pool *p, char *path, size_t pathlen, const char *dir,
/* The given `vpath` buffer is the looked-up path for the given `path`. */
int vroot_path_lookup(pool *p, char *vpath, size_t vpathsz, const char *path,
int flags, char **alias_path) {
char buf[PR_TUNABLE_PATH_MAX + 1], *bufp = NULL;
const char *cwd;

if (dir == NULL) {
if (vpath == NULL ||
path == NULL) {
errno = EINVAL;
return -1;
}

memset(buf, '\0', sizeof(buf));
memset(path, '\0', pathlen);
if (vpath != NULL &&
vpathsz > 0) {
memset(vpath, '\0', vpathsz);
}

if (strcmp(dir, ".") != 0) {
sstrncpy(buf, dir, sizeof(buf));
cwd = pr_fs_getcwd();

if (strcmp(path, ".") != 0) {
sstrncpy(buf, path, sizeof(buf));

} else {
sstrncpy(buf, pr_fs_getcwd(), sizeof(buf));
sstrncpy(buf, cwd, sizeof(buf));
}

vroot_path_clean(buf);

bufp = buf;

if (strncmp(bufp, vroot_base, vroot_baselen) == 0) {
bufp += vroot_baselen;
size_t len;

/* Attempt to handle cases like "/base/base" and "/base/basefoo", where
* the base is just "/base".
* See https://github.com/proftpd/proftpd/issues/1491
*/
len = strlen(bufp);
if (len > vroot_baselen &&
bufp[vroot_baselen] == '/') {
bufp += vroot_baselen;
}
}

loop:
Expand All @@ -280,19 +298,19 @@ int vroot_path_lookup(pool *p, char *path, size_t pathlen, const char *dir,
bufp[1] == '.' &&
(bufp[2] == '\0' ||
bufp[2] == '/')) {
char *tmp = NULL;
char *ptr = NULL;

tmp = strrchr(path, '/');
if (tmp != NULL) {
*tmp = '\0';
ptr = strrchr(vpath, '/');
if (ptr != NULL) {
*ptr = '\0';

} else {
*path = '\0';
*vpath = '\0';
}

if (strncmp(path, vroot_base, vroot_baselen) == 0 ||
path[vroot_baselen] != '/') {
snprintf(path, pathlen, "%s/", vroot_base);
if (strncmp(vpath, vroot_base, vroot_baselen) == 0 ||
vpath[vroot_baselen] != '/') {
snprintf(vpath, vpathsz, "%s/", vroot_base);
}

if (bufp[0] == '.' &&
Expand All @@ -303,7 +321,7 @@ int vroot_path_lookup(pool *p, char *path, size_t pathlen, const char *dir,
}

} else if (*bufp == '/') {
snprintf(path, pathlen, "%s/", vroot_base);
snprintf(vpath, vpathsz, "%s/", vroot_base);
bufp += 1;
goto loop;

Expand Down Expand Up @@ -345,19 +363,19 @@ int vroot_path_lookup(pool *p, char *path, size_t pathlen, const char *dir,
}

buflen = strlen(bufp) + 1;
tmplen = strlen(path);
tmplen = strlen(vpath);

if (tmplen + buflen >= pathlen) {
if (tmplen + buflen >= vpathsz) {
errno = ENAMETOOLONG;
return -1;
}

path[tmplen] = '/';
memcpy(path + tmplen + 1, bufp, buflen);
vpath[tmplen] = '/';
memcpy(vpath + tmplen + 1, bufp, buflen);
}

/* Clean any unnecessary characters added by the above processing. */
vroot_path_clean(path);
vroot_path_clean(vpath);

if (!(flags & VROOT_LOOKUP_FL_NO_ALIAS)) {
int alias_count;
Expand All @@ -372,7 +390,7 @@ int vroot_path_lookup(pool *p, char *path, size_t pathlen, const char *dir,
* aliases are found.
*/
bufp = buf;
start_ptr = path;
start_ptr = vpath;

while (start_ptr != NULL) {
char *ptr = NULL;
Expand Down Expand Up @@ -402,11 +420,11 @@ int vroot_path_lookup(pool *p, char *path, size_t pathlen, const char *dir,
*alias_path, start_ptr);
}

sstrncpy(path, src_path, pathlen);
sstrncpy(vpath, src_path, vpathsz);

if (end_ptr != NULL) {
/* Now tack on our suffix from the scratchpad. */
sstrcat(path, bufp, pathlen);
sstrcat(vpath, bufp, vpathsz);
}

break;
Expand Down Expand Up @@ -435,5 +453,11 @@ int vroot_path_lookup(pool *p, char *path, size_t pathlen, const char *dir,
}
}

/* Note that logging the session.chroot_path here will not help; mod_vroot
* deliberately always sets that to just "/".
*/
pr_trace_msg(trace_channel, 19,
"lookup: path = '%s', cwd = '%s', base = '%s', vpath = '%s'", path, cwd,
vroot_base, vpath);
return 0;
}
179 changes: 164 additions & 15 deletions t/api/path.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,34 +211,181 @@ END_TEST

START_TEST (path_lookup_test) {
int res;
char *path;
size_t pathlen;
const char *dir;
char *vpath = NULL;
size_t vpathsz = 1024;
const char *path;

mark_point();
path = pstrdup(p, "/foo");
pathlen = strlen(path);
res = vroot_path_lookup(p, path, pathlen, NULL, 0, NULL);
ck_assert_msg(res < 0, "Failed to handle null dir");
vpath = pcalloc(p, vpathsz);
res = vroot_path_lookup(p, vpath, vpathsz, NULL, 0, NULL);
ck_assert_msg(res < 0, "Failed to handle null path");
ck_assert_msg(errno == EINVAL, "Expected EINVAL (%d), got '%s' (%d)", EINVAL,
strerror(errno), errno);

mark_point();
path = "/";
res = vroot_path_lookup(NULL, NULL, 0, path, 0, NULL);
ck_assert_msg(res < 0, "Failed to handle null vpath");
ck_assert_msg(errno == EINVAL, "Expected EINVAL (%d), got '%s' (%d)", EINVAL,
strerror(errno), errno);

mark_point();
dir = "/";
res = vroot_path_lookup(NULL, NULL, 0, dir, 0, NULL);
ck_assert_msg(res >= 0, "Failed to handle null pool, path: %s",
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));

mark_point();
path = ".";
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));
}
END_TEST

/* See: https://github.com/proftpd/proftpd/issues/1491 */
START_TEST (path_lookup_issue1491_test) {
int res;
char *vpath = NULL;
size_t vpathsz = 1024, baselen;
const char *base, *path, *expected;

vpath = pcalloc(p, vpathsz);
base = "/store";
baselen = strlen(base);

/* Set the base. */
mark_point();
res = vroot_path_set_base(base, baselen);
ck_assert_msg(res == 0, "Failed to set base '%s': %s", base, strerror(errno));

/* Start with an absolute path that matches the base. */
mark_point();
path = base;

/* NOTE: Yes, this is a surprising expectation; it has to do with the
* necessary fixes for Issue #1491. Sigh.
*/
expected = "/store/store";
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));
ck_assert_msg(strcmp(vpath, expected) == 0, "Expected '%s', got '%s'",
expected, vpath);

/* Then try a relative path whose name matches the base, sans the leading
* path delimiter.
*/
mark_point();
path = "store";
expected = base;
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));
ck_assert_msg(strcmp(vpath, expected) == 0, "Expected '%s', got '%s'",
expected, vpath);

/* Next, try a relative path for a file whose name starts with that of
* the base.
*/
mark_point();
res = vroot_path_lookup(p, path, pathlen, dir, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup path '%s': %s", path,
path = "storetest";
expected = "/storetest";
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));
ck_assert_msg(strcmp(vpath, expected) == 0, "Expected '%s', got '%s'",
expected, vpath);

/* Next, use an absolute path for a file whose name starts with that of the
* base; this appears to be the root of Issue #1491.
*/
mark_point();
dir = ".";
res = vroot_path_lookup(p, path, pathlen, dir, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup path '%s': %s", path,
path = "/storetest";

/* NOTE: Yes, this is a surprising expectation; it has to do with the
* necessary fixes for Issue #1491. Sigh.
*/
expected = "/store/storetest";
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));
ck_assert_msg(strcmp(vpath, expected) == 0, "Expected '%s', got '%s'",
expected, vpath);

/* Set the new base. */
base = "/store/store";
baselen = strlen(base);

mark_point();
res = vroot_path_set_base(base, baselen);
ck_assert_msg(res == 0, "Failed to set base '%s': %s", base, strerror(errno));

/* Start with an absolute path that matches the base. */
mark_point();
path = base;

/* NOTE: Yes, this is a surprising expectation; it has to do with the
* necessary fixes for Issue #1491. Sigh. This is starting to look a little
* ridiculous.
*/
expected = "/store/store/store/store";
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));
ck_assert_msg(strcmp(vpath, expected) == 0, "Expected '%s', got '%s'",
expected, vpath);

/* Then try a relative path whose name matches the base, sans the leading
* path delimiter.
*/
mark_point();
path = "store";
expected = "/store";
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));
ck_assert_msg(strcmp(vpath, expected) == 0, "Expected '%s', got '%s'",
expected, vpath);

/* Next, try a relative path for a file whose name starts with that of
* the base.
*/
mark_point();
path = "storetest";
expected = "/storetest";
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));
ck_assert_msg(strcmp(vpath, expected) == 0, "Expected '%s', got '%s'",
expected, vpath);

/* Next, use an absolute path for a file whose name starts with that of the
* base; this appears to be the root of Issue #1491.
*/
mark_point();
path = "/storetest";

/* NOTE: Yes, this is a surprising expectation; it has to do with the
* necessary fixes for Issue #1491. Sigh.
*/
expected = "/store/store/storetest";
res = vroot_path_lookup(p, vpath, vpathsz, path, 0, NULL);
ck_assert_msg(res >= 0, "Failed to lookup vpath for '%s': %s", path,
strerror(errno));
ck_assert_msg(strcmp(vpath, expected) == 0, "Expected '%s', got '%s'",
expected, vpath);

/* Clear the base, using an empty string. */
mark_point();
path = "";
res = vroot_path_set_base(path, 0);
ck_assert_msg(res == 0, "Failed to set empty path as base: %s",
strerror(errno));
}
END_TEST

/* TODO */
START_TEST (path_lookup_with_alias_test) {
}
END_TEST

Expand All @@ -257,6 +404,8 @@ Suite *tests_get_path_suite(void) {
tcase_add_test(testcase, path_clean_test);
tcase_add_test(testcase, realpath_test);
tcase_add_test(testcase, path_lookup_test);
tcase_add_test(testcase, path_lookup_issue1491_test);
tcase_add_test(testcase, path_lookup_with_alias_test);

suite_add_tcase(suite, testcase);
return suite;
Expand Down
Loading

0 comments on commit 2335d4b

Please sign in to comment.