Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow refs starting with a non-letter or digit #1286

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/libostree/ostree-core-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
25 changes: 24 additions & 1 deletion src/libostree/ostree-core.c
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/libostree/ostree-repo-pull.c
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need this in the next else if for refs_to_fetch, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already one there, no? Line 3879?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops! Indeed.

goto out;
g_hash_table_insert (requested_refs_to_fetch,
ostree_collection_ref_new (collection_id, ref_name),
(*checksum != '\0') ? g_strdup (checksum) : NULL);
Expand Down
8 changes: 8 additions & 0 deletions src/libostree/ostree-repo-refs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
31 changes: 30 additions & 1 deletion tests/test-refs.sh
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ set -euo pipefail

setup_fake_remote_repo1 "archive"

echo '1..2'
echo '1..5'

cd ${test_tmpdir}
mkdir repo
Expand Down Expand Up @@ -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$"
Expand Down