system-helper: Validate ref arg in RemoveLocalRef method #5048
Conversation
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
| @@ -1299,6 +1300,13 @@ handle_remove_local_ref (FlatpakSystemHelper *object, | |||
| return G_DBUS_METHOD_INVOCATION_HANDLED; | |||
| } | |||
|
|
|||
| ref = flatpak_decomposed_new_from_ref (arg_ref, &error); | |||
There was a problem hiding this comment.
Unfortunately upon looking at this patch again a month later I think it's wrong, because it makes the RemoveLocalRef method error out when arg_ref does not begin with app/ or runtime/, but there's a note on the ref arg of flatpak_dir_remove_ref() which says /* NOTE: Not necessarily a app/runtime ref */, and that's because we sometimes remove other refs, e.g. appstream2/x86_64 or ostree-metadata.
There was a problem hiding this comment.
The flatpak_is_app_runtime_or_appstream_ref() helper would cover the cases where the ref has the prefix app/, runtime/, appstream/, or appstream2/. The only other possible refs I can think of are the deploy/ ones and ostree-metadata.
We could, instead of trying to make an exhaustive list of Flatpak ref patterns, use libostree's ostree_validate_rev, which would give us a little more flexibility to add ref patterns in the future without needing to update the list here.
In any case, I guess this patch needs to miss the boat for 1.14.0, since I'm trying to get that release out in time for Fedora 37 which has beta freeze tomorrow.
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