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

IMPORTANT! Vulnerability SNYK-JS-PDFMAKE-6347243 #2702

Closed
corzelito opened this issue Mar 1, 2024 · 28 comments
Closed

IMPORTANT! Vulnerability SNYK-JS-PDFMAKE-6347243 #2702

corzelito opened this issue Mar 1, 2024 · 28 comments

Comments

@corzelito
Copy link

Affected versions of this package are vulnerable to Arbitrary Code Injection via a crafted POST request to the /pdf path. An attacker can execute arbitrary code on the system by sending a specially crafted request.

https://security.snyk.io/vuln/SNYK-JS-PDFMAKE-6347243

@liborm85
Copy link
Collaborator

liborm85 commented Mar 1, 2024

This snyk report is incorrect. The simulation method is not very well described so that it can be understood.
Report refers to server endpoint /pdf in pdfmake npm package, but pdfmake has no such endpoint in release package.

@joaoviictorti
Copy link

Hello,

pdfmake includes vulnerable code that has been made available on GitHub, accessible to a wide range of users. This fact is not recent and highlights an important concern: the availability of code on an open source platform such as GitHub allows anyone to run the code without adequate security measures, regardless of whether or not it is made available via NPM.

This problem was officially acknowledged with the assignment of CVE-2022-46161, highlighting the seriousness of the security flaw. The attempted resolution by pdfmake, which consisted of replacing the "eval" function with the "Function" class, did not address the vulnerability effectively, keeping the code at risk.

It is crucial to address these security issues more thoroughly to ensure the protection of users and the integrity of the systems that depend on pdfmake.

The CVE has been recognized by Mitre as valid and there's nothing wrong with it.

Because if the CVE was incorrect, why was a solution attempted in CVE-2022-46161? I'm happy to help with any solution to this problem.

@liborm85
Copy link
Collaborator

liborm85 commented Mar 5, 2024

In old CVE-2022-46161 is explained that this vulnerability is not in any package which means that using the npm package is safe regardless of this vulnerability.
New reported vulnerability CVE-2024-25180 only announces vulnerability in npm package, but if I run npm install pdfmake there is no vulnerability here as report describes. Therefore report is incorrect.

CVE-2022-46161 was solved because PR 2519 was received and released new version 0.2.7 which also announces that there is actually no change to package.

PR for fix is welcome.

@zapl
Copy link

zapl commented Mar 6, 2024

@joaoviictorti what you've flagged is not pdfmake, it's a separate (developer only) application living in the same github repo.
The application is also meant to enable you to execute arbitrary code so one can test pdfmake. Think tiny JS Fiddle but it shows a pdf, like also available from http://pdfmake.org/playground.html - But if you want to test locally how "ls /root" looks like in a pdf, that's exactly what you'd use that application for.

The code in that application is not distributed in the package that is built from the sources in github. The package is therefore entirely safe and also doesn't have anything to do with POST requests to /pdf, thats only in this small utility/example/demo application.

@joaoviictorti
Copy link

Hello

As soon as you distribute a package in the same repository as the source code in pdfmake, it's included within it. If it's only for developers, it doesn't make sense to make it available in the public repository. This is a security flaw, yes, and it's not for nothing that there's already been an old CVE about it.

@joaoviictorti
Copy link

If you didn't consider this a security breach, you wouldn't have made this commit: c35b7ad

@zapl
Copy link

zapl commented Mar 6, 2024

@joaoviictorti I'm btw not related to the project in any way. I'm just a user of it that got notified by npm about a vulnerability.

I don't know why someone even did that, probably to make the CVE go away, a package version marked vulnerable even for bogus reasons is burned and maintainers need to do something about it.

As soon as you distribute a package in the same repository as the source code in pdfmake

What's distributed isn't the same as the source repository. Like in compiled languages there is a build step that produces the distributed artifacts. You can check e.g.

It's common practice to have additional utilities, scripts, examples, ... within open source repositories where else would developers working on something put their tools. That's why nowadays there are build systems to produce output from raw sources.

I simply don't see why

"The advisory has been revoked - it doesn't affect any version of package pdfmake"

is any different now. The packaged version is not vulnerable.

If you somehow use a raw clone the repository and deliberately use that developer tool or manage to have a vulnerability in your code that allows users to somehow start that developer tool, then you'd have a server that on purpose allows you to execute arbitrary code. But if you do that, you might as well flag all libraries that contain bad example code. That's not what CVEs are for.

@z3xddd
Copy link

z3xddd commented Mar 6, 2024

Hello!!

I found this vulnerability in my application when i used the last version of PDFMAKE.

Please fix this @zapl and @liborm85 because this bug still available.

Thanks so much for this found @joaoviictorti!

@joaoviictorti
Copy link

@zapl I think you still don't understand the real problem.

To have a vulnerability it doesn't necessarily have to be exposed via NPM, from the moment you make the code available in the repository on Github, you guarantee that every part of it is secure, so if it's not necessary to have this content, simply delete it.

@joaoviictorti
Copy link

Thanks @z3xddd!

@MrSimmmons
Copy link

MrSimmmons commented Mar 6, 2024

But the CVE is on the repository, not the NPM package. Therefore the CVE is incorrect as it's reporting on the wrong thing.

This just makes it a hassle for anyone who is using pdfmake in production as it's unnecessarily flagging something that isn't valid.

They are 2 separate entities by that description (which makes sense since the NPM package doesn't even include the vulnerable code)

@joaoviictorti
Copy link

But the product and the owner are still the same, there's no such thing as separate entities.

@z3xddd
Copy link

z3xddd commented Mar 6, 2024

@MrSimmmons I used this repo for customize in my application, because this still vulnerable.

@0xArthurSouza
Copy link

Hello,

Based on the aforementioned considerations, as a researcher, I agree with the comments made by @joaoviictorti and @z3xddd that the reported bug still represents a vulnerability that requires correction. @joaoviictorti highlighted that MITRE recognized the vulnerability as CVE, which suggests its legitimacy as a problem. I understand the views presented by @zapl and @MrSimmmons , however, it is important to point out that this is a valid vulnerability that needs to be patched to avoid exposure to CVE-2024-25180.

@zapl
Copy link

zapl commented Mar 6, 2024

@ArthurSouza37 But is it a vulnerability in the first place? That application is made to allow arbitrary code execution, ideally including the use of require. That's a very useful ability in the context it's meant to be used, local-only testing where you already have the same ability to run that code.

Is there any way to handle such a thing without restricting the functionality of the tool? Clarify intent of the application? Restrict it to listening on localhost only? There should be a way to have even dangerous dev tools available for developers.

@taufik230301
Copy link

what is the expected data in this /pdf ?, i'm sure if there are the way to validate the data, so th arbitary code can be validated.

@zapl
Copy link

zapl commented Mar 7, 2024

@taufik230301 https://github.com/bpampuch/pdfmake/tree/master/dev-playground a small Web REPL like application, the /pdf endpoint is meant to take anything you input into your browser. No need for fancy POCs, you have a nice frontend with syntax highlighting to write terrible code such as execSync('cat /etc/passwd'); in. It has no inherently "valid" input, you're testing all this as a developer locally on your own machine, valid is what you consider ok for your machine to run. That said, you should use it to test pdfmake's server side pdf rendering against the current source code state, since you run this in context of the checked out sources, e.g. primarily to work on pdfmake itself. To check if your changes look ok in a live web preview, you are building a pdfmake config object which is then rendered by the server part.
The issue is that treating this tool as a proper server, which https://securitylab.github.com/advisories/GHSL-2022-068_pdfmake/ simply did (maybe they thought it powers the online playground? That's the same tool but entirely on browser side and meant for endusers), means you need to make sure it fulfills all the standards a public webserver version of this tool would need. But it's an inherently bad idea to allow users to make a remote machine execute anything like that, as such it would require sophisticated sandboxed processes, resource (CPU, memory) limits, execution time limits etc. Any way of stalling or crashing the server part of that app is a DOS, allowing regex in the syntax is essentially reDOS, even loops and arrays are enough to trigger issues regarding excessive resource consumption. If it was a proper servers, that would all be very necessary, for a tiny 50 lines of code tool to develop locally that's not even feasible. You can potentially reduce the input to just JSON of the config object, but even that might cause issues if you still manage to overload your machine with e.g. a large enough pdf that crashes it.
Either this bit of code is deemed invalid for CVEs or it probably should be moved into a different repository where developers can still use it as is. The main goal not being to make the code safer but to make pdfmake immune from future claims that it enables any kind of web based vulnerability.

ps: not affiliated with this project in any way.

@liborm85
Copy link
Collaborator

liborm85 commented Mar 7, 2024

As soon as you distribute a package in the same repository as the source code in pdfmake, it's included within it. If it's only for developers, it doesn't make sense to make it available in the public repository. This is a security flaw, yes, and it's not for nothing that there's already been an old CVE about it.

@joaoviictorti pdfmake is open source and it is developed by the whole community. All source code and utilities are publicly available to any new contributor/developer. That's the essence of open source.

If you didn't consider this a security breach, you wouldn't have made this commit: c35b7ad

It is safer to use new Function than eval and that resolved old CVE.

To have a vulnerability it doesn't necessarily have to be exposed via NPM, from the moment you make the code available in the repository on Github, you guarantee that every part of it is secure, so if it's not necessary to have this content, simply delete it.

@joaoviictorti This is essentially true. But CVE-2024-25180 totally wrong because it says there is vulnerability in npm package, but that is not true, npm package is safe, that's the point.

I found this vulnerability in my application when i used the last version of PDFMAKE.

@z3xddd pdfmake package is not affected by this vulnerability. Using npm package is safe. Theoretical vulnerability is only in tool for pdfmake developers and it is not part of any package.

But the product and the owner are still the same, there's no such thing as separate entities.

@joaoviictorti It is not true. Most projects use a lot of tools, examples, etc. which are in git but not part of release. pdfmake is the same case.

@joaoviictorti
Copy link

Hello,

@liborm85 I have been working with security research for many years and know many security professionals and this is indeed vulnerable. However, I realize that there may be hesitation to accept that the project has vulnerabilities to attacks and that you are willing to take the risk.

And Function and eval will never be safe if it is used incorrectly, if you support this kind of development, you can take the risk as you see fit.

@joaoviictorti
Copy link

A simple search will find how to execute dynamic code in JavaScript safely, without using eval() or new Function(), you can consider using Web Workers or sandboxing libraries. But do as you see fit.

@joaoviictorti
Copy link

If you believe that accepting the risk and leaving the code that way is better for you, there's nothing I can do. As far as I'm concerned, this is the end of the conversation. Thank you very much.

@bmalinconico
Copy link

For anyone following along due to a scanner triggering on this package. This is not a vulnerability in the distributed package. This is a vulnerability in the dev server provided for experimentation.

You can be pedantic and say this is still a vulnerability, but your running application is not at risk unless you are using the example dev server. If you are using the example dev server you made a mistake a long time ago and need to get that out of your env right away.

I've validated the version I am using 0.2.7 is not distributed with the vulnerable code.

@corzelito
Copy link
Author

The vulnerability is still in Synk, and if it apparently depends on the playground, why isn't it changed or removed from this project? This causes the library to be marked as vulnerable, and will no longer be used as much...

@Pauan
Copy link

Pauan commented Mar 12, 2024

The Snyk report is wrong, it is a false positive, it is up to Snyk to fix the issue.

@MrSimmmons
Copy link

MrSimmmons commented Mar 20, 2024

@liborm85 I filed a ticket with snyk support team for them to look into it, and an amendment has been added deeming it not a vulnerability. It also doesnt appear on their DB search results.
So this issue can probably be closed.

@z3xddd
Copy link

z3xddd commented Mar 20, 2024

The vulnerability persists in this repository. My company has already stopped using this lib precisely because of your TERRIBLE treatment of security. Instead of adjusting the code in this repository, which YES is VULNERABLE to RCE, they want to keep making excuses that the package is not vulnerable.

Then learn to take information security seriously. :)

@Pauan
Copy link

Pauan commented Mar 20, 2024

@z3xddd That is incorrect, there is NO VULNERABLE CODE in the npm package. None at all.

That is why Snyk labeled it as a false positive and revoked their report:

https://security.snyk.io/vuln/SNYK-JS-PDFMAKE-6347243

Snyk amendment

Whatever security procedures you are using should be capable of handling false positives like this. False positives can and do happen.

@liborm85
Copy link
Collaborator

Thank you @MrSimmmons.

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

10 participants