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

Handle invalid payloads more clearly #11823

Merged
merged 2 commits into from Jun 4, 2019

Conversation

Projects
None yet
4 participants
@busterb
Copy link
Member

commented May 8, 2019

Currently, if you have an error in a payload module (e.g. you have a syntax error in a module), the error is not noticed early enough in the generation process by the parameter validator, leading to a mysterious message like so from msfvenom, due to it failing to instantiate the payload module later in the process:

Error: undefined method `platform' for nil:NilClass

This change cleans up some of the error handling, checks to see if the payload
module can be instantiated, and gives a more useful error output for the
different cases. This also tweaks some of the literal string outputs to match
other exception strings from payload_generator.

Verification

  • Generate an invalid payload: ./msfvenom -p linux/x86/shell_bind_tcp_random_portal
  • Verify you get an error message:
$ ./msfvenom -p linux/x86/shell_bind_tcp_random_portal
Error: invalid payload: linux/x86/shell_bind_tcp_random_portal
  • Modify a payload to have a syntax error (delete an end from a block, then try generating it: ./msfvenom -p linux/x86/shell_bind_tcp_random_port
  • Verify you get a less-mysterious error message (you still need to look at the logs to see why it was unloadable):
$ ./msfvenom -p linux/x86/shell_bind_tcp_random_port
Error: unloadable payload: linux/x86/shell_bind_tcp_random_port

@busterb busterb requested a review from asoto-r7 May 8, 2019

@busterb busterb changed the title handle invalid payloads more clearly [WIP] handle invalid payloads more clearly May 8, 2019

@asoto-r7

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

I'm seeing a handful of spec errors, both via Travis here on Github, and locally:

rspec ./spec/lib/msf/core/payload_generator_spec.rb:182 # Msf::PayloadGenerator when creating a new generator when given a payload through stdin should not raise Exception
rspec ./spec/lib/msf/core/payload_generator_spec.rb:158 # Msf::PayloadGenerator when creating a new generator when given an invalid payload should raise ArgumentError with "Invalid Payload Selected"
rspec ./spec/lib/msf/core/payload_generator_spec.rb:134 # Msf::PayloadGenerator when creating a new generator when not given a payload should raise ArgumentError with "Invalid Payload Selected"
rspec ./spec/lib/msf/core/payload_generator_spec.rb:206 # Msf::PayloadGenerator when creating a new generator when given an invalid format should raise ArgumentError with "Invalid Format Selected"
rspec ./spec/lib/msf/core/payload_generator_spec.rb:518 # Msf::PayloadGenerator#generate_raw_payload when passing a payload through stdin returns the payload from stdin
rspec ./spec/lib/msf/core/payload_generator_spec.rb:494 # Msf::PayloadGenerator#generate_raw_payload when passing a payload through stdin when no platform has been selected raises an IncompatiblePlatform error
rspec ./spec/lib/msf/core/payload_generator_spec.rb:469 # Msf::PayloadGenerator#generate_raw_payload when passing a payload through stdin when no arch has been selected raises an IncompatibleArch error

I think most of the issue is that a few of the test cases will need updating, but I'm unclear as to why some seemingly unrelated test cases are failing now.

I'm happy to dig deeper later, but I'm going to call it a night for now.

@busterb

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

Thanks, I had notice the test failures earlier, will update them to match.

busterb added some commits May 8, 2019

handle invalid payloads more clearly
Currently, if you have an error in a payload module (e.g. you have a syntax
error in a module), the error is not noticed early enough in the generation process by the parameter validator, leading to a mysterious message like so:

```
Error: undefined method `platform' for nil:NilClass
```

This change cleans up some of the error handling, checks to see if the payload
module can be instantiated, and gives a more useful error output for the
different cases. This also tweaks some of the literal string outputs to match
other exception strings from payload_generator.

@busterb busterb force-pushed the busterb:less-mystery-on-bad-payloads branch from ac9b2c7 to 621d404 May 31, 2019

@busterb busterb changed the title [WIP] handle invalid payloads more clearly Handle invalid payloads more clearly May 31, 2019

@busterb

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

Updated tests (and it found 1 bug, so fixed those too!)

raise InvalidFormat, "invalid format: #{format}" unless format_is_valid?
raise ArgumentError, "invalid payload: #{payload}" unless payload_is_valid?

# A side-effecto of running framework.payloads.create is that

This comment has been minimized.

Copy link
@bwatters-r7

bwatters-r7 Jun 2, 2019

Contributor

effect

@bwatters-r7 bwatters-r7 self-assigned this Jun 3, 2019

@bwatters-r7 bwatters-r7 merged commit 621d404 into rapid7:master Jun 4, 2019

3 checks passed

Metasploit Automation - Sanity Test Execution Successfully completed all tests.
Details
Metasploit Automation - Test Execution Successfully completed all tests.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

bwatters-r7 added a commit that referenced this pull request Jun 4, 2019

Land #11823, Handle invalid payloads more clearly
Merge branch 'land-11823' into upstream-master
@bwatters-r7

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Release notes

Error messaging have been improved for fatal conditions that occur while generating payloads.

msjenkins-r7 added a commit that referenced this pull request Jun 4, 2019

Land #11823, Handle invalid payloads more clearly
Merge branch 'land-11823' into upstream-master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.