Skip to content

fix: deny php execution inside vendor folders under wp-content - #21

Open
YvesCesar wants to merge 1 commit into
mainfrom
fix/deny-php-in-vendor
Open

YvesCesar wants to merge 1 commit into
mainfrom
fix/deny-php-in-vendor

Conversation

@YvesCesar

Copy link
Copy Markdown
Member

The nginx of the WordPress stack runs any .php file under the web root. The test suites of the LibreSign plugins and theme install a complete WordPress, the WordPress test suite and WooCommerce into vendor/ inside their own folder, which is under wp-content/ in this stack. After a composer install there, http://localhost/wp-content/themes/libresign/vendor/wordpress/wp-admin/setup-config.php opened a fresh WordPress installer, and the port is bound to all interfaces, so anyone on the network could point it at their own database and run code in the container. The same happened with the plugins that have that suite, such as woocommerce-restrict-switch.

This denies running PHP from any vendor/ or vendor-bin/ folder under wp-content/. Static files there are still served.

Verification

With the rule loaded on the local stack:

  • 403 for the installer and the test suite files under the theme and woocommerce-restrict-switch, and for plugins/woocommerce/vendor/autoload.php, also with the path in upper case, with ./ and with %76endor.
  • 200 for the home, the store, a product, /lost-password/, wp-login.php, the theme and WooCommerce stylesheets, and the admin pages (dashboard, WooCommerce settings, products, Site Editor).
  • No page of the site references a file under a vendor/ folder.

Signed-off-by: YvesCesar <yvesamorim73@gmail.com>

@vitormattos vitormattos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The security issue identified here is valid, but I don't think this fix should live only in LibreSign/saas.

The PHP execution behavior comes from LibreCodeCoop/wordpress-docker, and the proposed vendor / vendor-bin restriction is generic WordPress runtime hardening rather than LibreSign SaaS-specific configuration. I think this should be implemented upstream in wordpress-docker, ideally with security tests covering plugin/theme vendor directories and vendor-bin.

While reviewing this, I also noticed a related architectural problem: saas currently replaces the entire nginx default.conf. Because of that, it has already diverged from upstream and is missing protections that wordpress-docker now has, including the XML-RPC hardening include and the rule denying PHP execution under wp-content/uploads.

So I think we should:

  1. move this hardening to LibreCodeCoop/wordpress-docker;
  2. add the corresponding tests there;
  3. update the wordpress-docker submodule here;
  4. avoid maintaining a full copy of the upstream nginx config in saas just to apply SaaS-specific overrides such as client_max_body_size.

The vulnerability itself should definitely be fixed, but merging this as-is would solve it only locally while keeping the configuration drift that caused us to miss other upstream security changes.

@YvesCesar

Copy link
Copy Markdown
Member Author

Thanks, @vitormattos

I moved the fix upstream and split it in two PRs:

Once both are merged, I'll update this PR to bump the submodule, drop .docker/wordpress-nginx/default.conf and mount only a client_max_body_size 0; snippet. That also brings the uploads/ PHP rule from #35.

While checking this I found that XML-RPC isn't actually blocked in saas today. WORDPRESS_XMLRPC_ENABLED: 0 from #19 is set on the wordpress service, but the nginx entrypoint is the one that reads it, and our copy of default.conf doesn't have the XML-RPC include. I'll move the variable to the nginx service in this PR too.

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.

2 participants