Skip to content
This repository was archived by the owner on Dec 19, 2023. It is now read-only.

Fixed Remote Code Execution Vulnerability#1

Merged
huntr-helper merged 4 commits into418sec:masterfrom
mufeedvh:master
Mar 20, 2020
Merged

Fixed Remote Code Execution Vulnerability#1
huntr-helper merged 4 commits into418sec:masterfrom
mufeedvh:master

Conversation

@mufeedvh
Copy link

⚙️ Fix:

The PDFImage() function doesn't validate the input of pdfFilePath input parameter. The pdfFilePath parameter accepts the path of the PDF file which is then used in a system command execution which when given without validation will result in a Remote Code Execution (RCE) Vulnerability.

What the fix does is: It validates the pdfFilePath input and checks for characters that can be used to concatenate any other system commands.

How:

The mitigation is implemented with a Regex (/;|&|`|\$|\(|\)|\|\||\||!|>|<|\?|\${/g) check for invalid characters in index.js

📎 Proof of Concept (PoC):

poc.js (put inside the node-pdf-image folder)

var PDFImage = require('.').PDFImage;

var pdfImage = new PDFImage('"; sleep 500 #"');
pdfImage.getInfo();

🔥 Fix On Action:

OSX:

$ brew install imagemagick ghostscript poppler

Linux (Ubuntu):

$ sudo apt-get install imagemagick ghostscript poppler-utils

Run PoC To Test Fix:

$ npm install
$ node poc.js

❤️ After Fix:

node-pdf-image-fixed-poc


✌️ Fixed!


@JamieSlome JamieSlome requested review from a team, JamieSlome and toufik-airane and removed request for a team March 13, 2020 09:43
Copy link

@toufik-airane toufik-airane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear @mufeedvh,

Thank you for your contribution.

The regexp isn't easily readable and so maintainable and especially two parts of the expression.
Can you ease the regular expression or at least document it?

Best regards.

index.js Outdated

function PDFImage(pdfFilePath, options) {
// validating the file path for invalid characters to prevent remote code execution
if (/;|&|`|\$|\(|\)|\|\||\||!|>|<|\?|\${/g.test(pdfFilePath)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regexp isn't easily readable and so maintainable and especially two parts of the expression.
The first part is |\|\||\| and the second is |\${.

@adam-nygate adam-nygate requested review from adam-nygate and removed request for JamieSlome March 13, 2020 12:06
Copy link
Member

@adam-nygate adam-nygate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it looks like you're duplicating matches for $ symbols and pipes (matching both \|\| and \|). Feel free to revise and then we can accept the PR 🙂

@mufeedvh
Copy link
Author

Hi,

Check out the revised commit, I just cleaned up the Regex to /[`!@#$%^&*()_+\-=\[\]{};':"\\|,<>\/?~]/ to filter out special characters except . to only accept filenames.

Copy link

@toufik-airane toufik-airane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear @mufeedvh,

You are restricting too many characters. Some characters seem not related to a command shell feature.

index.js Outdated

function PDFImage(pdfFilePath, options) {
// validating the file path for invalid characters to prevent remote code execution
var filter_chars = /[`!@#$%^&*()_+\-=\[\]{};':"\\|,<>\/?~]/;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all characters could trigger remote code execution.
You've blacklisted some harmful and probably legit one such as _ (underscore), ~ (tilde), etc.

I would only filter these characters, that have potential command shell feature:
;|`$()&<>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah didn't think about it. I did it in a hurry and was sleepy! 😅

@mufeedvh
Copy link
Author

Done, fixed that! 👍

Copy link
Member

@adam-nygate adam-nygate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@huntr-helper
Copy link

Congratulations @mufeedvh - your fix has been selected! 🎉

Thanks for being part of the community & helping secure the world's open source code.
If you have any questions, please respond in the comments section. Your bounty is on its way - keep hunting!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants