Clean up empty directories created when file requested does not exist in remote storage.

[colinmollenhour]

Quick fix for #1334

Description (*)

Cleans up empty directories created in the same process.

Related Pull Requests

#601

Fixed Issues (if relevant)

1. Fixes Ability to create new directories on the server via URL #1334

Manual testing scenarios (*)

1. Make a request for a file in a directory that does not exist (/media/foo/bar/baz)
2. Check that no empty directories persist as a result of step 1

Questions or comments

Not a perfect solution since a large number of simultaneous requests with the same subdirectories could still result in some directories being persisted. The random cleanup job (1 in 1000) uses exec and "find" so will only work on Linux with "find" available.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[addison74]

@colinmollenhour The issue #1334 is still present in Open Mage code. Could you please revise this commit and speed up it's merging? Some of your comments should be eliminated from code.

[colinmollenhour]

@colinmollenhour The issue #1334 is still present in Open Mage code. Could you please revise this commit and speed up it's merging? Some of your comments should be eliminated from code.

What revisions and comments are you referring to? I think the benefits of the patch far outweigh the side effects and the provided patch is a significant improvement. The severity of the exploit is pretty low and mitigated reasonably well by this patch so I think this should still be merged in it's current state.

A quick test and code review would help get this merged! Thanks!

[addison74]

This commit was proposed as a quick fix. I was wondering if you have any ideas to improve the code or it is a final one.

About comments in code here is an example:

        if (rand() % 1000 === 0) {
            @exec("find {$this->getMediaBaseDirectory()} -empty -type d -delete"); // TODO - replace with native PHP?
        }

The problem that I see and that could be exploited would be to create as many directors as possible in a short time. The system will allow creating new ones until it takes all the available inodes. It would not be a problem of disk space, but one in which new files / directories can no longer be created.

I confirm I tested your solution and it solves the issue #1334.

[fballiano]

The "find {$this->getMediaBaseDirectory()} -empty -type d -delete" scares me a bit... it seems to me that the solution to the stampede problem kinda leads to more problems than the actual problem no?

[addison74]

If it were up to me I would have left the initial version but he specified that the initial PR represents an improvement and proposed a quick solution to the issue, but since then we do not know if it is a temporary or final solution. Indeed, seeing a Linux command in Magento code is rare.

[fballiano]

@addison74 agree, also create a directory and delete it leads to unnecessary I/O which may load the filesystem. Anyway @colinmollenhour is the best so I trust him completely.

[addison74]

We practically treat the effect and not the cause. The issue is why the change allows the creation of unlimited number of directories. If this is fixed then there is no need to periodically check for folders and delete them. Indeed it is a resource consuming I/O action. In production I didn't use the initial PR from Colin so I didn't need this temporary solution.

[colinmollenhour]

Thanks for the faith in me, Fabrizio! 😉

The facts to consider are:

• The original fix will prevent resource stampeding on busy sites which can easily occur in normal use. Result is fewer failed requests, less resources used and less potential for corrupted files.
• Writing a directory is very small/fast so intentional DoS would not be so easy anyway. With this patch I think it would be no more practical that before the original patch (using a request rate limiter on the webserver is highly recommended regardless).
• In normal use (clients requesting only images that actually exist) there will be no empty directories created so this code won't even execute in the normal case except for some fairly low number of leftover 404d urls.
• Who hosts Magento on Windows? 🤣 The find command could be replaced with native PHP that does the same thing but I venture to guess that find is much more efficient and I know it won't have bugs.
• The find command is just a backup for the automatic cleanup which should be nearly 100% effective, but just in case it isn't due to some odd race conditions there is the find command.. If anything we should just let the directories be created and leave them for the find command to clean up occasionally.

My take is to favor optimizing the common cases over the edge cases so I think the net improvement is still very favorable.

We practically treat the effect and not the cause. ...

I agree that is not ideal but this side effect is not easy to resolve at the root without sacrificing the benefits. The improvement of the original PR is to lock the actual file before writing to it so that conflicts are addressed using proper filesystem-level locking which is 100% atomic - zero potential for race conditions. This requires creating the file's parent directories before you fetch the file data which introduces the issue of creating new directories which will be empty if the file turns out to not exist. I just don't see a way around this without going back to the original code or using some hack like flattening the lock files into a single lock directory.

One idea for improvement would be to add some validation for requests so that files paths are only allowed to be say 10 directories deep which would just make it that much harder to DoS in the first place. E.g.:

if (substr_count($relativeFilename, '/') > 10) {
    sendNotFoundPage();
}

[fballiano]

It's not faith @colinmollenhour, you've proven to be our of the strongest member of the whole Magento community since forever and you've all my respect

[addison74]

I would not call it an exploit in the OM code but it is made public and can be reproduced by anyone in the stores of those who have not yet adopted this manual PR.

I think we shouldn't let the creation of directories go unlimited and we should control it in some way and then periodically delete what was created. Can it be added to this PR that when a visitor starts to create manual/automatic directories to be recorded in the system.log file as an error together with IP? I am considering using Fail2Ban to block IP's for a while. In addition I would know if such a events happen for an evaluation.

[colinmollenhour]

@addison74 I think effectively you can already do that with regular web logs using any 404 response code within the /media directory. I'd be pretty forgiving though just in case some glitch causes legitimate users to trigger a burst of 404s.

[addison74]

I agree t is a solution but I remain of the opinion that an exception written in the Magento log is a better solution. I see that you have already proposed one and thank you for considering it.

If there is nothing to add this PR should be merged.

[github-actions]

Unit Test Results

1 files  1 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌
7 runs  5 ✔️ 2 💤 0 ❌

Results for commit aa88658.