Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

max root rotations is way too small #2672

Closed
jku opened this issue Jul 15, 2024 · 7 comments · Fixed by #2675
Closed

max root rotations is way too small #2672

jku opened this issue Jul 15, 2024 · 7 comments · Fixed by #2675
Labels
client Related to the client (updater) implementation

Comments

@jku
Copy link
Member

jku commented Jul 15, 2024

Our default configuration is max_root_rotations=32.

This is really quite small: a real world repository can reach this number if they are doing a bit of experimentation and then the default client can no longer update with a single refresh call (and the issue is hard to debug as refresh() may actually succeed but client does not yet have the most current root).

To start the discussion: 500 rotations would allow a weekly root signing (not something I'd recommend but not completely ludicrous) for almost 10 years without clients shipping with a newer root... That would be 0.25 GB of root metadata at most per client refresh (this is assuming a malicious repository that stuffs every version to max size). That sounds like something that should not be a critical issue to most computers today. Systems running on smaller embedded devices can modify the config of course.

Does 500 sound like too much as default?

CC @lukpueh @kommendorkapten

@jku jku added the client Related to the client (updater) implementation label Jul 15, 2024
@lukpueh
Copy link
Member

lukpueh commented Jul 15, 2024

Hard to tell. Do I understand correctly, the attacker has root key access, but "only" tries to crash the client? Is that realistic? Also, it looks like setting "max root rotation" is not covered by the specification. I guess 500 is fine.

@kommendorkapten
Copy link
Member

500 is ok, I did create a PR against go-tuf with 256, I can change that to 500 too, I don't have any strong opinions on the exact value as long as it's "large enough".

@jku
Copy link
Member Author

jku commented Jul 15, 2024

Do I understand correctly, the attacker has root key access, but "only" tries to crash the client? Is that realistic?

Yeah that's the thing... needs a bit of imagination. For an embedded device bricking it by filling the disk might be a thing (would have to assume TUF is used for something that isn't interesting to attacker).

500 is ok, I did create a PR against go-tuf with 256

256 sounds reasonable too, we can go with that.

@JustinCappos
Copy link
Member

JustinCappos commented Jul 15, 2024 via email

@kommendorkapten
Copy link
Member

I'm in favor of this being higher rather than lower so that people don't accidentally bump up against the limit (which would be horrible)

Agree, this is the exact scenario we just ran into (in one of our staging TUF repos) when verifying the integrity from 1.root.json.

I'd also like to check that this is per-repo?

Yes, that's how it would work, as each repo would use it's own updater which is where the configuration option exists.

@JustinCappos
Copy link
Member

JustinCappos commented Jul 15, 2024 via email

@kommendorkapten
Copy link
Member

Not having a limit is not optimal I think. Having a limit would at least get the client to halt, which I think is a valuable signal (theoretically the client will halt at some point in time, but with a limit it's easier to control and manage). Even if the client wont be able to continue normal operation, fail fast is preferable IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Related to the client (updater) implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants