Skip to content

[Java] CWE-078: Add JSch lib OS Command Injection sink - #5710

Merged
smowton merged 3 commits into
github:mainfrom
p0wn4j:jsch-os-injection
May 10, 2021
Merged

[Java] CWE-078: Add JSch lib OS Command Injection sink#5710
smowton merged 3 commits into
github:mainfrom
p0wn4j:jsch-os-injection

Conversation

@p0wn4j

@p0wn4j p0wn4j commented Apr 17, 2021

Copy link
Copy Markdown
Contributor

JSch is a pure Java implementation of SSH2. JSch allows you to connect to a sshd server and use port forwarding, X11 forwarding, file transfer, command execution, etc.
If an attacker can control the command that is going to be executed on a sshd server, it can lead an attacker to Remote Code Execution on that server.

Code example:

JSch jsch = new JSch();
...
Session session = jsch.getSession(user, host, 22);
...
session.connect();
Channel channel = session.openChannel("exec");
((ChannelExec) channel).setCommand(controlledCommand);
channel.connect();
...

Found hits on these repos:
https://github.com/KingLai23/Puzzle
https://github.com/manuelacalvo/DATATourisme
https://github.com/jack775544/jTransfer

@p0wn4j
p0wn4j requested a review from a team as a code owner April 17, 2021 23:00
@p0wn4j
p0wn4j force-pushed the jsch-os-injection branch 5 times, most recently from de99cb7 to 746f43b Compare April 20, 2021 19:42

@owen-mc owen-mc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can simplify by making setCommand the sink. I can't think of any good reason why code would call setCommand apart from that they intend to call connect soon afterwards.

This shouldn't be a stand-alone query. It should be adding some sinks to ExecTainted, the existing command injection query. That query isn't very easy to extend at the moment, so I have made a PR to fix that.

Comment thread java/ql/src/experimental/Security/CWE/CWE-094/JSchOSInjection.qhelp Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/JSchOSInjectionSanitized.java Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/JSchOSInjectionSanitized.java Outdated
@owen-mc

owen-mc commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@p0wn4j My PR has been merged now, so you can extend the ExecTainted query easily.

@p0wn4j
p0wn4j force-pushed the jsch-os-injection branch from 746f43b to 6627773 Compare April 24, 2021 08:15
@p0wn4j
p0wn4j requested review from owen-mc and removed request for a team April 24, 2021 08:16
@p0wn4j
p0wn4j force-pushed the jsch-os-injection branch from 6627773 to 797e00f Compare April 24, 2021 08:19
@p0wn4j p0wn4j changed the title [Java] CWE-094: Query to detect OS Command Injection by JSch library [Java] CWE-078: Add JSch lib OS Command Injection sink Apr 24, 2021
Comment thread java/ql/test/experimental/query-tests/security/CWE-094/options Outdated
Comment thread java/ql/src/experimental/Security/CWE/CWE-094/JSchOSInjectionSanitized.java Outdated

@owen-mc owen-mc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That looks great. I've checked and the new version gets the same hits on LGTM as the old one. The other, more minor, comments from my previous review still stand.

@owen-mc
owen-mc dismissed their stale review April 26, 2021 09:49

Changes made

@owen-mc

owen-mc commented Apr 26, 2021

Copy link
Copy Markdown
Contributor

@p0wn4j Do you intend to make a bug bountry submission for this contribution?

@p0wn4j

p0wn4j commented Apr 26, 2021

Copy link
Copy Markdown
Contributor Author

Yes I want to make a bug bounty submission.

@p0wn4j
p0wn4j force-pushed the jsch-os-injection branch 2 times, most recently from 9a16be5 to 3d891f0 Compare April 26, 2021 10:20
@p0wn4j
p0wn4j requested a review from owen-mc April 26, 2021 10:21
@owen-mc

owen-mc commented Apr 26, 2021

Copy link
Copy Markdown
Contributor

@p0wn4j Great, that addresses all of the review comments. When you make a bug bountry submission this will be ready to pass to the next stage.

@p0wn4j

p0wn4j commented Apr 26, 2021

Copy link
Copy Markdown
Contributor Author

OK, thanks for the review :)

@p0wn4j

p0wn4j commented Apr 26, 2021

Copy link
Copy Markdown
Contributor Author

That looks great. I've checked and the new version gets the same hits on LGTM as the old one. The other, more minor, comments from my previous review still stand.

I have copied ExecTainted.ql, ExecCommon.qll from java/ql/src/Security/CWE/CWE-078/ path.

@smowton
smowton merged commit 0afe22d into github:main May 10, 2021
@p0wn4j
p0wn4j deleted the jsch-os-injection branch May 10, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants