Disallow refs starting with a non-letter or digit #1286
Conversation
tests/test-refs.sh
Outdated
| # One tool was creating .latest_rsync files in each dir, let's ignore stuff like | ||
| # that. | ||
| echo '👻' > repo/refs/heads/.spooky_and_hidden | ||
| ostree --repo=repo refs > refs.txt |
There was a problem hiding this comment.
Missing ${CMD_PREFIX} here and below!
|
We already do validate creating refs AFAICS, this is just another tool dropping files in there. |
|
What I mean is: We should have errored out there, right? |
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: ostreedev#1285
cgwalters
force-pushed
the
refs-robust
branch
from
0527566
to
afe3f0c
Compare
cgwalters
changed the title
. in refs/
|
Ah. Yes...well that definitely increases the scope of this, but I think you're right. Reworked in ⬆️ |
|
Hum, we currently accept anything that GRegex allows as |
| @@ -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)) | |||
There was a problem hiding this comment.
We also need this in the next else if for refs_to_fetch, right?
There was a problem hiding this comment.
There's already one there, no? Line 3879?
|
⚡ Test exempted: pull fully rebased and already tested. |
|
This is breaking stuff in flatpak due to the use of "${remote}:." as argument to ostree_repo_list_refs to find all local refs pulled from a particular remote. |
|
Hmm, we should just be able to support |
This patch could be important in case the ref arg was maliciously crafted to try to convince flatpak-system-helper to delete an arbitrary file on the filesystem. However, in practice (a) recent versions of libostree will not accept such a ref name which has e.g. "../" in it thanks to ostreedev/ostree#1286, and (b) even on ancient versions of Flatpak that use a version of libostree without the aforementioned patch, the exploit does not appear to be successful, at least on Debian 9. See GHSA-45jq-5658-v38x
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 startwith
.are traditionally Unix hidden files; let's ignore them under theassumption 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 optionprocessing; there's no good reason to support that. Finally, disallow
_juston 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 therefs/directory - there's a Red Hat tool for contentmanagement that injects
.rsyncfiles, which is why this patch was firstwritten.
V1: Update to ban all refs starting with a non-letter/digit, and
also add another call to
ostree_validate_revin the pullcode.
Closes: #1285