Filename filtering is inappropriate

[kolbma]

I came across your filename handling and filtering with the CVE-2022-29622 and this issue
#856 (comment)_

First you got blamed also inappropriate by this CVE-2022-29622 whoever is responsible for publishing this without correct approval.
Filenames of forms can have html-tags and js-like-text, like any other form inputs and it is the responsibility of the lib user to handle this, because only he/she knows where this filename is used and what is a safety risc. All this filtering and replacement makes it worse, because the original filename of upload gets lost.

Why I've opened this issue: Now your current code of formidable has some filename filtering which is dysfunctional.

This is an example you can put in e.g. https://replit.com/languages/nodejs
The code is from your current master https://github.com/node-formidable/formidable/blob/master/src/Formidable.js and I've added some asserts to show the problem.

const assert = require('assert');
const path = require('path');

const invalidExtensionChar = (c) => {
  const code = c.charCodeAt(0);
  return !(
    code === 46 || // .
    (code >= 48 && code <= 57) ||
    (code >= 65 && code <= 90) ||
    (code >= 97 && code <= 122)
  );
};

function _getFileName(headerValue) {
  // matches either a quoted-string or a token (RFC 2616 section 19.5.1)
  const m = headerValue.match(
    /\bfilename=("(.*?)"|([^()<>{}[\]@,;:"?=\s/\t]+))($|;\s)/i,
  );
  if (!m) return null;

  const match = m[2] || m[3] || '';
  let originalFilename = match.substr(match.lastIndexOf('\\') + 1);
  originalFilename = originalFilename.replace(/%22/g, '"');
  originalFilename = originalFilename.replace(/&#([\d]{4});/g, (_, code) =>
    String.fromCharCode(code),
  );

  return originalFilename;
}

// able to get composed extension with multiple dots
// "a.b.c" -> ".b.c"
// as opposed to path.extname -> ".c"
function _getExtension(str) {
  if (!str) {
    return '';
  }

  const basename = path.basename(str);
  const firstDot = basename.indexOf('.');
  const lastDot = basename.lastIndexOf('.');
  let rawExtname = path.extname(basename);

  assert(rawExtname === '.txt'); // node knows how to do it

  if (firstDot !== lastDot) {
    rawExtname =  basename.slice(firstDot);
  }

  let filtered;
  const firstInvalidIndex = Array.from(rawExtname).findIndex(invalidExtensionChar);
  if (firstInvalidIndex === -1) {
    filtered = rawExtname;
  } else {
    filtered = rawExtname.substring(0, firstInvalidIndex);
  }
  if (filtered === '.') {
    return '';
  }
  return filtered;
}

let headerValue = 'filename="data-<[email protected]>.txt"';
let filename = _getFileName(headerValue);
let ext = _getExtension(filename);

assert(ext !== '.com', 'wrong extension .com !== .txt: dangerous executable'); 
assert(ext === '.txt'); // all would be fine

[keymandll]

Got here as SCA blocked the build of one of our services due to the CVE referenced earlier.
I think @kolbma is right here: #856 (comment)
It should be OK to upload a file with a name that is supported by the file system. What someone does with the uploaded file after should be of no concern of this library.

I've reached out to both NVD and MEND asking them to look into this and hopefully get the CVE removed as the vulnerability report is literally a joke. The YouTube video attached as proof to the CVE only proves that the person who submitted the vulnerability report is incompetent and that NVD has a serious quality control issue when it comes to vulnerability submissions.

[tunnckoCore]

Thanks for the support @kolbma @keymandll, I think in the very same way.

I just got email from Daniel from Mend, and responded with the same narrative. We are low-level library and we intentionally do it that way, but provide extensions and options. It's a developer's job to ensure that better. We are not end-user service or whatever.

[keymandll]

No worries @tunnckoCore . Great news and big respect to Daniel from Mend for reaching out to you.

I'm still trying to explain to MITRE that the vuln report is false and on MITRE's end there's been a serious misunderstanding re @GrosSacASac 's comment:

#856 (comment)

[ShawnLee2018]

keepExtensions: true
When I upload a file with named "file_1.0.10000.0-1_amd64.deb", it is rename to "3200c4254bbdc91f8e8458b01.0.10000.0".
I think "3200c4254bbdc91f8e8458b01.deb" should be the correct name.

[tunnckoCore]

@ShawnLee2018 updates are near.

Use options.filename: Function for now.

[GrosSacASac]

What is SCA ?

The code from before already removed some characters from the file extension. But not always. So it was inconsistent.
The new code cuts the file extension at the first invalid character (invalid in a file extension).

The characters that are considered invalid inside a file extension are all except the . numbers and a-Z.

Do you want to change that ? Yes/No ?

We can also change the algorithm to filter invalid characters instead of cutting off at the first invalid. Yes/No ?
I believe @ShawnLee2018 that's what you want.

[keymandll]

@GrosSacASac Never mind. I'm perfectly happy now due to the security analysis I performed the industry slowly started realising that Formidable was not vulnerable in the first place.

(SCA is Software Composition Analysis.)