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

Tus client uses a deprecated cryptographic function to calculate the file checksums #157

Closed
sydseter opened this issue Nov 22, 2021 · 4 comments

Comments

@sydseter
Copy link

SHA-1 is not collision-resistant, which makes it easier for context-dependent attackers to conduct tampering attacks and alter the checksum which makes it possible to alter the file being uploaded itself. For a long time, it has been possible "to find collisions for SHA1 and that thus it is not secure to use for digital signatures, file integrity, and file identification purposes". see: https://arstechnica.com/information-technology/2017/02/at-deaths-door-for-years-widely-used-sha1-function-is-now-dead/

Also:
https://www.cvedetails.com/cve/CVE-2005-4900/
https://cwe.mitre.org/data/definitions/328.html

Finding:

https://github.com/tusdotnet/tusdotnet/blob/ca23bdd88f5b63545c6fb9c2ed18b12984da3078/Source/tusdotnet/Extensions/Internal/FileStreamExtensions.cs

var calculateSha1 = dataStream.CalculateSha1(chunkStartPosition);

Task<IEnumerable<string>> GetSupportedAlgorithmsAsync(CancellationToken cancellationToken);

return Task.FromResult<IEnumerable<string>>(new[] { "sha1" });

https://github.com/tusdotnet/tusdotnet/blob/c3f6f93bd3f0c76a5fd6572835a8d0f5f15909db/Source/tusdotnet/Helpers/ChecksumTrailerHelper.cs
internal static readonly Checksum TrailingChecksumToUseIfRealTrailerIsFaulty = new("sha1", new byte[20]);

@sydseter
Copy link
Author

Yes, if the checksum only is used to check for data corruption, then it's not a security issue.

@smatsson
Copy link
Collaborator

Thanks for reporting. I'm aware of the issues with sha1 for security purposes but in this case the checksum is only used for verification of chunks sent by the client to make sure it has not been corrupted in transfer. The tus spec requires us to have support for sha1:

The Server MUST support at least the SHA1 checksum algorithm identified by sha1

Source: https://tus.io/protocols/resumable-upload.html#checksum

To be able to use this as an exploit one would have to generate a sha1 that is broken for the exact chunk of the file being transferred by the client which I find unlikely. Adding support for sha256 and sha512 is not a big deal but in my experience very few clients use this feature.

@smatsson
Copy link
Collaborator

smatsson commented Dec 7, 2021

No activity for 14 days

@smatsson smatsson closed this as completed Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants