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
Closes: #1286
Approved by: jlebon

diff --git a/src/libostree/ostree-core-private.h b/src/libostree/ostree-core-private.h
index 22d725a08409e6a20448c45fcfc5591a4587179f..32fe7e51c8bf6f20c803a7c409914f683b4dc4ef 100644 (file)
--- 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 50a2e4fa4b3b92281686f9ea062d76d8c7fd0afc..cee036d83aea5bb8465369c3371ccb7d8112b574 100644 (file)
--- 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 (&regex_initialized))
+    {
+      regex = g_regex_new ("^" OSTREE_REF_FRAGMENT_REGEXP "$", 0, 0, NULL);
+      g_assert (regex);
+      g_once_init_leave (&regex_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 ea670c9c92234d4ec4b9c16c43e0de2c31ce1cf0..efe312d3ce127cb8ae1c513c9e428566330a91cc 100644 (file)
--- 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 b6bfb0ec69476e0ae63591afc7f517efc8e54f92..9289bb37cd7787bb160f509a5b63a15e55363785 100644 (file)
--- 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 da45605cd87c0d81156a74937ca9f60880f76a02..1f0dbdbd7088c72f10f84a0e42729df7a889f7c7 100755 (executable)
--- 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'; 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$"