The branch, master has been updated via fa1087bc5a9 s3: VFS: widelinks. Change call to resolve_realpath_name() -> canonicalize_absolute_path(). via 045e8898ba7 s3: selftest: Remove test_vfs_widelinks. via e4427ff8fe6 s3: lib: Remove the old canonicalize_absolute_path(). via 594c3712c88 s3: util: Replace the old (hard to understand) canonicalize_absolute_path() with a version created from resolve_realpath_name() in vfs_widelinks.c via d01e11cf261 s3: lib: Fix canonicalize_absolute_path() to pass the tests from resolve_realpath_name() via 9cea0cc5b5f s3: torture: Add the tests from resolve_realpath_name() to canonicalize_absolute_path(). from b7fb6100b4c util: Fix signed/unsigned integer comparison
https://git.samba.org/?p=samba.git;a=shortlog;h=master - Log ----------------------------------------------------------------- commit fa1087bc5a95c7743b85ee974a0bc276057d2068 Author: Jeremy Allison <j...@samba.org> Date: Tue Apr 21 13:39:10 2020 -0700 s3: VFS: widelinks. Change call to resolve_realpath_name() -> canonicalize_absolute_path(). That code was moved into source3/lib/util_path.c. We now have *one* canonicalize_absolute_path() funtion, tested more completely. Signed-off-by: Jeremy Allison <j...@samba.org> Reviewed-by: Ralph Boehme <s...@samba.org> Autobuild-User(master): Ralph Böhme <s...@samba.org> Autobuild-Date(master): Wed Apr 22 09:51:08 UTC 2020 on sn-devel-184 commit 045e8898ba727f0c7e9c39965c7d60d4692889aa Author: Jeremy Allison <j...@samba.org> Date: Tue Apr 21 13:34:52 2020 -0700 s3: selftest: Remove test_vfs_widelinks. All of the tests that were in there are now tested in samba3.smbtorture_s3.LOCAL-CANONICALIZE-PATH along with other paths. Clean revert of f7fe3474298 not possible due to changes in source3/selftest/tests.py Signed-off-by: Jeremy Allison <j...@samba.org> Reviewed-by: Ralph Boehme <s...@samba.org> commit e4427ff8fe66c8c371f711ec8fc59811e7383af5 Author: Jeremy Allison <j...@samba.org> Date: Tue Apr 21 13:30:38 2020 -0700 s3: lib: Remove the old canonicalize_absolute_path(). This code was really hard to understand. Signed-off-by: Jeremy Allison <j...@samba.org> Reviewed-by: Ralph Boehme <s...@samba.org> commit 594c3712c885447c096df9892da8f6f28f1ce8d9 Author: Jeremy Allison <j...@samba.org> Date: Tue Apr 21 13:24:44 2020 -0700 s3: util: Replace the old (hard to understand) canonicalize_absolute_path() with a version created from resolve_realpath_name() in vfs_widelinks.c This code is *much* more comprehensible and passes the stricter test set than the original (unfixed) canonicalize_absolute_path() out of the gate. Signed-off-by: Jeremy Allison <j...@samba.org> Reviewed-by: Ralph Boehme <s...@samba.org> commit d01e11cf261003ecf332765538216cff7001163d Author: Jeremy Allison <j...@samba.org> Date: Tue Apr 21 12:58:02 2020 -0700 s3: lib: Fix canonicalize_absolute_path() to pass the tests from resolve_realpath_name() Remove the knownfail. Signed-off-by: Jeremy Allison <j...@samba.org> Reviewed-by: Ralph Boehme <s...@samba.org> commit 9cea0cc5b5f37e8ec612ce3a33705ffb4670a070 Author: Jeremy Allison <j...@samba.org> Date: Tue Apr 21 11:49:44 2020 -0700 s3: torture: Add the tests from resolve_realpath_name() to canonicalize_absolute_path(). canonicalize_absolute_path() has a bug. In canonicalize_absolute_path() ///a/./././///component/../////path/ -> /a//path It should go to /a/path. Mark as knownfail. Adding these tests so I can ultimately remove resolve_realpath_name() and re-use the existing canonicalize_absolute_path() code in vfs_widelinks.c Signed-off-by: Jeremy Allison <j...@samba.org> Reviewed-by: Ralph Boehme <s...@samba.org> ----------------------------------------------------------------------- Summary of changes: source3/lib/util_path.c | 175 +++++++++++++++-------------------- source3/modules/test_vfs_widelinks.c | 88 ------------------ source3/modules/vfs_widelinks.c | 111 +--------------------- source3/modules/wscript_build | 5 - source3/selftest/tests.py | 4 - source3/torture/torture.c | 38 ++++++++ 6 files changed, 116 insertions(+), 305 deletions(-) delete mode 100644 source3/modules/test_vfs_widelinks.c Changeset truncated at 500 lines: diff --git a/source3/lib/util_path.c b/source3/lib/util_path.c index 63d01b81000..c34b734384c 100644 --- a/source3/lib/util_path.c +++ b/source3/lib/util_path.c @@ -107,135 +107,106 @@ char *cache_path(TALLOC_CTX *mem_ctx, const char *name) * @retval Pointer to a talloc'ed string containing the absolute full path. **/ -char *canonicalize_absolute_path(TALLOC_CTX *ctx, const char *abs_path) +char *canonicalize_absolute_path(TALLOC_CTX *ctx, const char *pathname_in) { - char *destname; - char *d; - const char *s = abs_path; - bool start_of_name_component = true; - - /* Allocate for strlen + '\0' + possible leading '/' */ - destname = (char *)talloc_size(ctx, strlen(abs_path) + 2); - if (destname == NULL) { + /* + * Note we use +2 here so if pathname_in=="" then we + * have space to return "/". + */ + char *pathname = talloc_array(ctx, char, strlen(pathname_in)+2); + const char *s = pathname_in; + char *p = pathname; + bool wrote_slash = false; + + if (pathname == NULL) { return NULL; - } - d = destname; + } - *d++ = '/'; /* Always start with root. */ + /* Always start with a '/'. */ + *p++ = '/'; + wrote_slash = true; while (*s) { - if (*s == '/') { - /* Eat multiple '/' */ - while (*s == '/') { + /* Deal with '/' or multiples of '/'. */ + if (s[0] == '/') { + while (s[0] == '/') { + /* Eat trailing '/' */ s++; } - if ((d > destname + 1) && (*s != '\0')) { - *d++ = '/'; + /* Update target with one '/' */ + if (!wrote_slash) { + *p++ = '/'; + wrote_slash = true; } - start_of_name_component = true; continue; } - - if (start_of_name_component) { - if ((s[0] == '.') && (s[1] == '.') && + if (wrote_slash) { + /* Deal with "./" or ".\0" */ + if (s[0] == '.' && + (s[1] == '/' || s[1] == '\0')) { + /* Eat the dot. */ + s++; + while (s[0] == '/') { + /* Eat any trailing '/' */ + s++; + } + /* Don't write anything to target. */ + /* wrote_slash is still true. */ + continue; + } + /* Deal with "../" or "..\0" */ + if (s[0] == '.' && s[1] == '.' && (s[2] == '/' || s[2] == '\0')) { - /* Uh oh - "/../" or "/..\0" ! */ - - /* Go past the .. leaving us on the / or '\0' */ + /* Eat the dot dot. */ s += 2; - - /* If we just added a '/' - delete it */ - if ((d > destname) && (*(d-1) == '/')) { - *(d-1) = '\0'; - d--; + while (s[0] == '/') { + /* Eat any trailing '/' */ + s++; } - /* - * Are we at the start ? - * Can't go back further if so. + * As wrote_slash is true, we go back + * one character to point p at the slash + * we just saw. */ - if (d <= destname) { - *d++ = '/'; /* Can't delete root */ - continue; + if (p > pathname) { + p--; } - /* Go back one level... */ /* - * Decrement d first as d points to - * the *next* char to write into. + * Now go back to the slash + * before the one that p currently points to. */ - for (d--; d > destname; d--) { - if (*d == '/') { + while (p > pathname) { + p--; + if (p[0] == '/') { break; } } - /* - * Are we at the start ? - * Can't go back further if so. + * Step forward one to leave the + * last written '/' alone. */ - if (d <= destname) { - *d++ = '/'; /* Can't delete root */ - continue; - } + p++; - /* - * We're still at the start of a name - * component, just the previous one. - */ - continue; - } else if ((s[0] == '.') && - ((s[1] == '\0') || s[1] == '/')) { - /* - * Component of pathname can't be "." only. - * Skip the '.' . - */ - if (s[1] == '/') { - s += 2; - } else { - s++; - } + /* Don't write anything to target. */ + /* wrote_slash is still true. */ continue; } } - - if (!(*s & 0x80)) { - *d++ = *s++; - } else { - size_t siz; - /* Get the size of the next MB character. */ - next_codepoint(s,&siz); - switch(siz) { - case 5: - *d++ = *s++; - - FALL_THROUGH; - case 4: - *d++ = *s++; - - FALL_THROUGH; - case 3: - *d++ = *s++; - - FALL_THROUGH; - case 2: - *d++ = *s++; - - FALL_THROUGH; - case 1: - *d++ = *s++; - break; - default: - break; - } - } - start_of_name_component = false; + /* Non-separator character, just copy. */ + *p++ = *s++; + wrote_slash = false; } - *d = '\0'; - - /* And must not end in '/' */ - if (d > destname + 1 && (*(d-1) == '/')) { - *(d-1) = '\0'; + if (wrote_slash) { + /* + * We finished on a '/'. + * Remove the trailing '/', but not if it's + * the sole character in the path. + */ + if (p > pathname + 1) { + p--; + } } - - return destname; + /* Terminate and we're done ! */ + *p++ = '\0'; + return pathname; } diff --git a/source3/modules/test_vfs_widelinks.c b/source3/modules/test_vfs_widelinks.c deleted file mode 100644 index c6055c8c17c..00000000000 --- a/source3/modules/test_vfs_widelinks.c +++ /dev/null @@ -1,88 +0,0 @@ -/* - * Unix SMB/CIFS implementation. - * - * Unit test for widelinks path validator. - * - * Copyright (C) Jeremy Allison 2020 - * - * 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 - * the Free Software Foundation; either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, see <http://www.gnu.org/licenses/>. - */ - -/* Needed for static build to complete... */ -#include "includes.h" -#include "smbd/smbd.h" -NTSTATUS vfs_widelinks_init(TALLOC_CTX *ctx); - -#include "vfs_widelinks.c" -#include <cmocka.h> - -struct str_test_values { - const char *src_str; - const char *dst_str; -} ; - -/* As many nasty edge cases as I can think of.. */ - -static struct str_test_values examples[] = { - { "/", "/" }, - { "/../../", "/" }, - { "/foo/../", "/" }, - { "/./././", "/" }, - { "/./././.", "/" }, - { "/.../././.", "/..." }, - { "/./././.foo", "/.foo" }, - { "/./././.foo.", "/.foo." }, - { "/./././foo.", "/foo." }, - { "/foo/bar/..", "/foo" }, - { "/foo/bar/../baz/", "/foo/baz" }, - { "////////////////", "/" }, - { "/////////./././././.", "/" }, - { "/./.././../.boo/../baz", "/baz" }, - { "/a/component/path", "/a/component/path" }, - { "/a/component/path/", "/a/component/path" }, - { "/a/component/path/..", "/a/component" }, - { "/a/component/../path/", "/a/path" }, - { "///a/./././///component/../////path/", "/a/path" } -}; - -/* - * Test our realpath resolution code. - */ -static void test_resolve_realpath_name(void **state) -{ - unsigned i; - TALLOC_CTX *frame = talloc_stackframe(); - - for (i = 0; i < ARRAY_SIZE(examples); i++) { - char *test_dst = resolve_realpath_name(frame, - examples[i].src_str); - if (test_dst == NULL) { - fail(); - } - assert_string_equal(test_dst, examples[i].dst_str); - TALLOC_FREE(test_dst); - } - TALLOC_FREE(frame); -} - -int main(int argc, char **argv) -{ - const struct CMUnitTest tests[] = { - cmocka_unit_test(test_resolve_realpath_name), - }; - - cmocka_set_message_output(CM_OUTPUT_SUBUNIT); - - return cmocka_run_group_tests(tests, NULL, NULL); -} diff --git a/source3/modules/vfs_widelinks.c b/source3/modules/vfs_widelinks.c index 9a005c9ce3f..36bd3ec6377 100644 --- a/source3/modules/vfs_widelinks.c +++ b/source3/modules/vfs_widelinks.c @@ -102,117 +102,13 @@ #include "includes.h" #include "smbd/smbd.h" +#include "lib/util_path.h" struct widelinks_config { bool active; char *cwd; }; -/* - * Canonicalizes an absolute path, removing '.' and ".." components - * and consolitating multiple '/' characters. As this can only be - * called once widelinks_chdir with an absolute path has been called, - * we can assert that the start of path must be '/'. - */ - -static char *resolve_realpath_name(TALLOC_CTX *ctx, const char *pathname_in) -{ - const char *s = pathname_in; - char *pathname = talloc_array(ctx, char, strlen(pathname_in)+1); - char *p = pathname; - bool wrote_slash = false; - - if (pathname == NULL) { - return NULL; - } - - SMB_ASSERT(pathname_in[0] == '/'); - - while (*s) { - /* Deal with '/' or multiples of '/'. */ - if (s[0] == '/') { - while (s[0] == '/') { - /* Eat trailing '/' */ - s++; - } - /* Update target with one '/' */ - if (!wrote_slash) { - *p++ = '/'; - wrote_slash = true; - } - continue; - } - if (wrote_slash) { - /* Deal with "./" or ".\0" */ - if (s[0] == '.' && - (s[1] == '/' || s[1] == '\0')) { - /* Eat the dot. */ - s++; - while (s[0] == '/') { - /* Eat any trailing '/' */ - s++; - } - /* Don't write anything to target. */ - /* wrote_slash is still true. */ - continue; - } - /* Deal with "../" or "..\0" */ - if (s[0] == '.' && s[1] == '.' && - (s[2] == '/' || s[2] == '\0')) { - /* Eat the dot dot. */ - s += 2; - while (s[0] == '/') { - /* Eat any trailing '/' */ - s++; - } - /* - * As wrote_slash is true, we go back - * one character to point p at the slash - * we just saw. - */ - if (p > pathname) { - p--; - } - /* - * Now go back to the slash - * before the one that p currently points to. - */ - while (p > pathname) { - p--; - if (p[0] == '/') { - break; - } - } - /* - * Step forward one to leave the - * last written '/' alone. - */ - p++; - - /* Don't write anything to target. */ - /* wrote_slash is still true. */ - continue; - } - } - /* Non-separator character, just copy. */ - *p++ = *s++; - wrote_slash = false; - } - if (wrote_slash) { - /* - * We finished on a '/'. - * Remove the trailing '/', but not if it's - * the sole character in the path. - */ - if (p > pathname + 1) { - p--; - } - } - /* Terminate and we're done ! */ - *p++ = '\0'; - return pathname; -} - static int widelinks_connect(struct vfs_handle_struct *handle, const char *service, const char *user) @@ -384,7 +280,10 @@ static struct smb_filename *widelinks_realpath(vfs_handle_struct *handle, config->cwd, smb_fname_in->base_name); } - resolved_pathname = resolve_realpath_name(config, pathname); + + SMB_ASSERT(pathname[0] == '/'); + + resolved_pathname = canonicalize_absolute_path(config, pathname); if (resolved_pathname == NULL) { TALLOC_FREE(pathname); return NULL; diff --git a/source3/modules/wscript_build b/source3/modules/wscript_build index 1f2d2d3acb6..7f056f2b7f7 100644 --- a/source3/modules/wscript_build +++ b/source3/modules/wscript_build @@ -628,8 +628,3 @@ bld.SAMBA3_MODULE('vfs_widelinks', init_function='', internal_module=bld.SAMBA3_IS_STATIC_MODULE('vfs_widelinks'), enabled=bld.SAMBA3_IS_ENABLED_MODULE('vfs_widelinks')) - -bld.SAMBA3_BINARY('test_vfs_widelinks', - source='test_vfs_widelinks.c', - deps='smbd_base cmocka', - for_selftest=True) diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index a536a473cb5..8210e981c35 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -508,10 +508,6 @@ plantestsuite("samba3.test_nfs4_acl", "none", [os.path.join(bindir(), "test_nfs4_acls"), "$SMB_CONF_PATH"]) -plantestsuite("samba3.test_vfs_widelinks", "none", - [os.path.join(bindir(), "test_vfs_widelinks"), - "$SMB_CONF_PATH"]) - plantestsuite("samba3.test_vfs_full_audit", "none", [os.path.join(bindir(), "test_vfs_full_audit"), "$SMB_CONF_PATH"]) diff --git a/source3/torture/torture.c b/source3/torture/torture.c index c3f387cd0e2..aa02163636e 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -14066,6 +14066,25 @@ static bool run_local_canonicalize_path(int dummy) ".././././", ".././././../../../boo", "./..", + "/", + "/../../", + "/foo/../", + "/./././", + "/./././.", + "/.../././.", + "/./././.foo", + "/./././.foo.", + "/./././foo.", + "/foo/bar/..", + "/foo/bar/../baz/", + "////////////////", + "/////////./././././.", + "/./.././../.boo/../baz", + "/a/component/path", + "/a/component/path/", + "/a/component/path/..", + "/a/component/../path/", + "///a/./././///component/../////path/", NULL }; const char *dst[] = { -- Samba Shared Repository