Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unrestricted Upload of File with Dangerous Type #329

Open
shahbaz-pucit opened this issue Sep 7, 2022 · 8 comments
Open

Unrestricted Upload of File with Dangerous Type #329

shahbaz-pucit opened this issue Sep 7, 2022 · 8 comments

Comments

@shahbaz-pucit
Copy link

This package has a CVSS Score: of 9.8, which is very high.
An arbitrary file upload vulnerability in the file upload module of Express-Fileupload v1.3.1 allows attackers to execute arbitrary code via a crafted PHP file.

@alperb
Copy link

alperb commented Sep 21, 2022

Well, you are technically right. It's a file upload module and it can lead to many problems as OWASP described here. As stated in OWASP Wiki:

There are really two classes of problems here. The first is with the file metadata, like the path and file name. These are generally provided by the transport, such as HTTP multi-part encoding. This data may trick the application into overwriting a critical file or storing the file in a bad location. You must validate the metadata extremely carefully before using it.
The other class of problem is with the file size or content. The range of problems here depends entirely on what the file is used for. See the examples below for some ideas about how files might be misused. To protect against this type of attack, you should analyse everything your application does with files and think carefully about what processing and interpreters are involved.

The second one is resolved with the busboy's size limiter. Actually, the first one is the one that can mess things up.

For the first problem, the package makes necessary move processes with inputs taken from the application. So if the file would end up in a place where it shouldn't be it seems like it's the developer's fault.

As far as I've seen there are many checks on file names. File name checks can be made with safeFileNames option. In the README:

Strips characters from the upload's filename. You can use custom regex to determine what to strip. If set to true, non-alphanumeric characters except dashes and underscores will be stripped. This option is off by default.

The safeFileNames: true option strips all "weird characters" from the filename.

However, I was not able to see any checks on file metadata. To be honest, I am not sure how this package can check for "malicious" stuff in the file's metadata since the metadata is not even being processed in this package. So it's more likely to be handled by whatever processes this uploaded file or however this file is processed.

Please correct me if I'm mistaken, I did my best to answer :)

@chqueen
Copy link

chqueen commented Jan 17, 2023

Thanks for the explanation @alperb. Would it be possible to mark the CVE's disputed? Seems like @richardgirges didn't agree on the severity at least. If the CVEs stay as written, I have to migrate to some other busboy wrapper.

@alperb
Copy link

alperb commented Jan 18, 2023

@chqueen, argued CVE is CVE-2022-27140.

I don't think the argued vulnerability can be considered valid. The resource provided as evidence of this vulnerability is a video that demonstrates the upload process of a PHP file. Uploading a PHP file may be the intended behavior of an application.

Seems like this vulnerability was first reported by @harunoz according to #316. Maybe he can elaborate more on this issue.

@chqueen
Copy link

chqueen commented Jan 18, 2023

Yes, I agree with you. The "exposure" in this package is probably the passing through of potentially dangerous metadata. Unless @harunoz clarifies his findings or @richardgirges disputes the CVE, this package will likely decline in usage.

@richardgirges
Copy link
Owner

This vulnerability is invalid, and the proof submitted in the YT video demonstrates a bad-faith usage of the package in which the package-user intentionally opens themselves up to attack. I am not familiar with disputing vulnerabilities - can anyone point me in the right direction?

@chqueen
Copy link

chqueen commented Jan 18, 2023

I have never done it personally, but I found this document: https://www.cve.org/Resources/General/Policies/CVE-Record-Dispute-Policy.pdf

@luan-dang-techlabs
Copy link

Were we able to dispute the vulnerabilities?

@boxexchanger
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants