From afe3f0c1122e102e5f1ace2a8d5b0b1a2080f200 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 17 Oct 2017 20:53:27 -0400 Subject: [PATCH] Disallow refs starting with a non-letter or digit Change the regexp for validating refs to require at least one letter or digit before allowing the other special chars in the set `[.-_]`. Names that start with `.` are traditionally Unix hidden files; let's ignore them under the assumption they're metadata for some other tool, and we don't want to potentially conflict with the special `.` and `..` Unix directory entries. Further, names starting with `-` are problematic for Unix cmdline option processing; there's no good reason to support that. Finally, disallow `_` just on general principle - it's simpler to say that ref identifiers must start with a letter or digit. We also ignore any existing files (that might be previously created refs) that start with `.` in the `refs/` directory - there's a Red Hat tool for content management that injects `.rsync` files, which is why this patch was first written. V1: Update to ban all refs starting with a non-letter/digit, and also add another call to `ostree_validate_rev` in the pull code. Closes: https://github.com/ostreedev/ostree/issues/1285 --- src/libostree/ostree-core-private.h | 5 +++++ src/libostree/ostree-core.c | 25 ++++++++++++++++++++++- src/libostree/ostree-repo-pull.c | 2 ++ src/libostree/ostree-repo-refs.c | 8 ++++++++ tests/test-refs.sh | 31 ++++++++++++++++++++++++++++- 5 files changed, 69 insertions(+), 2 deletions(-) diff --git a/src/libostree/ostree-core-private.h b/src/libostree/ostree-core-private.h index 22d725a084..32fe7e51c8 100644 --- a/src/libostree/ostree-core-private.h +++ b/src/libostree/ostree-core-private.h @@ -128,6 +128,11 @@ static inline char * _ostree_get_commitpartial_path (const char *checksum) return g_strconcat ("state/", checksum, ".commitpartial", NULL); } +gboolean +_ostree_validate_ref_fragment (const char *fragment, + GError **error); + + gboolean _ostree_validate_bareuseronly_mode (guint32 mode, const char *checksum, diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c index 50a2e4fa4b..cee036d83a 100644 --- a/src/libostree/ostree-core.c +++ b/src/libostree/ostree-core.c @@ -142,7 +142,10 @@ ostree_validate_checksum_string (const char *sha256, return ostree_validate_structureof_checksum_string (sha256, error); } -#define OSTREE_REF_FRAGMENT_REGEXP "[-._\\w\\d]+" +/* This used to allow leading - and ., but was changed in + * https://github.com/ostreedev/ostree/pull/1286 + */ +#define OSTREE_REF_FRAGMENT_REGEXP "[\\w\\d][-._\\w\\d]*" #define OSTREE_REF_REGEXP "(?:" OSTREE_REF_FRAGMENT_REGEXP "/)*" OSTREE_REF_FRAGMENT_REGEXP #define OSTREE_REMOTE_NAME_REGEXP OSTREE_REF_FRAGMENT_REGEXP @@ -196,6 +199,26 @@ ostree_parse_refspec (const char *refspec, return TRUE; } +gboolean +_ostree_validate_ref_fragment (const char *fragment, + GError **error) +{ + static GRegex *regex; + static gsize regex_initialized; + if (g_once_init_enter (®ex_initialized)) + { + regex = g_regex_new ("^" OSTREE_REF_FRAGMENT_REGEXP "$", 0, 0, NULL); + g_assert (regex); + g_once_init_leave (®ex_initialized, 1); + } + + g_autoptr(GMatchInfo) match = NULL; + if (!g_regex_match (regex, fragment, 0, &match)) + return glnx_throw (error, "Invalid ref fragment '%s'", fragment); + + return TRUE; +} + /** * ostree_validate_rev: * @rev: A revision string diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index ea670c9c92..efe312d3ce 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -3853,6 +3853,8 @@ ostree_repo_pull_with_options (OstreeRepo *self, while (g_variant_iter_loop (collection_refs_iter, "(&s&s&s)", &collection_id, &ref_name, &checksum)) { + if (!ostree_validate_rev (ref_name, error)) + goto out; g_hash_table_insert (requested_refs_to_fetch, ostree_collection_ref_new (collection_id, ref_name), (*checksum != '\0') ? g_strdup (checksum) : NULL); diff --git a/src/libostree/ostree-repo-refs.c b/src/libostree/ostree-repo-refs.c index b6bfb0ec69..9289bb37cd 100644 --- a/src/libostree/ostree-repo-refs.c +++ b/src/libostree/ostree-repo-refs.c @@ -560,6 +560,14 @@ enumerate_refs_recurse (OstreeRepo *repo, if (dent == NULL) break; + /* https://github.com/ostreedev/ostree/issues/1285 + * Ignore any files that don't appear to be valid fragments; e.g. + * Red Hat has a tool that drops .rsync_info files into each + * directory it syncs. + **/ + if (!_ostree_validate_ref_fragment (dent->d_name, NULL)) + continue; + g_string_append (base_path, dent->d_name); if (dent->d_type == DT_DIR) diff --git a/tests/test-refs.sh b/tests/test-refs.sh index da45605cd8..260baf12dd 100755 --- a/tests/test-refs.sh +++ b/tests/test-refs.sh @@ -23,7 +23,7 @@ set -euo pipefail setup_fake_remote_repo1 "archive" -echo '1..2' +echo '1..5' cd ${test_tmpdir} mkdir repo @@ -88,6 +88,35 @@ if ${CMD_PREFIX} ostree --repo=repo refs foo/ctest --create=ctest; then assert_not_reached "refs --create unexpectedly succeeded in overwriting an existing prefix!" fi +# https://github.com/ostreedev/ostree/issues/1285 +# One tool was creating .latest_rsync files in each dir, let's ignore stuff like +# that. +echo '👻' > repo/refs/heads/.spooky_and_hidden +${CMD_PREFIX} ostree --repo=repo refs > refs.txt +assert_not_file_has_content refs.txt 'spooky' +${CMD_PREFIX} ostree --repo=repo refs ctest --create=exampleos/x86_64/standard +echo '👻' > repo/refs/heads/exampleos/x86_64/.further_spooky_and_hidden +${CMD_PREFIX} ostree --repo=repo refs > refs.txt +assert_file_has_content refs.txt 'exampleos/x86_64/standard' +assert_not_file_has_content refs.txt 'spooky' +rm repo/refs/heads/exampleos -rf +echo "ok hidden refs" + +for ref in '.' '-' '.foo' '-bar' '!' '!foo' '🎲emoji'; do + if ${CMD_PREFIX} ostree --repo=repo refs ctest --create=${ref} 2>err.txt; then + fatal "Created ref ${ref}" + fi + assert_file_has_content err.txt 'Invalid ref' +done +echo "ok invalid refs" + +for ref in 'org.foo.bar/x86_64/standard-blah'; do + ostree --repo=repo refs ctest --create=${ref} + ostree --repo=repo rev-parse ${ref} >/dev/null + ostree --repo=repo refs --delete ${ref} +done +echo "ok valid refs with chars '._-'" + #Check to see if any uncleaned tmp files were created after failed --create ${CMD_PREFIX} ostree --repo=repo refs | wc -l > refscount.create1 assert_file_has_content refscount.create1 "^5$"