[SR] Private files security hardening

[jenlampton]

Backdrop doesn't sufficiently ensure private destination folders exist prior to saving files. If the private directory doesn't exist, Backdrop places the private file in a publicly accessible directory.

I would like to propose a hardening to Backdrop that will delete the file if the copy fails, rather than leaving it in a public location.

STR:

• Install backdrop
• Configure private files, set destination to private for example
• Confirm that the private directory was created, containing the .htaccess file
• Manually delete the whole private directory
• Proceed to the manage files overview page admin/content/files
• Add a new file
• On step 2, set the destination to Private local files served by Backdrop
• See the error The specified file public://license_0.txt could not be moved/copied because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.
• See that the private file remains in the public files location

This has been previously reviewed by the security team, and determined not to be a vulnerability because it can not be triggered by using the UI alone (one would also need access to the server). It is, however, not ideal, and should be improved.

Also related: https://www.drupal.org/sa-contrib-2024-040

[indigoxela]

@jenlampton with this change, 3 functional tests fail in all PHP versions:

• FileUploadSvgTestCase
• FileUploadWizardTestCase
• FileViewsTestCase

Undefined variable $moved_file

Seems like the PR still needs some work.

[izmeez]

On more carefully reading the description, and the possibility that the private files directory may have been deleted from the server, the solution proposed in the PR makes sense.

[quicksketch]

I'm working on test coverage for this PR.

[quicksketch]

I added automated tests and fixed the failing ones. The file upload wizard has some finicky behaviors but I think the PR now has minimal changes to keep the current code-paths in place.

Reproducing this problem within the testing framework is not trivial, since it requires a non-writable directory. Using chmod() to make a directory non-writable will not work, since Backdrop will happily make that directory writable again when file_prepare_directory() is called. So the new test coverage tries to use a directory that it is unlikely to have write access to. I chose /root (the home directory of the root user). Although /root doesn't exist on non-Linux machines, as long as the location is non-writable it should work (even if it doesn't exist).

[quicksketch]

@jenlampton has reviewed so I think this is RTBC.

[quicksketch]

I merged https://github.com/backdrop/backdrop/pull/4868 into 1.x and 1.28.x. Thank you @jenlampton for making the PR! Thanks @indigoxela and @izmeez for feedback!

[indigoxela]

Some past-last-minute testing from my side: works like a charm. :+1: I played locally with it, where I have to ability to chown and delete as needed. :wink:

[quicksketch]

:sweat_smile: Thanks for your testing @indigoxela!