@sgiehl opened this Pull Request on August 12th 2022 Member

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 commented on August 29th 2022 Member

@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.

@sgiehl commented on September 2nd 2022 Member

@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 commented on September 5th 2022 Member

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 commented on September 6th 2022 Member

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 commented on September 6th 2022 Member

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 commented on September 7th 2022 Member

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 commented on September 7th 2022 Member

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 <a class='mention' href='https://github.com/api'>@api</a> 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 <a class='mention' href='https://github.com/api'>@api</a> parameter maybe. It's just a minor thing though. Otherwise the documentation for the method would disappear from the API reference.

@sgiehl commented on September 9th 2022 Member

@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[bot] commented on September 24th 2022 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'.

Powered by GitHub Issue Mirror