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

Deprecate auto sanitize of API parameters & Common::getRequestVar and introduce Request class #19624

Merged
merged 12 commits into from Oct 6, 2022

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Aug 12, 2022

Description:

In a later major release of Matomo (maybe 6 or 7) we maybe want to finally get rid of the sanitizing system we are currently using. As discussed in #11786 in a first step this PR introduces

  • A new API property that allows to disable the autosanitize for that specific API
  • A new class Request was introduced, that allows fetching raw parameters from a request

In addition this PR marks Common::getRequestVar as deprecated, as we should prefer using the request class afterwards and sanitize/escape the values when needed manually.

For Matomo 5 we might then consider to change the autosanitize property for APIs to default to false.

refs #11786

Review

@sgiehl sgiehl added this to the 4.12.0 milestone Aug 12, 2022
@sgiehl sgiehl changed the title Deprecate auto sanitize of API parameters and introduce Common::getRequestParam Deprecate auto sanitize of API parameters & Common::getRequestVar and introduce Request class Aug 18, 2022
@sgiehl sgiehl force-pushed the autosanitize branch 3 times, most recently from aef0bd2 to ae1be1e Compare August 19, 2022 15:47
@sgiehl sgiehl requested a review from bx80 August 22, 2022 19:31
@sgiehl sgiehl marked this pull request as ready for review August 26, 2022 10:05
@sgiehl sgiehl added the Needs Review PRs that need a code review label Aug 26, 2022
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

I've reviewed the code and tested the new parameter. It all seems to work as expected and I can't see any issues.

@sgiehl
Copy link
Member Author

sgiehl commented Aug 29, 2022

@bx80 thanks for the review
@justinvelluppillai @tsteur Any objections in merging this one for 4.12?

Would create a PR for developer docs afterwards, as I guess they suggest to use Common::getRequestVar in a couple of places. But guess it makes sense to only suggest using the new Request class as of Matomo 5, as plugins won't be compatible with older version otherwise.

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

@sgiehl Left a comment.

Can we confirm we've done a security review / plan on how to prevent possible issues coming from this change for example related to null bytes? (in PHP and in JS). Things like eg <IMG SRC=java\0script:alert(\"XSS\")> which may bypass filters/validations etc. Also encoding wise for example are we confident there can't be any possible security regression from this? Is there anything to consider that could possibly create any security regression?

I see we mostly replaced it where we used to use Common::unsanitizeInputValue so it may be less of an issue for these but going forward if this was used in other places or by plugin developers then any possible security issue we need to prevent in another way or mention if they need to take care of extra steps etc.

Regarding 4.12: It could maybe wait for Matomo 5 unless there was some user related issue this solves that would be worth having in 4.12? This could also give more time later changing the API if needed etc .

plugins/CoreAdminHome/Controller.php Show resolved Hide resolved
@sgiehl
Copy link
Member Author

sgiehl commented Sep 2, 2022

@tsteur I've done some testing regarding null bytes for the places where I already replaced the usage of Common::getRequestVar. At least there it wasn't an issue. But when we start using the new class anywhere else, we for sure always need to keep in mind, that all values needs to be escaped/encoded properly before being used anywhere.
For string values we though could evaluate if it might make sense to always remove null byte values. Can't think of a good reason where null byte values might be useful, expected or required.

Regarding the version to merge this to. There is no specific issue this will solve for Matomo 4.12. But I thought it might be good to have it in already, so plugin developers might already see the upcoming changes.
Also my plan was to change the default value of the API property for autosanitizing with Matomo 5 and manually set it to false in every API (of every plugin), we did not yet refactor for unsanitized parameters. If we have this PR in 4.12, we could already prepare that upfront.

@tsteur
Copy link
Member

tsteur commented Sep 5, 2022

As other plugins may start using these methods, it be great to have it secure from the beginning. I reckon as well that we could always remove the null byte as it shouldn't have any downside I suppose. Not sure if there could be any kind of issues re encoding?

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Sep 5, 2022
@sgiehl
Copy link
Member Author

sgiehl commented Sep 6, 2022

As other plugins may start using these methods, it be great to have it secure from the beginning.

I'm not sure if that is the correct perspective on that. Imho any code using any type of input parameters should be responsible for handling them safely and correct. The implementation of some global security layer, like Common::getRequestVar, actually brought us in a situation that is causing more and more problems, where some might not even be solvable, as we used to store sanitized data in the database, which can't be undone.

We will still have some global methods that can be used to make a string "safe" for usage in the UI. Where this is needed Common::sanitizeInputValue can be used. That one also removes the null bytes.

Personally I wouldn't change anything else here. But we for sure should update our documentation, so it's clear for a developer that using the new Request class, requires to handle those values with care.

@tsteur
Copy link
Member

tsteur commented Sep 6, 2022

As other plugins may start using these methods, it be great to have it secure from the beginning.

With that I mean to have eg the null byte removed, and having docs what people need to be aware of when using these methods. Like what potential security issues could arise? What do they need to pay attention to? People generally don't know much about security and we need to make sure to explain possible security issues that could arise (eg are there any encoding issues) etc so that they won't be running into these problems.

Imho any code using any type of input parameters should be responsible for handling them safely and correct.

Generally, I would say Security is the most important for us at Matomo and the more built-in security we have, the better. You can put up with certain data issues but you can't put up with security issues. The guide could even recommend to use Common::getRequestVar unless they are aware of all possible security issues as outlined (if there are any, there may not even be any). People wouldn't know they need to sanitise a string and that it's even an option. In an ideal world, every developer would be aware of all possible issues, but that's just not the case.

The question really is what possible security issues could arise from this change? And if there is anything, we need to have the security risk assessed and have it documented.

@sgiehl
Copy link
Member Author

sgiehl commented Sep 7, 2022

From the changes in this PR there shouldn't be any security risk. Our docs should currently all advise to use Common::getRequestVar to get any request parameters, so that should still be safe.
For Matomo 5, we would change that to advise using the new Request class instead, but make it clear that this requires to handle those parameters with care.

@tsteur
Copy link
Member

tsteur commented Sep 7, 2022

I was more meaning if there's any change or risk in general regarding security by using this new class not just by the changes in this PR.

I'm meaning this because the new class is mentioned in the defveloper changelog and we tell developers to use the new one.

That means we'd need to analyse what could potentially go wrong if they did use the new methods which some might do and not just the changes in this PR. We wouldn't want any security issues to arise from this. If there's no change in terms of security then it'll be easy. If any kind of security issues around encoding could come up from this change then we'd need to explain this or see if we can prevent the issue in another way.

We could also just remove the changelog entry for now and then it maybe wouldn't be an issue. Note also that it's marked as API in the developer changelog but there's no @api property in the class itself I believe meaning it wouldn't appear on the developer website API reference. Maybe it's just easier to wait for Matomo 5 then we can also properly update the documentation on the developer website directly etc?

Should we also still maybe directly remove the null byte as that shouldn't cause any issues but improves security by default? Or what would be the disadvantage of not removing it?

As we're still supporting Common::getRequestVar for this release it be good to keep the @api parameter maybe. It's just a minor thing though. Otherwise the documentation for the method would disappear from the API reference.

@sgiehl
Copy link
Member Author

sgiehl commented Sep 9, 2022

@tsteur I've now updated the PR.

  • Common::getRequestVar is now still marked as API, even though its also marked as deprecated
  • the new Request class is now also marked as API
  • the Request class will now automatically filter all null byte values from strings
  • I've updated the CHANGLOG so it's clear that Common::getRequestVar will not be removed before Matomo 6

So the "only" security related risk that remains when using the Request class instead of Common::getRequestVar is that htmlentities is not automatically applied. But for twig templates that actually does not really matter, as twig does automatically encode them, unless |raw is used. Also vue should handle all received parameters safely, unless they are bound as html on purpose.

Personally I think it would be fine to merge this for Matomo 4.12. But if you insist, I can also rebase and merge that to Matomo 5 instead.

@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Sep 24, 2022
@justinvelluppillai justinvelluppillai modified the milestones: 4.12.0, 5.0.0 Oct 5, 2022
@sgiehl sgiehl changed the base branch from 4.x-dev to 5.x-dev October 6, 2022 08:17
@sgiehl
Copy link
Member Author

sgiehl commented Oct 6, 2022

I've rebased the PR to 5.x-dev. If tests are passing I'll merge this one, as all remaining objections were if it would be safe to merge it to 4.x-dev or not.

@sgiehl
Copy link
Member Author

sgiehl commented Oct 6, 2022

Note: All test failures are random and unrelated to the changes here.

@sgiehl sgiehl merged commit 41ddfc2 into 5.x-dev Oct 6, 2022
@sgiehl sgiehl deleted the autosanitize branch October 6, 2022 09:46
bx80 pushed a commit that referenced this pull request Nov 25, 2022
… introduce Request class (#19624)

* Introduce new API property to disable autosanitizing

* Adds new getRequestParam method to replace getRequestVar somewhen

* use new method in some places

* Introduce new request class instead of Common::getRequestParam

* Improve Request class and add tests

* Adds changelog

* clean up api proxy

* code improvements

* Added doc blocks

* filter null byte values

* update tests

* update changelog
@sgiehl sgiehl added Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. and removed Stale The label used by the Close Stale Issues action labels May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants