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

Refactored MageRun plugin; added frontname route checking for unknown modules. #23

Merged
merged 2 commits into from
Mar 6, 2019

Conversation

rhoerr
Copy link
Collaborator

@rhoerr rhoerr commented Mar 2, 2019

Per #11 (comment) and #20 (comment): Checking frontnames against those on installed modules would be useful in cases where we don't know what module a given frontname actually corresponds to. This will let us add suspicious routes from probes (#8, #20), and detect on those routes, without knowing further information on the modules in advance. In many cases the modules in question will be significantly out of date or used on a relatively small number of stores. It may also help with data collection by encouraging anyone running them to get in touch.

The original magerun script was pretty small and clean, but adding this code made it the opposite of small and clean, so I significantly refactored it in the process.

The previous module check is now contained within checkIsInstalledModule(), and the new check is within checkIsInstalledRoute().

The route check will run only for modules that are not directly known.

It uses the existing 'Relevant URI' data; no further normalizing needed. It does assume that the frontname is the first element, with or without a leading slash. It ignores anything after. It would be theoretically possible to check for a full controller/action match when the data is available, but that would be much more involved and also much more likely to produce false negatives.

There is a risk of false positives, especially for certain common keywords (EG ajax, vendor). This is unavoidable. In the event of a route match, I have it output:

Matched by route: This may be a false positive where your installed module
shares it with a vulnerable module, but it should be investigated further.
Please contribute info about the module to MageVulnDb if it is relevant.

2019-03-01_223202

I did not make any changes to the standalone execution script. Adding the same feature there would easily quadruple its size.

Also have not made any changes to the readme. I don't think any are necessary.

@gwillem
Copy link
Collaborator

gwillem commented Mar 2, 2019

Great work! I'm hoping @mpchadwick wants to do the code review, my PHP skills are lousy.

Copy link
Collaborator

@mpchadwick mpchadwick left a comment

Choose a reason for hiding this comment

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

A few comments, but mostly LGTM.

@mpchadwick mpchadwick merged commit 75cc370 into master Mar 6, 2019
@rhoerr rhoerr deleted the frontname-checking branch March 6, 2019 14:52
@rhoerr rhoerr mentioned this pull request Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants