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

Instantiate payload modules so parameter validation occurs #5838

Merged
merged 1 commit into from Aug 14, 2015

Conversation

bcook-r7
Copy link
Contributor

Calling .new on payload modules does not perform parameter validation, leading to a number cached sizes based on invalid parameters. Most notably, normalization does not occur either, which makes all OptBool params default to true. This caused the windows/adduser to be larger with its cached size parameter than it would be if you generate it with msfvenom or from framework directly.

Validation steps

  • The following command works without validation errors:
    ./msfconsole -qx 'use exploit/windows/smb/ms08_067_netapi; set payload windows/adduser'
  • tab completion with set payload using ms08_67_netapi should show 141 entries, not 119.
  • specs should pass consistently

@@ -9,7 +9,7 @@

module Metasploit3

CachedSize = 108
CachedSize = :dynamic
Copy link
Contributor

Choose a reason for hiding this comment

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

:?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Determining if a payload is :dynamic is a little precarious. It currently just generates the payload a few times and sees if the size changes, but this is not always reliable. We probably need a way to force a manual override. I probably got unlucky with the random seed.

Copy link
Contributor

Choose a reason for hiding this comment

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

oook, expected so :) just curious, thanks!

@jvazquez-r7
Copy link
Contributor

Looks like travis ask for ./spec/support/shared/examples/payload_cached_size_is_consistent update :)

@bcook-r7
Copy link
Contributor Author

Ah, you're right - I need to move the default datastore options into the library code for this instead of coding it into the tool directly. Thanks.

@bcook-r7
Copy link
Contributor Author

With the latest changes, some payloads marked as 'dynamic' go back to being statically sized. There were a few payloads that did something like cmd = datastore['CMD'] << "\x00", which has a side-effect of modifying the datastore option as well. This caused the payload to change size each time it was instantiated with the same datastore :P.

Calling .new on payload modules does not perform parameter validation, leading
to a number cached sizes based on invalid parameters. Most notably,
normalization does not occur either, which makes all OptBool params default to
true.
@jvazquez-r7
Copy link
Contributor

It is an awesome work :) thanks @bcook-r7, handling it in a while!

@jvazquez-r7
Copy link
Contributor

HEAD is now at 6b1e911... Instantiate payload modules so parameter validation occurs
aus-mac-0916:metasploit-framework jvazquez$ ./msfconsole -qx 'use exploit/windows/smb/ms08_067_netapi; set payload windows/adduser'
payload => windows/adduser
msf exploit(ms08_067_netapi) > set payload
Display all 141 possibilities? (y or n)

travis is green and also pass locally.

@@ -14,6 +14,27 @@ module Util

class PayloadCachedSize

@opts = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it has sense as instance variable. Indeed, I don't think we can't create PayloadCachedSize even :). I'm going to switch to a constant before landing if you don't mind. Looks definitely like a constant. Feel free to fix me if I'm wrong, just reviewing changes :)

@jvazquez-r7 jvazquez-r7 merged commit 6b1e911 into rapid7:master Aug 14, 2015
@jvazquez-r7
Copy link
Contributor

Landed after minor fixes here: f25a5da

Thanks @bcook-r7, this is an awesome patch!

@bcook-r7
Copy link
Contributor Author

Muchas gracias @jvazquez-r7

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.

None yet

2 participants