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

Fix arbitray code execution on wkhtmltoimage#1

Merged
1 commit merged into418sec:masterfrom
alromh87:master
Sep 17, 2020
Merged

Fix arbitray code execution on wkhtmltoimage#1
1 commit merged into418sec:masterfrom
alromh87:master

Conversation

@alromh87
Copy link

@alromh87 alromh87 commented Sep 9, 2020

📊 Metadata *

Bounty URL: https://www.huntr.dev/bounties/1-npm-wkhtmltoimage/

⚙️ Description *

wkhtmltoimage was vulnerable against arbitrary command injection cause some user supplied inputs were taken and composed into string to be executed without prior sanitization
After update Arbitary Code Execution is avoided

💻 Technical Description *

Commands that relay on piping functions are excuted usng spawn and piping, sanitization is implemented but not for option.output, needed sanitization was implemented removing escaping chars

🐛 Proof of Concept (PoC) *

  1. Install package
  2. Create the following PoC file:
// poc.js
var wkhtmltoimage = require('./');
wkhtmltoimage.generate("test", {output:"test; touch HACKED; #"}, function(){});
  1. Check there aren't files called HACKED
  2. Execute the following commands in another terminal:
node poc.js #  Run the PoC
  1. Recheck the files: now HACKED has been created

Captura de pantalla de 2020-09-09 22-26-56

🔥 Proof of Fix (PoF) *

After fix no file is created

Captura de pantalla de 2020-09-09 22-04-56

👍 User Acceptance Testing (UAT)

Commands can be executed normally, functionality unafected

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.

LGTM 👍

@ghost
Copy link

ghost commented Sep 17, 2020

Just to double check - would it not make more sense to whitelist allowed characters, since then the likelihood of a bypass is drastically decreased? I.e. alphanumeric only

@alromh87
Copy link
Author

In this case I think it would liit the user on the directory for output file

@ghost
Copy link

ghost commented Sep 17, 2020

I see what you mean - in that case can we tweak the regex to be alphanumeric and a slash (/)

@alromh87
Copy link
Author

Done!

#2

@ghost ghost merged commit ba72f5a into 418sec:master Sep 17, 2020
@huntr-helper
Copy link

Congratulations alromh87 - 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, or hit us up on Discord. Your bounty is on its way - keep hunting!

Come join us on Discord

This pull request was closed.
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.

3 participants