Really-Simple-Plugins / really-simple-ssl

Easily improve site security with WordPress Hardening, Two-Factor Authentication (2FA), Login Protection, Vulnerability Detection and SSL certificate generation.
https://really-simple-ssl.com
Other
144 stars 43 forks source link

wp_upload_dir() does not always return a valid location leading to a crash (v7.2.3) #607

Open rbubley opened 9 months ago

rbubley commented 9 months ago

In really-simple-ssl/security/wordpress/vulnerabilities/class-rsssl-folder-name.php the code assumes that $upload_dir = wp_upload_dir(); is an associative array that is the upload directory. It doesn't always return the correct location. If it doesn't, then an error is raised that is never caught. In my case, this meant the 'update plugins' page was inaccessible.

rlankhorst commented 9 months ago

Hi @rbubley, thanks for reporting the issue, we'll investigate and get back to you with a fix!

rlankhorst commented 8 months ago

@rbubley sorry for the delay. I'm currently investigating this, but not clear on what situation could cause the core function wp_upload_dir() not to return an array. As the docs state it always returns an array.

Do you have any more detailed error_logs that could shed a light on this? Is it possible that the function is not loaded yet?

rbubley commented 8 months ago

Sorry for the inclarity -- it does return an array but the directory returned doesn't exist.

So, specifically, in my case, which is a shared hosting environment, it returns a path starting: /var/home/redacted-username/public_html/ -- which does not exist. The correct path for me is" /var/sites/c/redacted-domain-name/public_html/

As a result I get and warnings and errors from vulnerabilities/class-rsssl-folder-name.php: a mkdir(): Permission denied warning on line 44, and a Fatal error: Uncaught RuntimeException on line 45.

The main problem, of course, is the uncaught exception, which brings down the site. I don't know how it is possible to figure out the correct directory programatically, but it should hopefully be straightforward to catch and handle the exception if this situation arises.

rlankhorst commented 8 months ago

@rbubley ah, thanks for the clarification, that explains indeed (at least partly). This sounds like the main issue is that the directory existence is not verified, for this we already have a branch, waiting to be merged. https://github.com/Really-Simple-Plugins/really-simple-ssl/tree/writable-%26-folder-exists-checks

What is weird, is that the core function wp_upload_dir() does not return the correct path. That might be a configuration issue on your server: I have to make the assumption in the code that this function returns the correct value.

That said, the writable and is_dir checks will catch any issues that might result of this. Let me know if this helps!