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

signing plugin: use SHA512 instead of SHA1 when signing artifacts #10543

Merged
merged 1 commit into from Sep 16, 2019

Conversation

vlsi
Copy link
Contributor

@vlsi vlsi commented Sep 10, 2019

PGP signs a digest, so MITM is still possible provided an attacker can update
the artifact in such a way that its SHA1 is intact.

Context

Relevant article is https://medium.com/@jonathan.leitschuh/many-of-these-gpg-signatures-are-signed-with-sha-1-which-is-vulnerable-to-a-second-preimage-attack-67104d827930

As you might know, Chromium does not trust SHA-1 signed certificates: https://www.chromium.org/Home/chromium-security/education/tls/sha-1

Gradle is often used to sign artifacts, so it would be nice to keep its default security level is up to date.

I'm not sure if this change needs to be represented in the documentation / upgrade notice.
AFAIK it should be backward and forward compatible, however it might make sense to clarify that "previous PGP signatures have SHA1 level of security".

I do not add tests in a hope that the code has already been covered.

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

@vlsi
Copy link
Contributor Author

vlsi commented Sep 10, 2019

Interesting case is SHA-512 might be faster on the modern hardware (SHA512 operates on 64 bit words0): https://www.nayuki.io/page/fast-sha2-hashes-in-x86-assembly).

So the checksum might default to SHA512 taking into account the security might be more important than tiny savings in terms of SHA256 vs SHA512

@vlsi
Copy link
Contributor Author

vlsi commented Sep 10, 2019

Just in case, gpg 2.2.11 defaults to SHA256:

$ gpg --version
gpg (GnuPG) 2.2.11
libgcrypt 1.8.4
Copyright (C) 2018 Free Software Foundation, Inc.
$ gpg --detach-sign --armour apache-jmeter-5.2.0-SNAPSHOT_src.tgz

$ pgpdump apache-jmeter-5.2.0-SNAPSHOT_src.tgz.asc
Old: Signature Packet(tag 2)(307 bytes)
...
	Hash alg - SHA256(hash 8)
	Hashed Sub: unknown(sub 33)(21 bytes)
	Hashed Sub: signature creation time(sub 2)(4 bytes)
		Time - Tue Sep 10 22:31:39 MSK 2019

@JLLeitschuh
Copy link
Contributor

Hi @vlsi,

I've been reading about your plugins/work. I really appreciate you pushing the security forward on this. I agree that it's important.

Do you know if Maven Central requires SHA1 or will it be fine if artifacts are uploaded signed with SHA512? I just want to make sure we aren't breaking peoples upload process with these changes.

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2019

@JLLeitschuh , this does not change the set of files that are uploaded to Central or whatever.

This change alters the internal contents of *.asc signature files.

For instance:
https://repo1.maven.org/maven2/org/apache/calcite/calcite-core/1.21.0/

$ curl https://repo1.maven.org/maven2/org/apache/calcite/calcite-core/1.21.0/calcite-core-1.21.0.jar.asc | pgpdump
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   833  100   833    0     0   1626      0 --:--:-- --:--:-- --:--:--  1633
Old: Signature Packet(tag 2)(563 bytes)
	Ver 4 - new
	Sig type - Signature of a binary document(0x00).
	Pub alg - RSA Encrypt or Sign(pub 1)
	Hash alg - SHA512(hash 10)
	Hashed Sub: unknown(sub 33)(21 bytes)
	Hashed Sub: signature creation time(sub 2)(4 bytes)
		Time - Fri Sep  6 15:49:16 MSK 2019
	Sub: issuer key ID(sub 16)(8 bytes)
		Key ID - 0xD77C3383F1927570
	Hash left 2 bytes - 73 5b
	RSA m^d mod n(4092 bits) - ...
		-> PKCS-1

There you see that the signature is using SHA512 digest, however you can clearly see that no .sha512 files are present on Central.

So:

  1. It impacts .asc file generation only
  2. End-users don't always download and verify .asc files, however when they do they use up to date cryptographic libraries that support SHA512 verification.

So I think the default could be just updated, and there's no need to make it configurable.

Does that answer your question?

@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2019

Here's an example of Apache Maven-generated SHA256-based signature:

https://repo1.maven.org/maven2/org/postgresql/postgresql/9.4.1208/

$ curl https://repo1.maven.org/maven2/org/postgresql/postgresql/9.4.1208/postgresql-9.4.1208.jar.asc | pgpdump
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   473  100   473    0     0   1962      0 --:--:-- --:--:-- --:--:--  1962
Old: Signature Packet(tag 2)(284 bytes)
	Ver 4 - new
	Sig type - Signature of a binary document(0x00).
	Pub alg - RSA Encrypt or Sign(pub 1)
	Hash alg - SHA256(hash 8)
	Hashed Sub: signature creation time(sub 2)(4 bytes)
		Time - Fri Feb 19 23:30:41 MSK 2016
	Sub: issuer key ID(sub 16)(8 bytes)
		Key ID - 0x38F47D3E410C47B1
	Hash left 2 bytes - f8 e4
	RSA m^d mod n(2045 bits) - ...
		-> PKCS-1

That artifact was deployed to Maven Central (2016-02), and no-one complained that they can't use pgjdbc (which is the official JDBC driver for PostgreSQL).

@JLLeitschuh
Copy link
Contributor

Hi @vlsi,

This makes sense. No reason not to go all the way to SHA512 in that case. Feel free to update your PR to do so.

One other question. In your opinion, is our use of SHA1 without allowing users to configure that worthy of a CVE on our behalf. Is there a president you can find where this may be a case where it's worth issuing one?

@JLLeitschuh JLLeitschuh self-assigned this Sep 11, 2019
@vlsi
Copy link
Contributor Author

vlsi commented Sep 11, 2019

Well, there's https://www.cvedetails.com/cve/CVE-2005-4900/ , so it probably is worth registering a CVE.

The case here is Gradle does not use consider PGP signatures (yet?), however users might not realize that Gradle-generated PGP signatures are as strong as SHA1 which is not that strong nowdays. So an attacker could replace a file which would still pass PGP signature verification.

PGP signs a digest, so MITM is still possible provided an attacker can update
the artifact in such a way that its SHA1 is intact.

Relevant article is https://medium.com/@jonathan.leitschuh/many-of-these-gpg-signatures-are-signed-with-sha-1-which-is-vulnerable-to-a-second-preimage-attack-67104d827930

Signed-off-by: Vladimir Sitnikov <sitnikov.vladimir@gmail.com>
@vlsi vlsi changed the title signing plugin: use SHA256 instead of SHA1 when signing artifacts signing plugin: use SHA512 instead of SHA1 when signing artifacts Sep 11, 2019
@JLLeitschuh
Copy link
Contributor

Ironic that I wrote that comment about SHA1 no longer being resistant to hash collisions, but I didn't think to check what Gradle itself was using. I really appreciate you following up on this.

CC: @melix This may be relevant to your work.

@melix
Copy link
Contributor

melix commented Sep 11, 2019

This looks like a good change. I'm wondering if it wouldn't be worth adding the sha512 signature to Gradle Metadata, since we compute it... This can be done in a separate PR, though.

@JLLeitschuh
Copy link
Contributor

@melix Can you create a card for this?

@big-guy
Copy link
Member

big-guy commented Sep 14, 2019

@JLLeitschuh could you make sure this gets merged for 6.0?

@big-guy big-guy added this to the 6.0 RC1 milestone Sep 14, 2019
@JLLeitschuh JLLeitschuh merged commit 425b2b7 into gradle:master Sep 16, 2019
@JLLeitschuh
Copy link
Contributor

Working on the CVE issuance now.

@abergmann
Copy link

CVE-2019-16370 was assigned to this issue.

@lptr
Copy link
Member

lptr commented Sep 17, 2019

@JLLeitschuh Is this something we need to mention in the release notes/upgrade guide?

@JLLeitschuh
Copy link
Contributor

@lptr Working on it.

@JLLeitschuh
Copy link
Contributor

@lptr Added to release notes:
99e8657

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.

None yet

6 participants