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 ProcessMaker Plugin Upload module #8539
Conversation
register_file_for_cleanup "#{upload_dir}#{@plugin_name}.php" | ||
register_file_for_cleanup "#{upload_dir}#{@plugin_name}/class.#{@plugin_name}.php" | ||
# uncomment when cleanup of folders is supported | ||
# register_folder_for_cleanup "#{upload_dir}#{@plugin_name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wvu-r7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping #8018.
public function setup() {} | ||
public function install() { #{payload.encoded} } | ||
public function enable() {} | ||
public function disable() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the best place to put the payload was in the install
method.
Putting the payload in the enable
method requires an extra HTTP request to trigger the payload; and also increases the chances something could go wrong if the code path to enable a module is changed in the future.
Putting the payload in the disable
method is nice, because this method gets called automatically when we remove the plugin during cleanup. Unfortunately this is also problematic, because every attempt to remove the plugin calls the disable
method, triggering the payload. This prevents the cleanup method from successfully removing the plugin from the web interface.
else | ||
print_warning "#{peer} Unexpected reply" | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this code block isn't ideal, triggering the payload during plugin installation was the best approach as per comments below.
Testing this |
This works for me.
|
'cookie' => @cookie, | ||
'vars_post' => vars_post | ||
|
||
if !res |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think unless is preferred.
# ProcessMaker 1.x requires "-" after the plugin name in the file name | ||
fname = "#{plugin_name}-.tar" | ||
|
||
boundary = "----WebKitFormBoundary#{rand_text_alphanumeric rand(10) + 5}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use Rex::MIME::Message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had issues with 0x0D
in the past.
I don't remember if that was an issue here - I might have just assumed Rex::MIME::Message
would be problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting that there's no (feasible) way to control the binary data to prevent use of 0x0D
as the post data is a generated TAR archive. It is not subject to BadChars
restrictions.
@bcoles: for a trophy wife, you sure as hell pr like a champ. |
@sempervictus I don't understand. This code isn't likely to be re-used - what part of this should belong in |
@bcoles: I hit the wrong PR I'd on that, sorry. Comment stands alone though - you implement a lot of useful functionality |
This module is made more useful by these bugs (currently un-patched): |
This PR is blocked on #8018 to add a |
sounds like |
Bump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh fun. Why a lib require in a module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sempervictus feedback from @Chiggins indicated that the following error occurs without the require
I'm not sure why, as I tested with and without the require
and the module works in both instances for me, on latest msf5 dev branch.
I opted to add the require
, as the modules/auxiliary/admin/http/telpho10_credential_dump.rb
module also makes use of Gem::Package::TarReader
, like this module does, and that module also includes the require
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not a big fan of requiring a library inside a module, but if that's the specialized case with this then ¯_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its ok with everyone, let's move the require somewhere into a higher level mixin if we can or just use a more common archive parser (Rex has some, right?). I can't tell you how many times I messed something up having a req in my plugins and expecting it to have already been spun up when the module leader got to something. Pretty sure I've borked PRs when upstreaming that way too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with not including library requirements in modules. However, chiggins volunteered to manage this PR, and given the exploit failed for him that didn't leave me much choice.
No, Rex does not have a Tar archive parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we should probably make a generic Rex archive module, a d bloody add fastlib to it :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, Rex should handle creation of TAR archives. Unfortunately I don't have time to implement this. It's also outside the scope of this PR.
Release NotesThe exploits/multi/http/processmaker_plugin_upload module has been added to the framework. It generates and uploads a plugin to ProcessMaker to execute PHP code as the web server user. Credentials for a valid Administrator account are required to exploit this vulnerability. |
This PR adds an exploit for ProcessMaker.
Verification
msfconsole
use exploit/multi/http/processmaker_plugin_upload
set rhost <RHOST>
set username <USERNAME>
set password <PASSWORD>
set workspace <WORKSPACE>
run
Example Output
Installation
Source and Installers: