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

nocompressio is vulnerable to swapped chunks in state files #10463

Open
zhaozhongn opened this issue May 17, 2024 · 1 comment
Open

nocompressio is vulnerable to swapped chunks in state files #10463

zhaozhongn opened this issue May 17, 2024 · 1 comment
Labels
type: bug Something isn't working

Comments

@zhaozhongn
Copy link
Contributor

Description

To prevent attack from swapping chunks in state files, the original compressio made each chunk's hash to be based on (data, size, previous_chunk_hash). The nocompressio is instead calculating chunk hash only based on (data, size). This means it will be vulnerable to swapped chunks in state files.

Steps to reproduce

No response

runsc version

No response

docker version (if using docker)

No response

uname

No response

kubectl (if using Kubernetes)

No response

repo state (if built from source)

No response

runsc debug logs (if available)

No response

@zhaozhongn zhaozhongn added the type: bug Something isn't working label May 17, 2024
@EtiennePerot
Copy link
Contributor

I'm not sure what type of threat this would protect against. If an attacker has the ability to modify the checkpoint file, it can also just corrupt it or replace any chunk with whatever it wants. The hash is only good for verifying integrity against random transcription errors; it does not protect against malicious rewriting.

That being said, I see no harm in making it robust against this regardless, so long as it can be done in a high-performance-friendly way (which was the intent of adding nocompressio mode in the first place). Adding in previous_chunk_hash to the computation forces the integrity verification to be serialized, because each expected hash requires checking the previous one. I suggest making the hash be over (data, size, index of chunk within checkpoint) instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants