ngclient: prevent using rolenames as filenames if they are dangerous

[MVrachev]

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:

• doesn't contain . and / in its name
• doesn't start with a number that can be misinterpreted as a metadata file name when consistent_snapshot is used.
  See: https://theupdateframework.github.io/specification/latest/#consistent-snapshots

I think the place where this validation should happen is before calling this Updater._download_metadata() here:

python-tuf/tuf/ngclient/updater.py

Line 372 in e6b41d5

data = self._download_metadata(role, length, version)

For context read: #152

This issue description is based on @jku comment here:
#1527 (comment)

[jku]

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)

[jku]

I have a possible, less limiting, approach to this: if someone wants to work on this let's chat

[jku]

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.