file_unmanaged_move() in Drupal 8 makes unreadable files readable

Perhaps we should add an explicit check to see if the file is readable before the rename?

Change looks good. Mind filling out the issue summary though for the TBD sections. Will keep an eye out for this again.

Updated the IS

Worth asking:
Should permissions just be retained?
Should owner be retained?

Thinking of other attack vectors (Quick visual review, not confirmed in the dev lab):
1)
File owned by www-data:some-other-group server runs as www-data:www-data permison 600 (only the web-server can read, no other users can) copied to same stream wrapper:
file_unmanaged_move() will (even with this patch) make the file world and group readable, this could allow another process (not the webserver) (through either the group read, or the public read) to read the file.

2)
File owned by www-data:some-other-group server runs as www-data:www-data
file_unmanaged_move() will either end up with the file being www-data:some-other-group OR www-data:-www-data (depends on if rename succeeds which will depend on if it crosses hard mount points or streamWrappers eg public:// to private://

Implied the above question is that this is not limited to just the public:// scheme or a path inside the webserver docroot, it impacts all files that Drupal touches as the webserver would not be the only access vector.

Systems admin hat on: Shell mv usually retains the users and permissions. I could very easily see devs not being aware of this and silently injecting some public exposure in random locations that allows sensitive information to leak.

Tangent: I saw the chmod() line in the filesystem service and didn't even think twice about how far ranging these impacts would be until this issue randomly popped up in my feed.

@cmlara do you think that adding checks for those potential attack vectors would be necessary for this issue to move forward? I can definitely see the concern of changing the owner of the file, however are we, or should we be, hardening to that point?

I'll leave it to someone else to change the status from Needs Review to Needs Work based on the comment, but I definitely agree that if it is a concern then maybe we should address it ahead of time.

however are we, or should we be, hardening to that point

Is it hardening or fixing a bug would be a more fundamental question.

The move function is called move not move_and_chmod_public_read().
As I alluded to before, i work in the filesystem service as a contrib streamWrapper maintainer (s3fs) and this never even registered in my mind quick reading the code to implement in a decorator.

I can’t imagine the average developer has thought how this might put their deployment at risk and did not expect move() to function as a normal move. For context I did see the Windows workaround at 0600 and always thought “ok private no big deal maybe breaks some stuff but should fail closed”

I suspect the DST and core team are underestimating the culpability of core in this.

I’m out with the flu so not in a spot to go digging through the CVE’s, CWE’s and CAPEC’s to find possible citations.

The key part is this is two fold:

1. We silently will perform a copy() when a rename fails (providing no signal to the caller they need to take actions for a change of ownership)
2. We silently change the permissions without the request of the caller.

Considering this method is suppose to be a drop in (more friendly OS agnostic replacement for php’s built-in move() it’s has already taken on higher responsibility. By hiding errors or has taken on the additional responsibility of being the secure error handler.

I ran across a PHP package the other day that went as far as to use flysystem IIRC to do local file management, at one point it seemed like overkill, however when you think of scenarios like this it makes a bit more logical sense that you “trust a library instead of trying to implant it all yourself” (not suggesting we convert to flysystem, just pointing out file management responsibilities can be large).

There a few assumptions in the following example however here is an example of what concerns me

User1:

$ touch private.key
$ chown www-user:user-group private.key
$ chmod 460 private.key
User1 securely puts contents into private.key
$ chmod 440 private.key
$ mv private.key /var/www/not_doc_root/private.key

User2 (not a memebr of user-group)

$ cat var /www/not_doc_root/private.key 
> File not readable.

Some time in the future $fileSystem->move(‘/var/www/not_doc_root/private.key’, ‘/var/www/not_doc_root/private-old.key’) (inside a hook_update as part of center magament, as part of a user taking an action in IMCE, whatever).

User2 (not a member of user-group)

$ cat var /www/not_doc_root/private-old.key 
> It is now time to go have some fun :)

The above example shows local however simialr risks occur through remote frontends too (think of a server running multiple HTTP server daemons some process isolation scheme such as multiple docker containers with shared storage).

Couple with full path disclosure vulnerabilities in core and you have even more fun (yes that is talking chained exploits, however it not like those have not happened in the past in 🐛 Maintenance pages leak sensitive environment information. Needs review and that we are sure they will never happen again in the future).

Going back to the IS:

(1) a file in the public files directory that deliberately has the above somewhat-unusual permissions, and (2) a contrib module installed that allows an attacker to affect the input to file_unmanaged_move(). I don't know of such a module off the top of my head,

This would not be limited to just public:// when you consider the local vector, though yes public makes it a LOT easier.

As for contrib module, IMCE would be a high-risk candidate to me unless it has already been proven that it can’t do so.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.