On Mon, Jan 02, 2017 at 07:06:32PM +0100, Michael Haggerty wrote: > My assumption was that only valid reference names should ever be allowed > to be inserted into a `ref_transaction` entry. But Peff is right that > sometimes the reference name is checked by `refname_is_safe()` rather > than `check_refname_format()`. Let's audit this more carefully... > > * `ref_transaction_add_update()` relies on its caller doing the check > (this fact is documented). Its callers are: > * `ref_transaction_update()` (the usual codepath), which checks the > reference itself using either check_refname_format() or > refname_is_safe() depending on what kind of update it is. > * `split_head_update()` passes the literal string "HEAD". > * `split_symref_update()` picks apart reference updates that go > through existing symbolic references. As such I don't think they > are an attack surface. It doesn't do any checking itself (here > we're talking about its `referent` argument). It has only one > caller: > * `lock_ref_for_update()`, which gets `referent` from: > * `files_read_raw_ref()`, which gets the value either: > * by reading a filesystem-level symlink's contents and > checking it with `check_refname_format()`, or > * reading a symref from the filesystem. In this case, *the > value is not checked*. > > Obviously this chain of custody is disconcertingly long and complicated. > And the gap for symrefs should probably be fixed, even though they are > hopefully trustworthy. Thanks as always for a careful analysis. I agree it seems like a bug that symlinks are checked but symrefs are not. > I think the best thing to do is to drop this patch from the patch > series, and address these checks in a separate series. (There are more > known problems in this area, for example that the checks in > `check_refname_format()` are not a strict superset of the checks in > `refname_is_safe()`.) Sounds like a good plan. I'd be very happy if the "superset" mismatch is fixed. It seems like it has come up in our discussions more than once. -Peff