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

[WIP] Fixing CLI-Handling #2749

Closed
wants to merge 5 commits into from
Closed

[WIP] Fixing CLI-Handling #2749

wants to merge 5 commits into from

Conversation

nzqo
Copy link
Contributor

@nzqo nzqo commented Mar 8, 2023

(Starting with) fixing beef cli option handling. Fixes #2174
Note that this is WIP, I'm opening this to get some feedback on how this should be handled exactly.

Category

Core Functionality

Feature/Issue Description

Q: Please give a brief summary of your feature/fix
A: Beef options were not part of the --help before and not clearly communicated.

Q: Give a technical rundown of what you have changed (if applicable)
A: The problem seems to pertain to Sinatra. Since some extensions use Sinatra, it is required from loader.rb (by requiring the bundle defaults). From then on, Sinatra basically hogs the command line parsing, parsing --helpand then exiting. A few key issues:

  • I couldn't find much on how to best solve this. It seems Sinatra provides little options here. The only approach I could think of so far was to basically parse before Sinatra and then "hand over".
  • Inevitably, this means we can't catch invalid options. We could hard-code Sinatra options to check, but if they ever change this would break.
  • Collisions are also hard to detect. I am guessing that for the port option, it is even desired to share. For others, it might not be?
  • I'm still not sure whether this actually works, i.e. whether Sinatra is properly parsing the remaining options. I tried to stipulate an error by passing garbage, but it didn't work. They are either not checking the argument content on parsing or I am still missing something.

Result

With these changes, we would at least see all the command line options being printed when running ./beef --help, i.e.

Usage: beef [options]
    -x, --reset                      Reset the database
    -v, --verbose                    Display debug information
    -a, --ascii-art                  Prints BeEF ascii art
    -c, --config FILE                Specify configuration file to load (instead of ./config.yaml)
    -p, --port PORT                  Change the default BeEF listening port
    -w, --wsport WS_PORT             Change the default BeEF WebSocket listening port
        --update-disable             Skips update
        --update-auto                Automatic update with no prompt
    -h, --help                       Prints this help

Sinatra webapp options:
Usage: beef [options]
    -p port                          set the port (default is 4567)
    -s server                        specify rack server/handler
    -q                               turn on quiet mode (default is off)
    -x                               turn on the mutex lock (default is off)
    -e env                           set the environment (default is development)
    -o addr                          set the host (default is (env == 'development' ? 'localhost' : '0.0.0.0'))

If anyone has a better idea on handling this, I'd be happy to hear and improve this. I hope I didn't miss anything too obvious :)

@github-actions
Copy link

github-actions bot commented Apr 2, 2023

Stale pull request message

@bcoles
Copy link
Collaborator

bcoles commented Apr 2, 2023

I haven't tested these changes but they seem reasonable. Unfortunately I can't merge this. Can you rebase with master branch?

@nzqo
Copy link
Contributor Author

nzqo commented Apr 2, 2023

I haven't tested these changes but they seem reasonable. Unfortunately I can't merge this. Can you rebase with master branch?

Yes, I can rebase this later. But it would be really good if someone could take a closer look at this first, since it also disables proper checks of beef CLI arguments. As I described above, I unfortunately couldn't find an elegant way of handling this.
If you are okay with that though, then that's okay for me as well :)

@bcoles
Copy link
Collaborator

bcoles commented Apr 2, 2023

Looks like this breaks the tests.

Also, we don't care about the Sinatra options. They are supposed to be obscured. If users change these options they are in for a bad time.

@nzqo
Copy link
Contributor Author

nzqo commented Apr 2, 2023

Looks like this breaks the tests.

Also, we don't care about the Sinatra options. They are supposed to be obscured. If users change these options they are in for a bad time.

If we don't care about the Sinatra options then I should probably re-enable the CLI arg checks. This way they would be completely inaccessible. Just noting that this change is still relevant because currently users only see the Sinatra Options.
I don't know why the tests break yet. I can try to have a look at it later, or maybe someone can tell me if you can see something obvious :)

@bcoles
Copy link
Collaborator

bcoles commented Apr 2, 2023

If we don't care about the Sinatra options then I should probably re-enable the CLI arg checks. This way they would be completely inaccessible. Just noting that this change is still relevant because currently users only see the Sinatra Options.

Correct. The current behaviour is incorrect.

I don't know why the tests break yet. I can try to have a look at it later, or maybe someone can tell me if you can see something obvious :)

There are a lot of undefined method `parse' for Module:Class errors.

@nzqo nzqo had a problem deploying to Integrate Pull Request April 2, 2023 14:05 — with GitHub Actions Failure
@nzqo nzqo had a problem deploying to Integrate Pull Request April 2, 2023 14:09 — with GitHub Actions Failure
@nzqo
Copy link
Contributor Author

nzqo commented Apr 2, 2023

If we don't care about the Sinatra options then I should probably re-enable the CLI arg checks. This way they would be completely inaccessible. Just noting that this change is still relevant because currently users only see the Sinatra Options.

Correct. The current behaviour is incorrect.

I don't know why the tests break yet. I can try to have a look at it later, or maybe someone can tell me if you can see something obvious :)

There are a lot of undefined method `parse' for Module:Class errors.

Yes, I can see that. But unfortunately not where they arise or why. It's weird to me since I thought I actually removed all calls to CommandLine.parse and substituted them with get_options. The method still exists though, so I don't really understand this right now.

I reintroduced exiting on invalid options now, though. Sinatra options should now be hidden.

@nzqo nzqo temporarily deployed to Integrate Pull Request April 2, 2023 14:17 — with GitHub Actions Inactive
@nzqo
Copy link
Contributor Author

nzqo commented Apr 5, 2023

@bcoles Can you trigger a rerun of the browser stack test? I would like to see whether my latest changes have changed anything with regards to it breaking

@nzqo
Copy link
Contributor Author

nzqo commented Apr 5, 2023

@bcoles
Im not sure but maybe the error came from calling self.CommandLine.parse() instead of self.parse()... It seems fixed now, at least. I would need to test it once more but the Sinatra options should also be hidden properly now.
Also rebased again.

@github-actions
Copy link

Stale pull request message

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

Successfully merging this pull request may close these issues.

Update ./beef launch flags in wiki and --help menu
2 participants