You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
ngclient: prevent using rolenames as filenames if they are dangerous #1562
Description of issue or feature request:
Delegation role names are not restricted in any way in the spec, but they are targets metadata role names.
This means they can be any random string. In Sigstore, for example, they are thinking of using delegations role names as URLs to avoid collisions (see #1527 (comment)).
We don't want to limit that behavior.
Still, using those role names as filenames is problematic (for both repository and client) as:
not all strings are actually valid filenames - but the spec at times requires using the role name as a file name. Sanitizing the filename is difficult (as collisions must be prevented)
Using those role names as filenames is also problematic because it can lead to path traversal with roles like ../../../filename. This is especially an issue for the client as role name is input from remote that should not be trusted more than is required
There are no known security issues with arbitrary role names as files and version numbers, but the area seems ripe for them: is 1.role.json the metadata for the role role or the role 1.role?
Current behavior:
Currently, we are using role names as file names without any checks.
Expected behavior:
A possible solution could be validating that delegated role name:
Looks good, just wanted to add that we would like to support arbitrary rolenames in ngclient but we just don't have a solution right now that is both:
clearly spec compliant and
safe
So we're trying to compromise here: keep using rolenames as filenames, but error out if those rolenames look like they might be unsafe as filenames. I think we should also at some point prototype with not using rolenames as filenames to show it works but this seems to be in conflict with the specification...
I think the place where this validation should happen is before calling this Updater._download_metadata() here:
Yeah that will work. The limitations are:
must happen before the file writing in _persist_metadata()
should happen only to roles that we plan to write to disk (so e.g. we should not validate every child role name in _preorder_depth_first_walk(), only the ones we end up downloading)
it could even be a validation step inside _download_metadata() for consistency (we know top-level metadata rolenames are valid but doesn't hurt to check)
Took a while but this got fixed in GHSA-wjw6-2cqr-j4qr in 0.19.0: rolenames are not limited but they are encoded before using them in filenames. Thank you martin for pushing on the issue.
Activity
jku commentedon Sep 3, 2021
Looks good, just wanted to add that we would like to support arbitrary rolenames in ngclient but we just don't have a solution right now that is both:
So we're trying to compromise here: keep using rolenames as filenames, but error out if those rolenames look like they might be unsafe as filenames. I think we should also at some point prototype with not using rolenames as filenames to show it works but this seems to be in conflict with the specification...
Yeah that will work. The limitations are:
_persist_metadata()_preorder_depth_first_walk(), only the ones we end up downloading)it could even be a validation step inside
_download_metadata()for consistency (we know top-level metadata rolenames are valid but doesn't hurt to check)jku commentedon Sep 11, 2021
I have a possible, less limiting, approach to this: if someone wants to work on this let's chat
jku commentedon Oct 28, 2021
Took a while but this got fixed in GHSA-wjw6-2cqr-j4qr in 0.19.0: rolenames are not limited but they are encoded before using them in filenames. Thank you martin for pushing on the issue.