Vulnerability CVE-2022-29622 is reported by Whitesource

[johnchen05]

https://www.whitesourcesoftware.com/vulnerability-database/CVE-2022-29622

[GrosSacASac]

Conditions to be vulnerable

• eval (user input) file name as code (for example unescaped in html)
• use the keepextension option
• use linux or ios (where <> are valid file chars)
• not using the filename option , or using it without validating user input
• use version 3.2.3 or earlier

Please install version 3.2.4 to have safer defaults

[andyedwardsibm]

Is there an official view of whether the v2 stream is also affected? The latest on https://www.npmjs.com/package/formidable right now is 2.0.1 and the advisory at GHSA-8cp3-66vr-3r4c has no information on the version ranges affected. Looking at #857 and

formidable/src/Formidable.js

Line 527 in 48521d7

_getExtension(str) {

it looks like it is

[GrosSacASac]

v2 has other vulnerabilities as well,
thank you for reminding me to tag v3 as latest, I will do it soon

[krsubbar]

@GrosSacASac I still see 2.01 as the version at https://www.npmjs.com/package/formidable . Can we mark v3 as latest as you suggested above?

[wxhthx]

@GrosSacASac As I fond the v3.2.4 ver on npm, When can we make the v3.2.4 tag on github ? We need the github tag

[kolbma]

WTF...?!
@GrosSacASac
Where is here the problem, when someone uploads a valid filename to the system with HTML-Tags and JS in it?

Doesn't the problem only exist in any frontend application showing filenames from the system unfiltered in any browser etc...?!

With this fix you have broken a valid usecase. Invalid filenames are only invalid if the filesystem says they are invalid and fs-module throws exceptions.

Do I miss the real problem here?

[GrosSacASac]

Doesn't the problem only exist in any frontend application showing filenames from the system unfiltered in any browser etc...?!

Correct.

The fix is to help people have safe default even if they blind copy paste code from stack-overflow for example.

If you want to have any filename possible, you can still do it by using the explicit filename option.

Having safe defaults is a principle I adhere to.

[kolbma]

Having safe defaults is a principle I adhere to.

Hopefully you will not get the idea of filename length or special unicode might also be a problem in some of the many client applications and operating systems out there. Never ending story if you'd like to hit this on the wrong side.
Did ever before came anybody to the thoughts to limit the capability of scp, what filenames it may accept because somebody could show a textual directory listing of the server per web application?

[GrosSacASac]

There was already removal of invalid characters from filename in the code since 2011-2010.
If you look at the Pr you will see I only made it work 100% of the time. I did not add it.

[kolbma]

There was already removal of invalid characters from filename in the code since 2011-2010. If you look at the Pr you will see I only made it work 100% of the time. I did not add it.

Sorry, didn't mean you (@GrosSacASac) with this "you". Your "Improve keep extension" is good purpose.
I'm just thinking of this CVE is a joke.

But now I'm curious...
I've had a fast look at

formidable/src/Formidable.js

Lines 513 to 538 in 143e473

	_getExtension(str) {
	if (!str) {
	return '';
	}
	const basename = path.basename(str);
	const firstDot = basename.indexOf('.');
	const lastDot = basename.lastIndexOf('.');
	let rawExtname = path.extname(basename);
	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;
	}

Shouldn't the extension start with the lastDot?

formidable/src/Formidable.js

Line 524 in 143e473

rawExtname = basename.slice(firstDot);

A filename data-<[email protected]>.txt.
The extension is txt and not com>.txt which would become filtered to com (which is an executable in MS-DOS/Windows).
The bug is the idea of this line...

formidable/src/Formidable.js

Line 510 in 143e473

// able to get composed extension with multiple dots

OS handles files by the extension after the lastDot!

And formidable makes from a valid uploaded txt-file an executable com file, great improvement ;-)

[GrosSacASac]

Open an issue if you think that it is a problem

[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: HERE
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.

The CVE looks to me like yet another overly eager "security professional" trying to get a CVE attached to his/her name by ignoring context and understanding of responsibilities. Then, all the other lemmings just accept it and pick it up without actually looking into the reported "issue".

[keymandll]

Conditions to be vulnerable

• eval (user input) file name as code (for example unescaped in html)
• use the keepextension option
• use linux or ios (where <> are valid file chars)
• not using the filename option , or using it without validating user input
• use version 3.2.3 or earlier

Please install version 3.2.4 to have safer defaults

@GrosSacASac I have submitted a request to MITRE to remove the CVE assigned as some of us believe the library is not vulnerable. The MITRE guys decided that just because a contributor, it this case specifically referring to you admitted that the library is vulnerable under certain circumstances (see quoted above) they refuse the remove the CVE.

To help everyone better understand the "issue" could you please provide more context re your comments, following these questions:

1. RE: "Conditions to be vulnerable" - do you mean if these conditions are met then formidable is vulnerable or all these conditions are related to an application that may insecurely use formidable?
2. RE: "eval (user input) file name as code (for example unescaped in html)" - Are you referring to this library specifically or another application using this library that insecure may eval() the file name? (I have not seen a single line with eval in this library.
3. RE: "use the keepextension option" - When using this option, will it result in a vulnerability that is affecting formidable or you were also referring to another, insecurely implemented application that may use formidable?
4. RE: "use linux or ios (where <> are valid file chars)" - Do you consider this library being responsible for file systems supporting <> characters in the file name?
5. RE: "not using the filename option , or using it without validating user input" - Do you consider some users not using the filename option of the library a direct fault of the library? Any chance that it may be the users fault not reading the documentation?

In general, are you seriously considering this library to be vulnerable?

@tunnckoCore @kolbma You guys may be also interested is this.

[GrosSacASac]

I'll be back in 30 days, consider the filename option if it is blocking

[salielim]

Will this vulnerability fix be backported to v2.x since v2 is still maintained?
Some libraries cannot upgrade to v3 because the latest version of formidable is ESM only (e.g. ladjs/superagent#1724)

[keymandll]

I have written up my analysis of the "vulnerability" (I don't think there is a vulnerability) here if anyone is interested: https://medium.com/@zsolt.imre/cve-2022-29622-in-vulnerability-analysis-5cf783c3721

I personally could not find any indication of any of the vulnerabilities mentioned in the CVE to be present. The information disclosed by the CVE and the YT video referenced is not sufficient to prove the library is affected by the vuln. If anyone can come up with an exploit to prove there is an arbitrary code execution vulnerability in formidable, please share. Otherwise, I do believe the CVE is invalid.

[tunnckoCore]

Uh... 🤦‍♂️ that's continuing... @keymandll thank you.

@salielim there's nothing to be backported in reality. the codebase between v3 and v2 is almost the same, and especially in that regard. The options are there and the documentation is there. I really don't like this getExtension function, but that's it for now.

[titanism]

For most packages like superagent (and related supertest), an ESM/CJS version release will support the upgrade and these false vulnerability notices will go away. Thank you all for your help. 🙇

[titanism]

We've reached out to some folks at Snyk to see if we can decrease the severity or remove the CVE altogether. In the interim, please ping us once you have released a new version with CJS/ESM support @tunnckoCore and @keymandll and most of these issues should go away. Thanks!

[tunnckoCore]

Yep, me and others also reached Mend to remove it, cuz it's nothing.

[dpace-cs]

Check this too https://medium.com/@zsolt.imre/cve-2022-29622-in-vulnerability-analysis-5cf783c3721

I had read the article and completely agree with author's assessment that remote execution is not possible, at least not with formidable alone. The NVD remote-execution assessment and corresponding CVSS score of 9.8 appear incorrect. However, this should still be viewed as a CVSS 7.1 (High) vulnerability, although I'll admit the best exploit I can come up with right now is a Medium, I just don't have the time to do an extensive threat analysis for this. Here is my calculation:
https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:N/AC:H/PR:L/UI:R/S:U/C:H/I:N/A:N&version=3.1

Here's my scenario to prove this is at least a Medium risk.

• We have two users, Alice and Bob.
• Alice needs to upload a very important file to a website that relies on formidable and that website doesn't randomize the file name.
• Alice also provides Bob with a copy of the file, or maybe Bob has also has access to it somehow.
• Bob doesn't like what he sees in the file and uploads a modified copy after Alice. Since Formidable doesn't randomize the file name, Alice's file is overwritten, and no one is alerted.

[keymandll]

@dpace-cs Interesting scenario. However, as far as I know and as far as the linked medium post goes, the default behavior is that formidable does not keep the extensions. (keepExtensions: false) In this default configuration, if you upload the same file twice, without changing its name, you cannot overwrite the file. These are the file names I'm getting:

DEMO % ls -l ./uploads
total 0
-rw-r--r--  1 keyman  staff  0  8 Jun 21:04 3f7e734605e781171150a5c00
-rw-r--r--  1 keyman  staff  0  8 Jun 21:04 3f7e734605e781171150a5c01

It's definitely not randomized, its seems to be incremental.

If we try with the non-default configuration (keepExtensions: true), we get the following file names on the file system after having uploaded the same file multiple times:

DEMO % ls -l ./uploads 
total 0
-rw-r--r--  1 keyman  staff  0  8 Jun 21:10 f350d2616fc05a8b97df47700.txt.malicious.png
-rw-r--r--  1 keyman  staff  0  8 Jun 21:10 f350d2616fc05a8b97df47701.txt.malicious.png
-rw-r--r--  1 keyman  staff  0  8 Jun 21:10 f350d2616fc05a8b97df47702.txt.malicious.png

As can be seen, the file name still has the incremnting part, threfore overwriting a previous file so far does not seem possible.

Considering how the default behavior works, the worst that can happen is someone could potentially brute-force the file names and maybe gain access to someone else's files. BUT, before anyone misunderstands, something important to note here is what @dpace-cs said: "... and that website doesn't randomize the file name". The developer of the website has the option to read the documentation and use the library according to his/her need/specific use case.

At the same time, if the default behaviour (keepExtensions: false) already produces a name that is not identical to the original file name, it could definitely generate a totally random value and then just hex string encode that. But, it does not seem to be a necessity at the moment.

What I ultimately wanted to say is, that while I like how @dpace-cs was thinking, I do not think there even a Medium risk there.

[tunnckoCore]

@dpace-cs the last point is not correct though. By default when there's not provided options.filename rename function, formidable randomizes the names with hexoid.

As about second point, that's our user's (developer in our case) fault. We are not end-user service or library or whatever. Things that are building on top of open source software and libs should be 1) cautious, and 2) read docs, especially when there are docs. There's not a single package of mine for the past 8 years that has not excellent and big docs. And all these years I'm noticing exactly that - nobody reads, even with ton of documentation, concrete examples and notes.

In this default configuration, if you upload the same file twice, without changing its name, you cannot overwrite the file.

Exactly. That's good defaults. And is maximum we should and can do.

It's definitely not randomized, its seems to be incremental.

True tho, hexo is interesting one. But IT IS guaranteed (not cryptographically tho) random and there's minimal possibility of conflicts.

(uh, why all this time i think of cuid lol, anyway)

---

edit: not to mention

formidable/src/Formidable.js

Line 19 in 81dd350

const toHexoId = hexoid(25);

we force 25 length, while Hexo's default is 16. Quite enough to me.

Important: Your risk of collisions decreases with longer strings!

[keymandll]

@tunnckoCore One thing to note is that hexoid produces incremental values and the first portion of the generated file name is fairly static. For example, I get the following when uploading two files, even with identical names.

DEMO % ls -l ./uploads
total 0
-rw-r--r--  1 keyman  staff  0  8 Jun 21:04 3f7e734605e781171150a5c00
-rw-r--r--  1 keyman  staff  0  8 Jun 21:04 3f7e734605e781171150a5c01

So we have 3f7e734605e781171150a5c part that is going to be static for a while until we reach a certain number of uploads, when the last character c will, I guess, turn into d. If a malicious actor uploads a file (just as a probe) he/she can get a significant portion of the file name. If the web app does not implement proper access control, the attacker may be able to access someone else's files. PLEASE NOTE: I'm still not blaming formidable. I'm blaming the developer of the fictional web app.

If I restart the service that I'm using now to do these tests of formidable, the file names now have a different start, such as:

-rw-r--r--  1 keyman  staff  0  8 Jun 21:26 4ab0f591f75905fd411b0c901
-rw-r--r--  1 keyman  staff  0  8 Jun 21:26 4ab0f591f75905fd411b0c902

So, an attacker targeting an insecurely implemented website could guess file names fairly easily between two service restarts, if the file name of his/her uploaded file is exposed.

Generating a uuid or cuid as mentioned by you instead of using hexoid would probably help with the above.

(I'm just praying no one going to misunderstand me and believe I'm saying there's any problem here... :) )

For now, my opinion that this "issue" worth maximum an INFO severity.

[tunnckoCore]

Oh yea, perfectly agree. I probably didn't noticed that with hexoid back than.

Will switch to cuid, because i'm big fan of it since day one.

[keymandll]

@dpace-cs Just realized there's something I've missed in your comment. W.r.t. the calculated CVSSv3 score, if I understand correctly, you calculated that for your example scenario, right? In that case, you have calculated that for a fictional webapp that behaves in a specific way. For example, the way we use formidable at our company you would not be able to calculate the same score. I'm sure you agree that any CVSS score you calculate will vary depending on specifics of the web application using the library and even in that case, it will be more about how the web app implemented file management, rather than formidable.

[dpace-cs]

It's definitely good that the filename is incremented and thus cannot be overwritten, and @tunnckoCore switching to cuid would eliminate an enumeration exploit (ref: https://owasp.org/www-community/attacks/Forced_browsing).

Outside of this issue, I have to disagree with @tunnckoCore and @keymandll assessment of who's responsible for fixing or mitigating a vulnerability. CVSS scoring will take the rating of the highest risk vector to a vulnerability at the system's default state. If the default state allows for file enumeration you can't say it's not a vulnerability because the programmer didn't modify the default state to secure it (ref: https://www.first.org/cvss/specification-document#3-1-Exploit-Code-Maturity-E). That would be like Linksys saying that the default admin credentials to a router being set to admin/admin aren't a vulnerability because the user didn't read the read the manual and change it after installation. I also understand that a good webapp should prevent file enumeration via proper access controls but again, that doesn't negate the default state of the package. What keymandll is referring to would be considered a Modified Base Metric per CVSS 3.1 specifications (ref: https://www.first.org/cvss/specification-document#4-2-Modified-Base-Metrics). That occurs when the score would be lowered due to mitigating controls such as access control to prevent file enumeration, but the base metric would still not change, you would just have a Modified Base Metric instead.

Unrelated to this issue: I wouldn't use INFO as risk level as there is no widely accepted risk standard for the that term. LOW should always be used in the place of INFO (ref: https://www.first.org/cvss/specification-document#Qualitative-Severity-Rating-Scale).

None the less, thank you both for replying and deeply analyzing instead of ignoring.

[keymandll]

@dpace-cs I completely understand what you are saying and you are not wrong, but you are not wrong in a different context.
My professional opinion is that mitigating a vulnerability should be done where the vulnerability manifests itself. I do believe formidable was not vulnerable as the CVE stated, therefore I do not think it makes much sense to discuss who should be fixing something that did not exist.

What I'm trying to say, using what you have said: "If the default state allows for file enumeration" - I'm fairly certain that formidable does not provide an interface that would allow file enumeration. A web application may provide an interface indeed, independently of formidable, in which case, file enumeration may be possible. But, who is responsible for implementing an access control mechanism, and logical (or physical) segregation of user data? Would that be formidable? I don't think so. I do not think it's a good approach to force your unique use case on a general-purpose file upload library. Instead, you should implement an abstraction layer that utilizes the library the way you want to use it.

I'm noticing two things from some of the discussions I had so far (not only here):

1. For some reason people tend to confuse formidable with a Content Management System (CMS). What you have said would absolutely make sense and I'd agree 100% if we were talking about e.g. a content management system.
2. People confuse "attack vector" with "vulnerability"

Btw, I'm not saying formidable should not contribute to a defence in-depth approach. But, it should not be accused of being vulnerable to something it isn't.

[tunnckoCore]

Well @dpace-cs , good point and example. I totally agree. I'm always for sane defaults and practicing it.
Plus, I'm not that familiar with all the security procedures.

Thank god we are having experts. 🙏🙌

[keymandll]

Alright, here is my proposal. As people so much want to force some vulnerability on formidable, I would be willing to make a compromise. Let's get rid of the original CVE as it's nonsense. We can get a new one with the following "vulnerability" description and with LOW severity (given the context):

"The way Formidable generated file names with the default configuration could have contributed to the successful exploitation of applications that were vulnerable to Forced Browsing."

What do you think @dpace-cs and @tunnckoCore ?

[tunnckoCore]

Well, agree. "The default way Formidable generate filenames..."

Plus, we will patch the defaults, so..

[keymandll]

I have updated my proposed "vuln" description based on @tunnckoCore 's comment.

[titanism]

Make sure you email [email protected] when you make the updates or to request changes to the original. cc @keymandll Thanks!!!

[keymandll]

Let's wait to see what @dpace-cs says about the proposed. Then, I will talk to MITRE to see if an update or a new entry would be more appropriate.

[dpace-cs]

I'm still having trouble seeing as how this would qualify as a Low. I'm calculating the CVSS score as 5.9 (Medium). I think it would be a big win if you could get NIST to drop the NVD score from 9.8 (Critical) to 5.9 (Medium).

Here is my reasoning:

1. Attack Vector: Should be Network based on the highest threat vector (i.e.. being integrated into a publicly exposed webapp).
2. Attack Complexity: Should be High since this would require a threat actor to be very familiar with formidable, and formidable on it's own doesn't provide access to the enumerated files.
3. Scope: Should be Unchanged based on https://www.first.org/cvss/specification-document#2-2-Scope-S
4. Integrity: Should be set to None since we determined files cannot be overwritten or modified.
5. Availability: Should be set to None since there is no proof or indication that formidable can be exploited to reduce, interrupt, or deny any component of the system.
6. Confidentiality: Should be set to High since the way formidable saves files could allow a threat actor to utilize a file-enumeration attack to gain full access to the file and not just the metadata. I understand that formidable is not providing that access but it is playing a role in enabling the attack.
7. User Interaction: Keep in mind that User Interaction means "This metric captures the requirement for a human user, other than the attacker, to participate in the successful compromise of the vulnerable component." Ultimately this should be set to None since we are only evaluating formidable in a default state, and there is no authentication or enforced second-user interaction, etc. I'm not saying that this is Formidable's fault or anything, I'm just saying there is no mitigation for User Interaction until a developer integrates Formidable into their app and provides access control or some other mechanisms that would mitigate this. Example: I could make a website like file.io to allow anonymously file uploads, no login required.
8. Privileges Required: This is defined as "This metric describes the level of privileges an attacker must possess before successfully exploiting the vulnerability." This gets a little subjective; but if we are evaluating Formidable in a default state again, there is no mitigating control until a developer integrates Formidable into their app and provides them. I'll refer again to my file.io example.

https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:N/A:N&version=3.1

FYI: I have no associated with MITRE or NIST and can't help you remove or reduce the existing CVE. You would need to communicate with them directly but referencing this threat would probably be very helpful to convincing them to reduce the score.

[keymandll]

@dpace-cs Just to confirm, are we both talking about this? If yes:

I almost agree with all your points, except with #6 (Confidentiality). I just do not see how you can calculate these for formidable, because all these depend on how an application using formidable is implemented. What I'm trying to say is that it's contextually inappropriate to have this calculation in the first place. Below is my reasoning.

Formidable does not dictate how an application should implement file management. It is possible to implement an application that even with formidable's default behaviour an attacker would not be able to access any files. For example, what about the simplest scenario where you can upload files but you can never download them? (That's exactly our scenario, we are also a user of formidable) So, your 5.9 (Medium) is a 2.2 (Low) at best according to CVSS in our specific use case.

So, given your calculation and mine, if I subtract 2.2 from 5.9, the resulting 3.7 is all up to the specifics and use cases supported by the application. Why would you blame that 3.7 on formidable?

[kolbma]

Ridiculous.
Do you know the other victims of Whitesource they have used for their published self-marketing study?

[keymandll]

@kolbma As far as I know Whitesource is not directly responsible of this mess. I suspect, in this case it is most likely that their "vulnerability" scanner, just like all the others picked up the same nonesense from NVD. So now we have all these software composition analysis tools spitting out issue reports based on the CVE. Probably it just happened that @johnchen05, probably a user/customer of Whitesource got a report from their tool...

However, your second sentence sounds like there is an interesting story out there I have not heard of. Most likely this is not the right forum to share, so If you write it up somewhere, let me know, happy to read it.

[tunnckoCore]

You're amazing guys! Seems like you know your job.

But I think it would be far more easier, faster and better to just upgrade on #857 merged pr and my comments from there.
I don't think we need to talk to them or disputing it, downgrading, dropping it, or opening a new one. It happened, let it go.

Or I'm just tired and not that familiar and interested in such things. What I can do is to try to patch it by the end of the week, and and add some more automation on the repo, plus CodeQL GH Action for the future. All that was part of the #635 and #830 anyway. I even think to skip v3, hahahaha. Joking.

Thanks, and don't think it or worry that much. :P

[keymandll]

@tunnckoCore Sure, I completely understand. Indeed, there's no need for a new CVE. If you replace hexoid with cuid all is going to be good. Also, it wasn't your fight anyways, it was mine, so I'm just going to take it somewhere else. :)

Anyway, it was great having this discussion with all of you. Feel free to reach out to me if I can be of any help in the future.

Oh, and before I forget thanks a lot to all contributors for this amazing library.