Security Fix for RCE on "easy-pdf-merge" - huntr.dev

[huntr-helper]

https://huntr.dev/users/Asjidkalam has fixed the RCE on "easy-pdf-merge" vulnerability 🔨. Asjidkalam has been awarded $25 for fixing the vulnerability through the huntr bug bounty program 💵. Think you could fix a vulnerability like this?

Get involved at https://huntr.dev/

Q | A
Version Affected | ALL
Bug Fix | YES
Original Pull Request | 418sec#1
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/npm/easy-pdf-merge/1/README.md

User Comments:

📊 Metadata *

Remote code execution vulnerability

Bounty URL: https://www.huntr.dev/bounties/1-npm-easy-pdf-merge

⚙️ Description *

The easy-pdf-merge module is vulnerable against RCE since user supplied inputs are formatted inside a command which is executed without prior checks. The argument options can be controlled by users without any sanitization. It was using exec() function which is vulnerable to Command Injection if it accepts user input and it goes through any sanitization or escaping.

💻 Technical Description *

The use of the child_process function exec() is highly discouraged if you accept user input and don't sanitize/escape them. I replaced it with execFile() which mitigates any possible Command Injections as it accepts input as arrays.

🐛 Proof of Concept (PoC) *

Install the package and run the below code:

const merge = require('easy-pdf-merge');

merge(["test", "test1"], "test2", {maxHeap:"test; touch HACKED; #"}, function(err){
  if(err) {
    return console.log(err)
  }
  console.log('Success')
});

A file named HACKED will be created in the current working directory.

🔥 Proof of Fix (PoF) *

After applying the fix, run the PoC again and no files will be created. Hence command injection is mitigated.

👍 User Acceptance Testing (UAT)

Only execFile is used, no breaking changes introduced.

[karuppiah7890]

@Asjidkalam Thanks for this PR. I'm still reading and trying to understand stuff here 😅

The first question I had in my mind was - why is the vulnerability out in the open? I thought Security issues and Security vulnerabilities are first privately informed to maintainers through email or similar platform and the project's security issue is fixed and then the vulnerability and the fix is told to the world.

The current way how this is done looks more like mocking all the npm and other package maintainers - putting all the vulnerabilities out in the open. The good part is you raised a PR yes. I need to review and merge it. I'm assuming the same is happening with any repo that is present on https://github.com/418sec/huntr . And sometimes review might take time. Since this open now, I'm just overwhelmed and rushing!

I see this more as a bad practice. Correct me if I'm wrong.

cc @JamieSlome as you seem to be part of https://github.com/418sec

[karuppiah7890]

I understand that maxHeap is allowing Remote Code Execution. I'm still thinking if the users of the library will allow their end users to put a value directly into it. It is possible though, yes - in case the devs decide that all inputs can be obtained from their end user, who is probably using a web browser and the library is running on the server side. Then it's a problem, yes.

I'll consider this as a rare case in my perspective and take the PR so that the users don't have to think too much about such security issues or have to worry about sanitization of user input before passing it to the library.

Note: I'm really not maintaining this small module much. I hope no one is using it in production for some critical thing! This is a very simple small module. Someone could just copy paste the code and use their own function to do it with some customization too probably

[JamieSlome]

@karuppiah7890 - thanks for your feedback and for getting in touch with us. I have responded to you on Twitter and also partially addressed your point in your GitHub Issue.

Cheers! 🍰

[karuppiah7890]

I should have setup CI/CD for this. Anyways, there's just a very simple single test file for this, which doesn't run as part of this PR. The happy path for the module is not satisfied by this PR

$ npm test

> [email protected] test /Users/karuppiahn/oss/github.com/karuppiah7890/easy-pdf-merge
> node test.js

Error: Command failed: java -jar  "/Users/karuppiahn/oss/github.com/karuppiah7890/easy-pdf-merge/jar/pdfbox.jar" PDFMerger "test/github cheat sheet.pdf" "test/text_extraction.pdf" "test/Out.pdf"
Error: Unable to access jarfile

    at ChildProcess.exithandler (child_process.js:308:12)
    at ChildProcess.emit (events.js:314:20)
    at maybeClose (internal/child_process.js:1021:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:286:5) {
  killed: false,
  code: 1,
  signal: null,
  cmd: 'java -jar  "/Users/karuppiahn/oss/github.com/karuppiah7890/easy-pdf-merge/jar/pdfbox.jar" PDFMerger "test/github cheat sheet.pdf" "test/text_extraction.pdf" "test/Out.pdf"'
}

This is because of one existing extra space in the command, which wasn't an issue before, but with this PR, this became an issue

[karuppiah7890]

I have pushed some changes in master for changing the behavior and removing unnecessary spaces.

The happy path still fails

$ npm test

> [email protected] test /Users/karuppiahn/oss/github.com/karuppiah7890/easy-pdf-merge
> node test.js

Error: Command failed: java -jar /Users/karuppiahn/oss/github.com/karuppiah7890/easy-pdf-merge/jar/pdfbox.jar PDFMerger "test/github cheat sheet.pdf" "test/text_extraction.pdf" "test/Out.pdf"
Exception in thread "main" java.io.FileNotFoundException: "test/github (No such file or directory)
        at java.base/java.io.RandomAccessFile.open0(Native Method)
        at java.base/java.io.RandomAccessFile.open(RandomAccessFile.java:347)
        at java.base/java.io.RandomAccessFile.<init>(RandomAccessFile.java:261)
        at java.base/java.io.RandomAccessFile.<init>(RandomAccessFile.java:216)
        at org.apache.pdfbox.io.RandomAccessBufferedFileInputStream.<init>(RandomAccessBufferedFileInputStream.java:99)
        at org.apache.pdfbox.pdmodel.PDDocument.load(PDDocument.java:1079)
        at org.apache.pdfbox.pdmodel.PDDocument.load(PDDocument.java:1006)
        at org.apache.pdfbox.multipdf.PDFMergerUtility.legacyMergeDocuments(PDFMergerUtility.java:451)
        at org.apache.pdfbox.multipdf.PDFMergerUtility.mergeDocuments(PDFMergerUtility.java:346)
        at org.apache.pdfbox.tools.PDFMerger.merge(PDFMerger.java:70)
        at org.apache.pdfbox.tools.PDFMerger.main(PDFMerger.java:49)
        at org.apache.pdfbox.tools.PDFBox.main(PDFBox.java:81)

    at ChildProcess.exithandler (child_process.js:308:12)
    at ChildProcess.emit (events.js:314:20)
    at maybeClose (internal/child_process.js:1021:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:286:5) {
  killed: false,
  code: 1,
  signal: null,
  cmd: 'java -jar -Xmx10m /Users/karuppiahn/oss/github.com/karuppiah7890/easy-pdf-merge/jar/pdfbox.jar PDFMerger "test/github cheat sheet.pdf" "test/text_extraction.pdf" "test/Out.pdf"'
}

[karuppiah7890]

I think the behavior of execFile is quite different? Not sure, I gotta read and understand better

[karuppiah7890]

@Asjidkalam Do you want to look into this?

[karuppiah7890]

Also, please rebase with master while doing so