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

Upgrade Checksum algorithms #6634

Merged
merged 2 commits into from
Aug 31, 2018
Merged

Upgrade Checksum algorithms #6634

merged 2 commits into from
Aug 31, 2018

Conversation

ogoffart
Copy link
Contributor

@ogoffart ogoffart commented Jul 5, 2018

The first commit move the checksum code from filesystembase.cpp to checksum.cpp, where they belong. I don't want to add more wrapper around QCryptographicHash in filesystem, ot include QCryptographicHash from there.

The second commit add support for "SHA2" (SHA256) and "SHA3" (SHA3-256).
SHA-1 has already been deprecated for 10 years. We should at least support new algorithm so the server will also be able to support them.

SHA-1 is still the default contents checksum as this is what the server supports and we want to use the same.

To be bikesheded: the name of the string "SHA2" and "SHA3" or should it be "SHA256" and "SHA3-256"

@IljaN What about server support?

@ogoffart ogoffart added this to the 2.6.0 milestone Jul 5, 2018
@ogoffart ogoffart requested a review from ckamm July 5, 2018 13:00
@IljaN
Copy link
Member

IljaN commented Jul 5, 2018

Should be easy to add but nothing planned atm. Also not sure if the security implications of SHA1 also apply to it's usage in non-cryptographic contexts(checksums) as we also still support MD5 and ADLER32 which are even more broken in this regard.

@ogoffart
Copy link
Contributor Author

ogoffart commented Jul 5, 2018

The client is already using the checksum as content hash to detect if the file was changed in order to avoid uploading the file if the checksum is the same. In the future, with #5867, we would even do a COPY instead of re-uploading the files with the same contents.
Of course we only do that if the hash algorithm is safe enough. So we do not do it with Adler32. (Only with MD5 and SHA1)

We probably do not need to worry about a collision attack (or should we?) but if users upload these collision files, we would have a bug right now.

@guruz
Copy link
Contributor

guruz commented Jul 5, 2018

To be bikesheded: the name of the string "SHA2" and "SHA3" or should it be "SHA256" and "SHA3-256"

Isn't the longest string the most accurate one? Especially if the Qt enums are called similarly.
In the German wikipedia it sounds like SHA2 is a generic term for 4(!) algorithmns. https://de.wikipedia.org/wiki/SHA-2

In case, some day, the server also supports it
@ogoffart
Copy link
Contributor Author

ogoffart commented Jul 5, 2018

Right, So i changed the string value to "SHA256" and "SHA3-256".
I could have used "SHA2-256" instead of "SHA256", but the SHA256 is what is how it is normally called.

Also i could have chosen the 224bit variant, but the 256 variant is the most used.

@guruz
Copy link
Contributor

guruz commented Jul 16, 2018

@IljaN @ogoffart Who creates an issue in 'core'?

@IljaN
Copy link
Member

IljaN commented Jul 16, 2018

Should I create a issue for this?

/cc @PVince81

@PVince81
Copy link
Contributor

@IljaN please create a core issue to schedule an upgrade.

@settermjd any thoughts on the above ?

@IljaN
Copy link
Member

IljaN commented Jul 17, 2018

Core issue: owncloud/core#32071

@ogoffart ogoffart merged commit e448f5e into master Aug 31, 2018
@ogoffart ogoffart deleted the checksum branch August 31, 2018 08:41
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

Successfully merging this pull request may close these issues.

5 participants