Adding support for PMA 5.1 and up#5
Merged
Conversation
Member
Author
|
Any reviewers / testers? |
williamdes
reviewed
Sep 4, 2022
williamdes
left a comment
williamdes
left a comment
There was a problem hiding this comment.
No rules for the other endpoints: url.php and the setup/ folder ?
Member
Author
|
@williamdes Currently no, have you encountered any problems with that endpoints while using CRS? False positives or so. |
I have not heavy-tested it, however, the basic usage of PMA does not give false positives anymore. |
Member
Author
|
@WaleedMortaja Thank you very much! Which version of PMA are you using? |
|
@azurit PMA 5.2.0 |
|
@azurit I was testing some configs and decided to try PMA setup feature. I found some false positives on the setup "export" page with URL |
Member
Author
|
@WaleedMortaja Can you, please, upload logs from ModSecurity? |
Member
Author
|
@WaleedMortaja Can you try current version? Thank you. |
Member
Author
|
@williamdes As you wished, now we have few rules also for |
Member
Author
|
Old URL format can be considered as tested. |
| ctl:ruleRemoveTargetById=933210;ARGS:prev_sql_query,\ | ||
| ctl:ruleRemoveTargetById=942110;ARGS:sql_delimiter,\ | ||
| ctl:ruleRemoveTargetByTag=attack-sqli;ARGS:sql_query,\ | ||
| ctl:ruleRemoveTargetByTag=attack-xss;ARGS:sql_query,\ |
There was a problem hiding this comment.
I am not an expert, but curious about why are we removing XSS rules from a SQL query parameter? Did XSS cause any problems already?
Member
Author
There was a problem hiding this comment.
sql_query parameter holds the whole SQL command, including the data which can contain anything, for example HTML code (which is triggering XSS rules).
@azurit It still has false postivies. I tried the setup's "export" page only, and got this log. |
Member
Author
|
@WaleedMortaja Thanks! Can you try it with current version? |
|
@azurit the setup/export is working now! Here is the log for setup/import (URL: Here is the log for setup/Main Panel (URL: |
Member
Author
|
@WaleedMortaja Thanks, try now! |
|
@azurit All the setup pages are working now. Thank you for your efforts 😄 |
Member
Author
|
@WaleedMortaja Thank you very much for testing! |
williamdes
reviewed
Dec 27, 2022
| ctl:ruleRemoveTargetById=942200;REQUEST_COOKIES:pma_console_config,\ | ||
| ctl:ruleRemoveTargetById=942260;REQUEST_COOKIES:pma_console_config" | ||
|
|
||
| SecMarker "END-PHPMYADMIN-RULE-EXCLUSIONS-PLUGIN-V51-URL-FORMAT" |
There was a problem hiding this comment.
Can you limit the URLs that can be called to only the endpoints that 5.1 has?
https://github.com/phpmyadmin/phpmyadmin/blob/QA_5_1/index.php
https://github.com/phpmyadmin/phpmyadmin/blob/QA_5_1/url.php
https://github.com/phpmyadmin/phpmyadmin/blob/QA_5_1/show_config_errors.php
https://github.com/phpmyadmin/phpmyadmin/blob/QA_5_1/js/messages.php
PHP files in https://github.com/phpmyadmin/phpmyadmin/tree/QA_5_1/setup
See my nginx regex https://github.com/sudo-bot/gh-deployer-container/blob/04b50cde53ac13f71c444b09dd31dcecaca3eee2/docker/nginx-default.conf#L29
location ~ ^(?!(\/favicon\.ico|\/robots\.txt|\/index\.php|\/url\.php|\/show_config_errors\.php|\/js\/messages\.php|\/js\/dist|\/js\/vendor|\/doc\/html|\/setup|\/themes|\/)) {
deny all;
}
Member
Author
There was a problem hiding this comment.
@williamdes Not sure about this. What exactly do you expect from it?
There was a problem hiding this comment.
it limits the urls that can be visited adding more security by 403 everything else
Member
Author
There was a problem hiding this comment.
@dune73 What do you think about this? Should it be part of CRS RE plugin? For me, it sounds like it's out of scope.
Member
There was a problem hiding this comment.
Creating an allow-list is a useful security measure, but it's out of scope for a rule exclusion plugin.
If you guys think there is wider interest for such a configuration, I suggest to create a separate allow-list plugin.
williamdes
reviewed
Dec 28, 2022
| ctl:ruleRemoveTargetById=941120;REQUEST_COOKIES,\ | ||
| ctl:ruleRemoveTargetById=942100;REQUEST_COOKIES:auto_saved_sql,\ | ||
| ctl:ruleRemoveTargetById=942200;REQUEST_COOKIES:pmaAuth-1,\ | ||
| ctl:ruleRemoveTargetById=942340;REQUEST_COOKIES:pmaAuth-1,\ |
There was a problem hiding this comment.
Does this assume there is only one server on the configuration?
Member
Author
There was a problem hiding this comment.
Probably. Can you show how cookies looks like for multiple server setup?
There was a problem hiding this comment.
1 is the server ID, any unsigned integer should be allowed
You can view cookies for a multi server setup on our demo server 5.2.x-dev
Member
Author
There was a problem hiding this comment.
Does this apply for both pmaAuth-X and pmaUser-X?
Member
Author
|
@williamdes pls what's difference between routes |
It seems that one is more for tables and the other for databases |
Member
Author
For tables, there seems to be a
No, i just need to know if i should:
It's more or less a philosophical question. :) |
@MauricioFauth you created the controllers, maybe you could better answer this question? But I would say that since it is not prefixed it could be used in different ways, so maybe a new rule? |
Initially the routes were a direct map with the files. For example:
Now, more routes are been added as we are extracting then from the controllers. As a lot of routes are doing too much. Basically, |
Member
Author
|
@MauricioFauth Thanks, that helped a lot! @MauricioFauth @williamdes What about this? What action it was? |
I guess it's the ping pong to check if the session expired? |
|
Hi @azurit |
Member
Author
|
@williamdes Thanks for the info! To all: New URL format can be considered as tested. |
Member
|
Just in case, please use |
Member
|
@azurit To the best of my knowledge, I think I've fixed the conflict. Let me know if this is ready before merging. |
Member
Author
|
@fzipi Thank you! Should be ready to merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes in this PR: