Metadata API: Delegation role names validation

[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.
They could be ".", "../../filename" or 1.role.
The problem is that at some point those delegation role names are used when constructing an URL used
to download the delegated target metadata file:
https://github.com/theupdateframework/tuf/blob/e9106b59cdb5bbfb4260c5ffc3144e79f8f9596a/tuf/ngclient/updater.py#L287 which is likely to be a problem.

Current behavior:
No validation is used for Delegation role names.

Expected behavior:
Escape special symbols like . or \.

[trishankatdatadog]

Cc @asraa @mnm678 @joshuagl

[jku]

I don't know of specific problems with using these as urls (although those might exist as well), but the obvious problems are in client using the rolename as part of a local file name (this is not just how the reference client is implemented, the spec seems to require doing this): in this client case role names like 1.root or snapshot or ../../filename could easily be pretty bad if we assume an attacker having the ability to create new roles and delegations in the server and then client writing files with filenames fromed from these role names.

I don't think think escaping is enough: we probably should not allow rolenames with any of ['.', '/', '\0'] in them and also should not allow root/snapshot/timestamp/targets as delegated targets role names.

[asraa]

I don't think think escaping is enough: we probably should not allow rolenames with any of ['.', '/', '\0'] in them and also should not allow root/snapshot/timestamp/targets as delegated targets role names.

Ah, yeah, by and large I agree. In the case of Sigstore delegations to projects, e.g. github.com/kubernetes/kubernetes, we were thinking of using the full URL (to avoid e.g. github vs gitlab collisions) as the project name, which would obviously cause problems. I'm really not sure what the right answer is here, if we should sanitize it in the application uniformly by removing the /. I definitely think there should be rolename validation.

[mnm678]

Maybe this should be part of the filesystem API? That way . and /' could be disallowed for unix-like filesystems, but could be allowed, with restrictions for sigstore delegations to git repo urls.

Although, these role names are in signed metadata, so it might be reasonable to just trust the delegator to know what a reasonable role name is for a particular situation.

[jku]

Maybe this should be part of the filesystem API? That way . and /' could be disallowed for unix-like filesystems, but could be allowed, with restrictions for sigstore delegations to git repo urls.

I'm not suggesting banning e.g. dots because filesystems can't cope with them (they certainly can). But if you implement clients/repository tools that use the filesystem to store metadata, you end up with code that writes a lot of files with either "ROLENAME.EXT" or "VERSION.ROLENAME.EXT". So let's assume there is a file 1.root.json in our metadata storage: is that the first version of "root" role or a non-versioned "1.root" role? 🤷

In other words, I don't think this is just a filesystem compatibility issue, I think it's a "this may lead to bugs or vulnerabilities because the spec demands using role name as filename - issue". It could be I'm overreacting but I definitely have to review all of my own file saving code with this in mind...

[jku]

Ah, yeah, by and large I agree. In the case of Sigstore delegations to projects, e.g. github.com/kubernetes/kubernetes, we were thinking of using the full URL (to avoid e.g. github vs gitlab collisions)

Yeah I can see really reasonable use cases here... preventing this does seem bad.

Maybe we need to think about changing the filename aspect instead: either review all the possible conflicting cases WRT versions and names or use something safe as filename (straw man suggestion: rolename hashes as filenames)

[asraa]

So let's assume there is a file 1.root.json in our metadata storage: is that the first version of "root" role or a non-versioned "1.root" role?

Ugh, what a case. Yeah, this turns hairy. You could say we expect that if there's a . then interpret that as a VERSION, but then we get really weird parsing rules like, we expect rolenames to start with an ^[0-9] character, or say we only expect [a-z], _ that end up limiting our possible name and uses of URLs.

safe as filename (straw man suggestion: rolename hashes as filenames)

Yeah interesting! This all be solved by having the delegation object include a "file": <sha>.json that you'd sign over. I guess it doesn't have to be a sha, but the ref implementation could use that.

[trishankatdatadog]

Ugh, what a case. Yeah, this turns hairy. You could say we expect that if there's a . then interpret that as a VERSION, but then we get really weird parsing rules like, we expect rolenames to start with an ^[0-9] character, or say we only expect [a-z], _ that end up limiting our possible name and uses of URLs.

Clients should not store consistent snapshots of metadata with version numbers in the filename, but this may complicate consistent snapshots on the server side. You may see funny things like 1.1.root.json (version 1 of the "1.root" role).

I'm not sure this is a security issue: as long as one cannot forge keys, we should be ok, but need to think about this a bit more.

[jku]

Clients should not store consistent snapshots of metadata with version numbers in the filename [...]

spec say does say that, you are right... but one of the reasons I noticed this was that I was planning to store versioned files for root as it would help with bootstrapping from a "more" trusted root (#1168). Since "1.root" is also a valid rolename, this obviously isn't currently safe -- and I don't think this is immediately obvious to a client developer: it wasn't to me.

I'm not sure this is a security issue: as long as one cannot forge keys, we should be ok, but need to think about this a bit more.

For the client, I'm not yet convinced: A rolename like "../../filename" could mean a badly written client might overwrite a file in my home directory when I just queried some target info, didn't even download anything. I don't think "but that requires repository key compromise" is a good enough explanation at that point.

I'm actually not sure what a well written client should do with role "../../filename"

[trishankatdatadog]

For the client, I'm not yet convinced: A rolename like "../../filename" could mean a badly written client might overwrite a file in my home directory when I just queried some target info, didn't even download anything. I don't think "but that requires repository key compromise" is a good enough explanation at that point.

Agreed. I think we should able to define good rolenames without opening any funny path-traversal attacks.

A regex that allows for simple rolenames but also GitHub repos should work. Might be a bit too restrictive to keep absolutely everybody happy, but better than allowing nothing complicated, and also safe. What does everyone think?

[trishankatdatadog]

And also, obviously, this becomes a https://github.com/theupdateframework/specification rather than implementation-specific issue

[jku]

Collecting some observations from the thread:

• Using arbitrary strings (urls, paths, whatever) as rolenames is a feature and we should try not to limit it
• Using those rolenames 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 rolename as filename. Sanitizing the filename is difficult (as collisions must be prevented)
• Using those rolenames 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 rolename is input from remote that should not be trusted more than is required (the input is verified by threshold of signatures, but still not something to blindly obey if it tells me to overwrite files in my home dir...)
• There are no known security issues with arbitrary rolenames 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?

My thinking right now is that the spec can not both require using rolenames as filenames and say that rolenames are arbitrary strings: it's impossible to implement safely or reliably. In fact spec should not require the storage format anyway: we already know implementations might not use files at all (see Warehouse). Possibly a complementary "implementor notes" document should instead explain the issues and offer advice.

---

As for this immediate Metadata API implemention issue (how do we validate rolenames): I guess we don't (since I believe we don't want to prevent arbitrary strings as rolenames):

• filename validity is the responsibility of the code choosing the filename
• preventing path traversal is also the responsibility of the code choosing the filename/path

We could still prevent delegated roles names being any of the toplevel role names -- it might prevent some weird corner cases from happening

[MVrachev]

Collecting some observations from the thread:

• Using arbitrary strings (urls, paths, whatever) as rolenames is a feature and we should try not to limit it
• Using those rolenames 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 rolename as filename. Sanitizing the filename is difficult (as collisions must be prevented)
• Using those rolenames 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 rolename is input from remote that should not be trusted more than is required (the input is verified by threshold of signatures, but still not something to blindly obey if it tells me to overwrite files in my home dir...)
• There are no known security issues with arbitrary rolenames 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?

My thinking right now is that the spec can not both require using rolenames as filenames and say that rolenames are arbitrary strings: it's impossible to implement safely or reliably. In fact spec should not require the storage format anyway: we already know implementations might not use files at all (see Warehouse). Possibly a complementary "implementor notes" document should instead explain the issues and offer advice.

As for this immediate Metadata API implemention issue (how do we validate rolenames): I guess we don't (since I believe we don't want to prevent arbitrary strings as rolenames):

• filename validity is the responsibility of the code choosing the filename
• preventing path traversal is also the responsibility of the code choosing the filename/path

We could still prevent delegated roles names being any of the toplevel role names -- it might prevent some weird corner cases from happening

@jku I agree with all of your thoughts here.
To summarize we will:

• validate that role names are not using top-level metadata names in Metadata API: Metadata API: prevent Delegation role names to be one of top level metadata roles #1558
• validate that role names are not used as file names in ngclient: ngclient: prevent using rolenames as filenames if they are dangerous #1562

With this, I think we have specified this issue scope and split it into two.
I don't think we need this issue anymore.
Does somebody disagrees and am I missing something?

[MVrachev]

@jku do you agree we can close this issue?

The item left is validating that DelegatedRole names are not top-level metadata names. This is discussed in #1558.

[jku]

yeah agreed.

• using rolenames as filenames is dangerous, but we've decided to address this at the point where filenames are formed, and to not limit rolenames in general
• we can still limit delegated rolenames to not be one of top level role names as in 1558 (but this is just a belt-and-suspenders approach: client is not vulnerable to this)