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

Bypass the https://github.com/418sec/node-pdf-image/pull/1 Fix#2

Closed
Mik317 wants to merge 1 commit into418sec:masterfrom
Mik317:master
Closed

Bypass the https://github.com/418sec/node-pdf-image/pull/1 Fix#2
Mik317 wants to merge 1 commit into418sec:masterfrom
Mik317:master

Conversation

@Mik317
Copy link

@Mik317 Mik317 commented Mar 19, 2020

Hi Huntr Dev team :),
as I had promised, I would have started working on some of yours modules.

I'd like to say that the #1 fix (which has been verified as fixing the RCE vuln disclosed previously), doesn't fix the issue.

The fix proposed by @mufeedvh doesn't check for the " (double quote) character, making an attacker able to escape anyway the filename passed as argument, leading to argument injection.
Also, take in mind that in the bash world, the !! (double exclamation point) can be used to refer to the last command executed in the same shell.

Chaining this 2 characters, an attacker may be able still to bypass the fix, leading anyway to RCE.
I'm attaching my pull request which blacklists also those 2 characters, in order to avoid this (unprobable, but still fully exploitable) scenario :).

Best, Mik

@Mik317 Mik317 changed the title Bypass https://github.com/418sec/node-pdf-image/pull/1 Fix Bypass the https://github.com/418sec/node-pdf-image/pull/1 Fix Mar 19, 2020
@mufeedvh
Copy link

Nice Catch! It does lead to an Argument Injection although my fix was for the Remote Code Execution vulnerability which is to be patched as per the bounty. As my fix has already been approved for mitigating the RCE, I am adding these two characters ! and " to my fix as a plus for this particular vulnerability. :)

How chaining it to RCE is not possible (or Bypassing The Fix)

The !! (double exclamation point) is used to refer the previous command on a shell prompt and it's not possible to execute it from a bash script or any code that executes this shell command.

For instance, I tried executing this command via Node (as this project is written in it):

executing-previous-command-via-node-attempt

As you can see from the above screenshot, executing the !! (double exclamation point) is not possible outside of a shell prompt.

Hence this does not bypass the fix for the RCE although it is a valid finding of an Argument Injection vulnerability.

I may be wrong, so can you please demonstrate a PoC to chain " and !! (Argument Injection) to escalate to RCE and thus bypassing the fix? :)

Thanks & Regards,
Mufeed

@ferretwithaberet
Copy link

Next time, instead of posting the whole link to the other pull request, do #1.

@Mik317
Copy link
Author

Mik317 commented Mar 19, 2020

Hi @mufeedvh :),
I've been testing early today with autocomplete active in node, leading me to the erroneous conviction that child_process would have taken the input and processed it in the same way.

Yep, I can confirm it's not possible completely bypass your fix (good work mate ;))

$ child_process.exec('!!');

...
{ Error: Command failed: `!!`
/bin/sh: 1: !!: not found

    at ChildProcess.exithandler (child_process.js:294:12)
    at ChildProcess.emit (events.js:198:13)
    at ChildProcess.EventEmitter.emit (domain.js:466:23)
    at maybeClose (internal/child_process.js:982:16)
    at Socket.stream.socket.on (internal/child_process.js:389:11)
    at Socket.emit (events.js:198:13)
    at Socket.EventEmitter.emit (domain.js:466:23)
    at Pipe._handle.close (net.js:607:12) killed: false, code: 127, signal: null, cmd: '`!!`' }

Anyway, as I outlined before, it's still possible argument injection (happy you've integrated the resolution in the official fix).

PS: Hi @RadoiAndrei :), excuse me, I'm not so practice with Git. The next time I'll use the #1 syntax :).

Regards, Mik

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

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

@Mik317 - we really appreciate your contributions and spotting the argument injection! As this wasn't a bounty, we can't award it to you, but to say thank you, we will be sending some credits your way! 🍰 💰 We look forward to reviewing more of your fixes/disclosures 😉in the future!

From the huntr team

@huntr-helper
Copy link

Sorry @Mik317, we enjoyed reviewing your fix but another was selected. We appreciate your effort and look forward to reviewing more of your fixes in the future! 🔨 😎

@Mik317
Copy link
Author

Mik317 commented Mar 20, 2020

Hi @JamieSlome :),
thank you so much for the kind words and the constant help :).
What do you mean with

we will be sending some credits your way!

? (I'm so curious lol).

Hey @mufeedvh :),
still congrats for the bounty and the great fix. I'm so happy it has been possible to collaborate for fixing also the minor argument injection issue :). Thank again and keep rocking 💪

Regards, Mik

@JamieSlome
Copy link

@Mik317 - we have credited your huntr account with some credits for contributing to the fix! These can be redeemed for prizes in the future; once we have launched our store 💸!

Once again, good job @mufeedvh & @Mik317 👍 🍰!

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.

5 participants