search

thedevdojo / voyager

Voyager - The Missing Laravel Admin
https://voyager.devdojo.com
MIT License
11.8k stars 2.67k forks source link

Security: Compass artisan command is arbitrary #3436

Closed dereks closed 6 years ago

dereks commented 6 years ago

Version information

Description

Any artisan command can be run by the compass user (not just the ones listed in the compass GUI).

Steps To Reproduce

Under /admin/compass > commands.

The php artisan "command" string is pulled verbatim out of the POST request. This means a user can run ANY php artisan command, not just the ones listed in the GUI. All they need to do is hand-craft the POST request. For example, a user can run the "voyager:admin" command even though it is not listed in the GUI.

Expected behavior

To fix this, the code must vet the input data, to make sure the user-requested command is in the list of allowed "php artisan ..." commands. (NEVER trust user submitted data -- code like it is always an attack!)

Alternatively, the GUI must make is absolutely clear that this tool allows you to run any / every php artisan command, and give a box at the top that lets you type in whatever "php artisan ..." you want to run, including "voyager:admin" or anything else. That way there is no false sense of security.

Additional context

Bug #3267

emptynick commented 6 years ago

This is a duplicate of #3032 and #3227. You already found #3267 which will fix the problem.

dereks commented 6 years ago

@emptynick

Thank you for your quick response.

With respect, I believe you are incorrect. This is a separate issue from the bugs you listed.

Both of those appear to be fixed by #3267, as you describe.

This bug report is new and is about the fact that any php artisan command can be run. This is a less severe version of #3032, as it restricts the user to php artisan commands only. But that is still dangerous, as some of those commands can be destructive and they are not all presented as options in the GUI (such as php artisan voyager:admin).

In short, the feature presents itself as allowing the user to run only certain php artisan commands. But in truth it allows an attacker to run any and all php artisan commands. The code should verify the command is one that is okay to run -- or else make it clear that the user can run any and all php artisan commands.

dereks commented 6 years ago

First the code blindly grabs the php artisan command name from the attacker's post:

$command = escapeshellcmd($request->command);

Then the code runs it, regardless of what it says:

$process = new Process('cd '.$path.' && php artisan ' . $command . $args);

Again, the user is limited to just php artisan commands (and not arbitrary shell commands), but that is still unsafe.

If this is intended behaviour, then I don't think it should show a GUI listing just a specific subset of commands. I assumed that only these particular artisan commands could be run, and I had to check the source code to see what it actually did...

dereks commented 6 years ago

I've been thinking through this feature today.

There are many php artisan commands that require filesystem write permissions, e.g. some to create .php files, and some to edit the .env file. These are things a public webserver (e.g. Apache) process should never be able to do, due to the inherent risk. It's a known attack vector with a proven history.

But for developers running a local server with php artisan serve, the webserver will be running under their own UID. That is very convenient for development because the same user can own and edit the .php source code files, including ones generated by php artisan commands, and easily work with it using git. For this, the one-click access to stub creators (like php artisan make:...) is very convenient. (The same is true for users of Laravel Homestead or Valet.)

However, there are some php artisan commands which should clearly be part of the admin panel on a Production webserver. Putting the application into (or out of) maintenance mode, flushing cache, and retrying queue jobs are all examples of things that should be an easy button click on the admin panel. These do not (?) need filesystem access beyond the ones installed for Laravel, and are things one would want to do on a production server.

So what if the php artisan commands are put into two groups:

  1. The "Production" group. If you have Voyager admin privs, you can run these. These are commands an admin might need to run on a Production server. None of these commands require filesystem write access by the webserver (but they might modify the database using the DB_PASSWORD in .env, or modify Laravel's files under ./bootstrap/cache and ./storage).

  2. The rest of the commands. These are suitable for development only, and might touch the filesystem. For these, we assume a development environment where the webserver runs under the UID of the developer.

As for user interface, I think the second group should be disabled (made grey) unless APP_DEBUG=true in the .env file. This way the commands are all still visible in Voyager (with the help text), even though the admin may need to log in remotely via SSH to execute them. This will prevent developers from trying to tell admins to click buttons that don't exist if an emergency comes up. Maybe there could be a button to copy the command to clipboard for easy paste into a shell terminal.

As a first draft, here are the php artisan commands I think should be in the Production group.

Laravel Framework 5.6.28

Usage:
  command [options] [arguments]
...
Available commands:

  down                 Put the application into maintenance mode
  env                  Display the current framework environment
  up                   Bring the application out of maintenance mode
  migrate              Run the database migrations
  list                 Lists commands
  help                 Displays help for a command
auth
  auth:clear-resets    Flush expired password reset tokens
cache
  cache:clear          Flush the application cache
  cache:forget         Remove an item from the cache
config
  config:clear         Remove the configuration cache file
  config:cache         Create a cache file for faster configuration loading
 migrate
  migrate:fresh        Drop all tables and re-run all migrations
  migrate:install      Create the migration repository
  migrate:refresh      Reset and re-run all migrations
  migrate:reset        Rollback all database migrations
  migrate:rollback     Rollback the last database migration
  migrate:status       Show the status of each migration
 queue
  queue:failed         List all of the failed queue jobs
  queue:failed-table   Create a migration for the failed queue jobs database table
  queue:flush          Flush all of the failed queue jobs
  queue:forget         Delete a failed queue job
  queue:listen         Listen to a given queue
  queue:restart        Restart queue worker daemons after their current job
  queue:retry          Retry a failed queue job
  queue:table          Create a migration for the queue jobs database table
  queue:work           Start processing jobs on the queue as a daemon
 route
  route:cache          Create a route cache file for faster route registration
  route:clear          Remove the route cache file
  route:list           List all registered routes
 schedule
  schedule:run         Run the scheduled commands
fletch3555 commented 6 years ago

"any artisan command" is a subset of "any shell command". I don't see how this isn't a duplicate of #3032.

I would be open to a pull request where you can list which artisans are allowed in config.

As mentioned in one of your other issues. If you want to restrict access to commands, you can restrict write access to the filesystem by that user. The command will simply fail. Security is ALL of our responsibility, not just that of the package you're using.

github-actions[bot] commented 4 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. If you have further questions please ask in our Slack group.