'How to resolve External Control of File Name or Path (CWE ID 73)

I am working on fixing Veracode issues in my application. Veracode has highlighted the flaw "External Control of File Name or Path (CWE ID 73) " in below code. 

Thread.currentThread().getContextClassLoader().getResourceAsStream(lookupName)

How do I validate the parameter? If I need to use below ESAPI validation, then what is the exact parameter I should be passing in getValidFileName() method. Currently I am passing the parameters as below.

ESAPI.validator().getValidFileName(lookupName, lookupName,
          ESAPI.securityConfiguration().getAllowedFileExtensions(), false);

Correct me whether I am following the right approach for fixing this issue.

javasecurityesapiveracode


Solution 1:[1]

There are several suggestions at: https://community.veracode.com/s/article/how-do-i-fix-cwe-73-external-control-of-file-name-or-path-in-java

You can use hardcoded values, if these files are stored in the server side. (i.e.: in a HashMap).

Another solution is to use a custom validator (from veracode page) :

// GOOD Code
String extension = request.getParameter("extension");
File f = new File(buildValidAvatarPath(extension))

@FilePathCleanser
public String buildValidAvatarPath(extension) {
  String[] allowedExtensions = new String[]{"jpg","gif","png"};
  String extension = "png"; // Default extension
  for (String allowedExtension: allowedExtensions) {
    if (allowedExtension.equals(request.getParameter("extension"))) {
      extension = request.getParameter("extension");
    }
  }
  // See "Note on authorization"
  User user = getCurrentUser();
  if (!userMayAccessFile(user, path)) {
    throw new AuthorizationException("User may not access this file", user);
  }
  File(configPath + "avatar." + extension)

  return path;
}

Solution 2:[2]

Okay, so the problem is that you are allowing user-control of that file path. Imagine its on a UNIX box and they enter:

../../../../../../../etc/shadow

Whatever user privileges are granted to the user running that java Thread is possible to expose to the user in question. I don't know what processing is going on in your application, but the danger is that you need to prevent user control of that lookup variable.

The call you're making is consistent with the single test in ValidatorTest.java, which is definitely a deficiency in code coverage on our behalf.

Now, there's an excellent chance that even if you use this call that Veracode might still flag it: the default file list in ESAPI.properties will need to be either truncated for your use case, or you'll have to create your own Validator rule for legal file extensions for your specific use case.

Which brings up the next bit: There's a lot of mischief that can happen in regards to file uploads.

In short, to be actually secure about file uploads will require more than what ESAPI currently offers, which is unfortunately, only an extension check. In your particular case, make sure you try some directory traversal attacks. And use that OWASP link to help analyze your application.

Given that the OP wants to clear the issue in Veracode, you would want to chain a couple calls:

ESAPI.validator().getValidDirectoryPath() and ESAPI.Validator.getValidFileName()

But be sure you've properly truncated the extension list in HttpUtilities.ApprovedUploadExtensions in validator.properties as the default list is too permissive, at least until we release 2.1.0.2.

I have to stress however that even with this particular combination there is absolutely nothing ESAPI does to prevent a user from renaming "netcat.exe" to "puppies.xlsx" and bypassing your validation check, that's why the rant on the first part of this answer.

ESAPI's file validation is NOT secure, it's quite simply better than nothing at all.

Doing this correctly requires more work than just using 1-2 calls to ESAPI.

DISCLAIMER: as of this writing I am the project co-lead for ESAPI.

Solution 3:[3]

You can change file name by sanitizing it as below code snippet:

private static String sanitizeFileName(String name) {
    return name
            .chars()
            .mapToObj(i -> (char) i)
            .map(c -> Character.isWhitespace(c) ? '_' : c)
            .filter(c -> Character.isLetterOrDigit(c) || c == '-' || c == '_'  || c == ':')
            .map(String::valueOf)
            .collect(Collectors.joining());
}

Sources

This article follows the attribution requirements of Stack Overflow and is licensed under CC BY-SA 3.0.

Source: Stack Overflow

Solution Source
Solution 1 devwebcl
Solution 2
Solution 3 Margi212

Related Questions