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

Fix command injection vulnerability #2

Merged
merged 2 commits into from
May 15, 2020
Merged

Fix command injection vulnerability #2

merged 2 commits into from
May 15, 2020

Conversation

69
Copy link

@69 69 commented May 8, 2020

⚙️ Description

I've remedied the command injection vulnerability by using a more secure method to run the scp command.

💻 Technical Description

I've replaced the usage of exec in this module with execFile, which automatically escapes the command variables being passed to scp.

🐛 Proof of Concept (PoC)

When running the following snippet in the node-scp directory, a file named HACKED should show up in your current working directory, proving the command injection vulnerability:

const client = require("./scp");
client.get({ port: "; touch HACKED ;" })

🔥 Proof of Fix (PoF)

When the above snippet is ran on my fork of the module, the execution fails with the error "Bad port '; touch HACKED ;'", which indicates the port variable has been escaped.

image

👍 User Acceptance Testing (UAT)

I've fixed the get test to request the correct file, as it requested a different file when being ran. Running the included tests after setting up a SSH location named core proves that the module is working perfectly fine:

$ node test/send.js
[Arguments] { '0': null, '1': '', '2': '' }

$ node test/get.js
[Arguments] { '0': null, '1': '', '2': '' }

The file what showed up on my remote system and after deleting it locally and running test/get, it was properly re-downloaded.

@adam-nygate adam-nygate self-requested a review May 15, 2020 14:39
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
Member

Congratulations @69 - 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!

@huntr-helper huntr-helper merged commit 32858eb into 418sec:master May 15, 2020
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