[MWRAPPER-50] Verify checksum when downloading maven-wrapper.jar

[jira-importer]

Premek Vyhnal opened MWRAPPER-50 and commented

Hi,

Sorry if I just cannot find it

but it seems the checksum is not checked of the maven-wrapper.jar downloaded here:

https://github.com/apache/maven-wrapper/blob/efba2bde13feeabfb42e9dc120e8a35c127baf0d/maven-wrapper-distribution/src/resources/mvnw#L207

 

Checksum of the downloaded file should be checked before executing it to avoid a remote code execution attack on the developer machine.

 

---

Affects: 3.1.0

Issue Links:

• MWRAPPER-79 Automatically add sha256 to maven-wrapper.properties

• MWRAPPER-75 Allow for sha256 checksum verification of downloaded artifacts.

1 votes, 6 watchers

[jira-importer]

Michael Osipov commented

Checksums are not designed to protect against RCEs. Too much Log4J2?

[jira-importer]

Marcin Zajaczkowski commented

Checksums can mitigate some risk with a the wrapper downloaded from local mirror (e.g. on a public CI server) which could be tempered by a malicious party (to still credentials or poise produced artifacts). Of course, it just narrows the scope of a possible attack and definitely it is not a remedy for everything.

However, the implementation should be easier than a verification of Apache's digital signature and it would be worth to have it.

Btw, Gradle, the originator of the wrapper concept, has been supporting it for years - https://docs.gradle.org/current/userguide/gradle_wrapper.html#customizing_wrapper (but unfortunately, still doesn't provided OpenPGP signed artifacts...).

[jira-importer]

Hervé Boutemy commented

PR welcome :)

[jira-importer]

Premek Vyhnal commented

https://github.com/apache/maven-wrapper/blob/efba2bde13feeabfb42e9dc120e8a35c127baf0d/maven-wrapper-distribution/src/resources/mvnw#L207

Here I'd change the if from (pseudo code)

if command wget
  use wget
elif command curl
  use curl
else
  compile and use java
fi

to 

if command wget && command sha1sum
  use wget
  if not sha1sum matches then delete the downloaded file
elif command curl && command sha1sum
  use curl
  if not sha1sum matches then delete the downloaded file
else
  compile and use java
fi 

The java code would also check the checksum

Will this work on mac?

In the windows script there is only one way to download and the checksum may be verified using something like 

1. Windows CMD:
C:\> CertUtil -hashfile C:\file.img MD5 | findstr /v "hash"

1. Windows PowerShell:
PS C:\> $(CertUtil -hashfile C:\file.img MD5)[1] -replace " ","" 

 

We can use sha256sum but sha1 is being published (to see it's the same): https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar.sha1

 

In mvnw script there is @project.version@ replaced (how?) and I suppose the checksum value should get there in the same way. Same with {}MavenWrapperDownloader.java{}.

I just have to figure out how to get the correct checksum value during build. Is it maven-wrapper-distribution that should get the already packaged jar and that fills in the version (and newly the checksum) in the bash script and the java file?

 

Note the "correct" checksum that the downloaded jar file is checked against has to be part of the mvnw script or maven-wrapper.properties, it cannot be downloaded when the checksum is checked from the same server where the jar file is being downloaded from.

[jira-importer]

Marcin Zajaczkowski commented

Thanks Premek Vyhnal  for taking a look at that!

We can use sha256sum but sha1 is being published (to see it's the same): https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar.sha1

In general, sha1 is better than nothing, but from the security point of you both MD5 and SHA1 are "broken" nowadays (the malicious matching artifacts can be prepared in sensible time). Since  May 2021, Nexus has supported also SHA256 as metadata for published artifacts. In fact, a level above, there is also a .sha256 file. Maybe it would be possible to enhance the Maven wrapper building process to publish also sha256/512 checksum files (or course as a separate, somehow independent task)?

However, again, from the security point of view, using checksum files from the same repo (possibly a mirror) generate the similar security risk. An attacker could rpelace both JAR and .sha1 files. OpenGPG signature would be resistant to that, however, using a checksum, probably it should be downloaded from a separate site, such as https://downloads.apache.org/maven/maven-3/3.8.4/binaries/apache-maven-3.8.4-bin.tar.gz.sha512 (available for Apache Maven itself) - unfortunately, for Maven wrapper it doesn't look so good: https://downloads.apache.org/maven/wrapper/

Maybe it could be also improved during the release process?

[jira-importer]

Premek Vyhnal commented

both MD5 and SHA1 are "broken" 

I didn't remember that about sha1 from the top of my head, lets use sha256sum then.

An attacker could replace both JAR and .sha1 files

I didn't mean it like that, see my note that I added later. The link to the .sha1 file was confusing, sorry.

I think the checksum should be hardcoded in the mvnw script somehow, probably in the properties file, filled in by the release process (not sure how exactly that works)

[jira-importer]

Hervé Boutemy commented

the wrapper.jar url comes first from maven-wrapper.properties and there are default values also in mvnw scripts the reference checksums will have to be put along each place where the url can be defined

[jira-importer]

Marcin Zajaczkowski commented

I didn't mean it like that, see my note that I added later.

I replied quickly, and missed your update, sorry for confusion.

I think the checksum should be hardcoded in the mvnw script somehow, probably in the properties file, filled in by the release process (not sure how exactly that works)

Yes, you are right. It works that way also in Gradle. Recently, I've been scripting an automatic local wrapper checksum generation based on the published official checksums, which sidetracked me. In that case, a hardcoded value in the local wrapper .properties file is ok.

[jira-importer]

Slawomir Jaranowski commented

I hope that MWRAPPER-75  will resolve this issue.

[jira-importer]

Premek Vyhnal commented

Yes MWRAPPER-75  looks like it's solving the same issue as I had in mind, but it looks to me that it is disabled by default unless a user enters the checksums into their configuration themselves.

I thought it would be nice if the checksums were generated during build, included in mvnw and always checked by default. If possible..

 

[jira-importer]

Slawomir Jaranowski commented

There is MWRAPPER-79