fix https://security.snyk.io/vuln/SNYK-JAVA-COMMONSFILEUPLOAD-3326457 potential DoS attack upgrade commons-fileupload to latest version 1.5 add setFileCountMax handle FileCountLimitExceededException update test file add @SuppressWarnings("deprecation") in CorutinesUtils.java order to build project

Comment From: pivotal-cla

@topce Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Comment From: pivotal-cla

@topce Thank you for signing the Contributor License Agreement!

Comment From: bclozel

This PR doesn't fix CVE-2023-24998 by itself, as upgrading merely exposes the new configuration option introduced in commons-fileupload. According to https://security.snyk.io/vuln/SNYK-JAVA-COMMONSFILEUPLOAD-3326457, the option must be manually set in order to avoid security issues.

This PR effectively raises the commons-fileupload baseline to 1.5 as it relies on newly introduced API; as a result, I don't think we can apply this in a maintenance branch like 5.3.x. This support has been removed entirely in Spring Framework 6.0 (see #27423).

I think the most viable options here would be:

Comment From: topce

"This PR doesn't fix https://github.com/advisories/GHSA-hfrx-6qgj-fp6c by itself, as upgrading merely exposes the new configuration option introduced in commons-fileupload. According to https://security.snyk.io/vuln/SNYK-JAVA-COMMONSFILEUPLOAD-3326457, the option must be manually set in order to avoid security issues."

Agree user still need to set new configuration option in order to avoid DoS

Comment From: topce

"This PR effectively raises the commons-fileupload baseline to 1.5 as it relies on newly introduced API; as a result, I don't think we can apply this in a maintenance branch like 5.3.x. This support has been removed entirely in Spring Framework 6.0 (see https://github.com/spring-projects/spring-framework/issues/27423)."

Here not sure , I know that it is removed from 6.0 that why made PR for 5.3.x that is still supported.

Comment From: topce

"I think the most viable options here would be:

override the CommonsFileUploadSupport.newFileUpload method in a subclass and setting the option there use the container-based variant instead"

Can you elaborate more here not sure that understand. I think it is more easy to use new parameter for example multipartResolver.setFileCountMax like before : multipartResolver.setMaxUploadSize multipartResolver.setMaxUploadSizePerFile

Comment From: bclozel

@topce if we introduce this change, this makes commons-fileupload 1.5 a requirement for all applications. This means that an application upgrading from the current 5.3.27 to the next maintenance version 5.3.28 would be broken and would be required to upgrade to commons-fileupload 1.5. We usually don't introduce such changes in maintenance versions.

  • override the CommonsFileUploadSupport.newFileUpload method in a subclass and setting the option there

As described in the reference documentation, you need to create a CommonsMultipartResolver. In this case, one could subclass CommonsMultipartResolver and override its newFileUpload method to set that setFileCountMax option.

As for using the standard Servlet mechanism, this is described in the reference documentation - and is the preferred variant anyway.

Comment From: topce

@bclozel thanks for explanation!

"if we introduce this change, this makes commons-fileupload 1.5 a requirement for all applications. This means that an application upgrading from the current 5.3.27 to the next maintenance version 5.3.28 would be broken and would be required to upgrade to commons-fileupload 1.5. We usually don't introduce such changes in maintenance versions."

Understand but maybe here it is worth to do it because it allow them to address security issue potential DoS attack

Comment From: topce

"As described in the reference documentation, you need to create a CommonsMultipartResolver. In this case, one could subclass CommonsMultipartResolver and override its newFileUpload method to set that setFileCountMax option." Yes but this new property is present in latest version of commons-fileupload 1.5 so anyway they need to upgrade to latest version.

Comment From: topce

"As for using the standard Servlet mechanism, this is described in the reference documentation - and is the preferred variant anyway." Thank you, for this one I did not know. Is this option available in spring framework 5.3.x?

Comment From: bclozel

@topce Yes, the StandardServletMultipartResolver class has been available since Spring Framework 3.1.

Comment From: topce

@topce Yes, the StandardServletMultipartResolver class has been available since Spring Framework 3.1.

@bclozel yes but even there you need to migrate commons-fileupload 1.5 if you want to address potential DoS issue !? As this is security patch I think it is justify to make commons-fileupload 1.5 a requirement for all applications.

Comment From: bclozel

Merging this PR is not enough to secure applications out of the box as the library does not enforce any value by default. Developers will require to manually update their codebase for that.

We will discuss this issue as a team and report back here.

Comment From: topce

OK Thanks!

Comment From: topce

@bclozel FYI I fixed it in my code by upgrading to common-fileupload 1.5 then create class SafeCommonsMultipartResolver that extends CommonsMultipartResolver add setFileCount method and override prepareFileUpload

// fix potential DoS attack
// https://security.snyk.io/vuln/SNYK-JAVA-COMMONSFILEUPLOAD-3326457?utm_medium=Partner&utm_source=RedHat&utm_campaign=Code-Ready-Analytics-2020&utm_content=vuln%2FSNYK-JAVA-COMMONSFILEUPLOAD-3326457
public class SafeCommonsMultipartResolver extends CommonsMultipartResolver {

  private long fileCountMax;

  public void setFileCountMax(final long fileCountMax) {
    this.fileCountMax = fileCountMax;
  }

  @Override
  protected FileUpload prepareFileUpload(@Nullable String encoding) {
    FileUpload actualFileUpload = super.prepareFileUpload(encoding);
    actualFileUpload.setFileCountMax(fileCountMax);
    return actualFileUpload;
  }
}

finally I use new class SafeCommonsMultipartResolver instead CommonsMultipartResolver and limit file count to one because application does not support uploading multiple files ``` java @Bean(name = "multipartResolver") public MultipartResolver multipartResolver(EushareConfiguration esConfig) { SafeCommonsMultipartResolver multipartResolver = new SafeCommonsMultipartResolver();

// prevent Dos attack
// we just upload one file
multipartResolver.setFileCountMax(1);

multipartResolver.setMaxUploadSize(esConfig.getMaxSizeAllowedInBytes());
multipartResolver.setMaxUploadSizePerFile(
    esConfig.getMaxSizeAllowedInBytes());
return multipartResolver;

} ````

Comment From: topce

@bclozel In order to fix issue I need to add new class that inherit Spring class override method add new method use new class instead of Spring class call new method if you accept PR I would just need to add one line of code : multipartResolver.setFileCountMax(1);

That was main motivation for this PR

Comment From: bclozel

We have decided to keep things as is because there is a workaround for this problem (and manual configuration is required anyway). Also, this support has been removed in the next generation of Framework so this is the opportunity for 5.3.x users to migrate to the standard fileupload support.