Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preventing deleting any file in registry by mistake and filtering paths in topics #13980

Merged

Conversation

alroniks
Copy link
Collaborator

@alroniks alroniks commented Jul 7, 2018

What does it do?

It fixes some issues with path traversal in the cache manager and determining file extensions for deleting.

Why is it needed?

It is a security fix, let's discuss details personally in Slack.

Related issue(s)/PR(s)

none

@alroniks alroniks requested a review from bezumkin July 9, 2018 09:25
@alroniks alroniks changed the title Fixed path traversal and type recognizing for deleting Preventing deleting any file in registry by mistake and filtering paths in topics Jul 9, 2018
@alroniks
Copy link
Collaborator Author

@opengeek @Mark-H @Jako
I've updated PR. Please, have a look asap, because it is a security issue.

@alroniks
Copy link
Collaborator Author

@OptimusCrime Hi, could you have a look?

* @return string The sanitized path
*/
protected function sanitizePath($path) {
return preg_replace(array("/\.*[\/|\\\]/i", "/[\/|\\\]+/i"), array('/', '/'), $path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason the two expressions can not be merged to /\.*[\/|\\]+/i? It should match both ../, ..//// and ////. You might also want to consider an expression such as (?:\.*[\/|\\]+)+. This matches ../../....////// too.

@OptimusCrime
Copy link
Contributor

Looks fine to me. The expression can be improved/combined I think. I do not have the security issue available, so I do not know just what the attack vector is.

Copy link
Member

@opengeek opengeek left a comment

Choose a reason for hiding this comment

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

In my opinion, all files in the specified registry topic should be removed, so I find the extensions part of this extraneous. I'm going to submit a security fix that addresses only the path traversal issue. We can discuss the extensions issue later. Regardless, the change to _build/transport.core.php should not be in here.

@opengeek opengeek dismissed their stale review July 12, 2018 00:55

Never mind, just merging this…

@opengeek opengeek merged commit 9f46bd4 into modxcms:2.x Jul 12, 2018
@opengeek opengeek added this to the v2.6.5 milestone Jul 12, 2018
@opengeek opengeek added bug The issue in the code or project, which should be addressed. area-security urgent The issue requires attention and has higher priority over others. labels Jul 12, 2018
@alroniks alroniks deleted the fix/modfileregister_remove_only_configured_files branch July 31, 2018 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-security bug The issue in the code or project, which should be addressed. urgent The issue requires attention and has higher priority over others.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants