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

Update msfvenom format CLI parameter to be case insensitive #10802

Merged
merged 3 commits into from Nov 21, 2018

Conversation

Projects
None yet
4 participants
@kkirsche
Contributor

kkirsche commented Oct 12, 2018

Taking after:
https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/payload_generator.rb#L296

This pull request makes minor adjustments to conditional statements related to the format parameter allowing the condition to act in a case insensitive manner rather than requiring lowercase options.

Verification

With the changes installed, use the following command:

msfvenom -p windows/exec CMD=calc.exe -b '\x00\x0A\x0D'-a x86 --platform windows -f C

Before the change:

root@kali:~/exploits/freefloat-ftp# msfvenom -p windows/exec CMD=calc.exe -b '\x00\x0A\x0D'-a x86 --platform windows -f C
[-] No arch selected, selecting arch: x86 from the payload
Found 10 compatible encoders
Attempting to encode payload with 1 iterations of x86/shikata_ga_nai
x86/shikata_ga_nai succeeded with size 220 (iteration=0)
x86/shikata_ga_nai chosen with final size 220
Payload size: 220 bytes
Error: Unsupported buffer format: C

After the change:

root@kali:~/exploits/freefloat-ftp# msfvenom -p windows/exec CMD=calc.exe -b '\x00\x0A\x0D'-a x86 --platform windows -f C
[-] No arch selected, selecting arch: x86 from the payload
Found 10 compatible encoders
Attempting to encode payload with 1 iterations of x86/shikata_ga_nai
x86/shikata_ga_nai succeeded with size 220 (iteration=0)
x86/shikata_ga_nai chosen with final size 220
Payload size: 220 bytes
Final size of C file: 949 bytes
unsigned char buf[] = 
"\xdb\xc1\xd9\x74\x24\xf4\x58\x2b\xc9\xb1\x31\xbb\x65\xc6\xc2"
"\x9f\x31\x58\x18\x03\x58\x18\x83\xe8\x99\x24\x37\x63\x89\x2b"
"\xb8\x9c\x49\x4c\x30\x79\x78\x4c\x26\x09\x2a\x7c\x2c\x5f\xc6"
"\xf7\x60\x74\x5d\x75\xad\x7b\xd6\x30\x8b\xb2\xe7\x69\xef\xd5"
"\x6b\x70\x3c\x36\x52\xbb\x31\x37\x93\xa6\xb8\x65\x4c\xac\x6f"
"\x9a\xf9\xf8\xb3\x11\xb1\xed\xb3\xc6\x01\x0f\x95\x58\x1a\x56"
"\x35\x5a\xcf\xe2\x7c\x44\x0c\xce\x37\xff\xe6\xa4\xc9\x29\x37"
"\x44\x65\x14\xf8\xb7\x77\x50\x3e\x28\x02\xa8\x3d\xd5\x15\x6f"
"\x3c\x01\x93\x74\xe6\xc2\x03\x51\x17\x06\xd5\x12\x1b\xe3\x91"
"\x7d\x3f\xf2\x76\xf6\x3b\x7f\x79\xd9\xca\x3b\x5e\xfd\x97\x98"
"\xff\xa4\x7d\x4e\xff\xb7\xde\x2f\xa5\xbc\xf2\x24\xd4\x9e\x98"
"\xbb\x6a\xa5\xee\xbc\x74\xa6\x5e\xd5\x45\x2d\x31\xa2\x59\xe4"
"\x76\x5c\x10\xa5\xde\xf5\xfd\x3f\x63\x98\xfd\x95\xa7\xa5\x7d"
"\x1c\x57\x52\x9d\x55\x52\x1e\x19\x85\x2e\x0f\xcc\xa9\x9d\x30"
"\xc5\xc9\x40\xa3\x85\x23\xe7\x43\x2f\x3c";

@kkirsche kkirsche changed the title from Update msfvenom formatter to be case insensitive to Update msfvenom format CLI parameter to be case insensitive Oct 12, 2018

@rwhitcroft

This comment has been minimized.

Contributor

rwhitcroft commented Oct 16, 2018

Just to be pedantic - this seems like a slippery slope and counter to the spirit of unix being case sensitive.

@kkirsche

This comment has been minimized.

Contributor

kkirsche commented Oct 16, 2018

I can understand your point, though I disagree in this case. Many options that msfvenom and msfconsole provide should be case sensitive, though in this case I don't see the rational for it.

Unlike parameter names which benefit from, as using -C and -c can be valuable, parameters such as -o which are going to decide output filenames, and should match the OS's decisions, is there an example where the case of the format string, used internal to the tool for deciding output structure, matters?

@rwhitcroft

This comment has been minimized.

Contributor

rwhitcroft commented Oct 16, 2018

I guess it's also a question of uniformity. If -f is case insensitive, shouldn't -a (and others) be as well? Without a fair amount of investigation, it's hard to be confident that case changes won't cascade into other parts of the code that aren't case-aware and cause annoying problems.

@kkirsche

This comment has been minimized.

Contributor

kkirsche commented Oct 16, 2018

From my perspective it comes down to tool intelligence. Tools should have intelligent understanding of their options and adjust user input accordingly either automatically via forced downcasing or via suggestions, "did you mean c"? It's frustrating that the tool can't make basic adjustments to user arguments that, in all ways except case, match a known option.

Honestly, I think all options should have this intelligence if it's applicable to the type of values the option takes. With that said, this is the one that most commonly bites me and that I see bite other people, as MSF doesn't follow the naming conventions of the language naming styles (which commonly leverage case). For this reason, I often see developers using "proper" naming (C, Python, etc.) instead of the lowercase naming used within the tool.

@acammack-r7

This comment has been minimized.

Contributor

acammack-r7 commented Oct 17, 2018

I like this as a user-interface thing, but scattering all these #downcases in library code doesn't feel right to me. I think adding a single f.downcase in the option parsing (https://github.com/rapid7/metasploit-framework/blob/master/msfvenom#L93) would be better.

@kkirsche

This comment has been minimized.

Contributor

kkirsche commented Oct 22, 2018

Sorry for the delay in getting back on this. OSCE exam prep and exam took over my time.

I don't mind making that change (to use the single f.downcase) if that's the preferred path. Is that the only change that should be made?

@acammack-r7

This comment has been minimized.

Contributor

acammack-r7 commented Oct 24, 2018

No worries @kkirsche, life gets busy! I think that's only change needed to get the PR merged, if we decide we want to make this a deeper property of the code we can always propagate it down.

A related change you could add to this PR or do separately if you wanted would be to print the formats when an invalid one has been chosen. You could do this by changing the exception raised in https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/base/simple/buffer.rb (lines 61 and 91 at time of writing) from ArgumentError to a new error type that inherits from it (like class BufferFormatError < ::ArgumentError; end) and raise the new error type. You could then handle it specifically in https://github.com/rapid7/metasploit-framework/blob/master/msfvenom#L440 and print out a message and the result of dump_formats. Totally optional, but would really the solidify the UX of this piece of code.

@kkirsche

This comment has been minimized.

Contributor

kkirsche commented Nov 4, 2018

Pushed that update with the dump_formats change and the change to where format downcasing occurs

@kkirsche

This comment has been minimized.

Contributor

kkirsche commented Nov 12, 2018

Following up on this, is there any additional information that you need from me to be able to review and process this?

@acammack-r7

This comment has been minimized.

Contributor

acammack-r7 commented Nov 21, 2018

Sorry for the delay @kkirsche looks good!

@acammack-r7 acammack-r7 self-assigned this Nov 21, 2018

@acammack-r7 acammack-r7 merged commit ad58930 into rapid7:master Nov 21, 2018

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

acammack-r7 added a commit that referenced this pull request Nov 21, 2018

msjenkins-r7 added a commit that referenced this pull request Nov 21, 2018

@acammack-r7

This comment has been minimized.

Contributor

acammack-r7 commented Nov 21, 2018

Release Notes

The format -f option for msfvenom is no longer case-sensitive, allowing for formats like C and Python.

@kkirsche

This comment has been minimized.

Contributor

kkirsche commented Nov 22, 2018

No worries, life happens, especially around the holidays. Thanks for the review and feedback — always happy to contribute

@kkirsche kkirsche deleted the kkirsche:msfvenom_fmt_caseinsensitivity branch Nov 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment