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

Prevent setting pk_ref cookie with an empty string; it is perceived as sql injection attempt #21170

Open
atom-box opened this issue Aug 23, 2023 · 14 comments · May be fixed by #22071
Open

Prevent setting pk_ref cookie with an empty string; it is perceived as sql injection attempt #21170

atom-box opened this issue Aug 23, 2023 · 14 comments · May be fixed by #22071
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.

Comments

@atom-box
Copy link

Expected:
pk_ref will never be set with "" property.

Actual:
pk_ref is sometimes set with: ["", "", 1234567890, "https://www.example.com/"].

Why this is a problem:
Security software throws a false positive, seeing this as a SQL injection attempt.

Here is the original email from our user:

We integrated Matomo Analytics in a react-application that is hosted in azure. When redirected to this page (https://validation.EXAMPLE.ai) from the main homepage of our company (https://www.EXAMPLE.dk) via a link a cookie called _pk_ref.67.352a is being created by Matomo Analytics like so: ["", "", 1234567890, "https://www.EXAMPLE.dk/"].

This causes our application gateway to return HTTP 403 since the web application firewall running in the background assumes this cookie contains an sql injection attempt. This is due to the two double quotes in the cookie. Since the azure web application firewall which we use is commonly used and we do not want to disable checking for sql injections entirely could you provide us with an explanation as to how and why the cookie is created this way and provide potential a fix. Thank you in advance.

--

@atom-box atom-box added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. To Triage An issue awaiting triage by a Matomo core team member labels Aug 23, 2023
@sgiehl
Copy link
Member

sgiehl commented Aug 24, 2023

@atom-box We are setting a json encoded array as cookie value. If no campaign details were provided for the visit, the first two values are empty strings. Which ends up in your example. We can't easily change that and if we would decide to do so, this can't be done before a new major release.
Anyway, I'm not sure if their WAF might be a bit too restrictive in that case. Maybe they should disable that rule for Matomo.

@michalkleiner
Copy link
Contributor

Could we, in that case, not set the cookie at all and treat a cookie that is not set the same way as one without the values?
Or send null string instead indicating an empty value, @sgiehl? That sounds like a minor version enhancement or possibly even a patch fix if the logic processing the cookies treated the empty string and the null value the same way.

@sgiehl
Copy link
Member

sgiehl commented Aug 25, 2023

As that's part of the tracking, the code would need to be able to handle the old and the new style of cookies. Setting an empty cookie wouldn't be the same, as the third and fourth value of the json encoded array are set and required. We could set null instead and implement a handling for that. But t.b.h. I'm pretty sure, that might only fix one of the problems a tracking behind a very restrictive waf would have.

@julipp
Copy link

julipp commented Aug 28, 2023

Hi, I originally raised this issue through the support page
The ruleset for the azure waf is OWASP 3.1 CRS which comes with a range of predefined rules to detect security anomalies. It is common standard choice for a ruleset for an azure waf.
We could exclude requests containing the _pk_ref cookie from certain rule checks - I will discuss this internally with the responsible person and I hope that this should resolve the issue for us.
Whether the OWASP 3.1 CRS rules are too restrictive for you to want to support tracking behind them out of the box is open for discussion and ultimately your choice.

@sgiehl
Copy link
Member

sgiehl commented Oct 9, 2023

@tsteur Do you think this should be fixed?

@sgiehl sgiehl removed the To Triage An issue awaiting triage by a Matomo core team member label Oct 9, 2023
@tsteur
Copy link
Member

tsteur commented Oct 9, 2023

@sgiehl I guess it depends how easy it is to fix. It would be nice if it just worked out of the box with such WAF rules. If it's too much effort, then I would maybe rather wait until more people run into this issue. The least we should do though, is create an FAQ around Matomo and WAF rules. So it's clear that certain rules may be causing issues and trigger eg 403 response codes etc.

@sgiehl
Copy link
Member

sgiehl commented Oct 9, 2023

Guess passing null instead of an empty string should be implemented quite easily.

@sgiehl sgiehl added this to the For Prioritization milestone Oct 9, 2023
@tsteur
Copy link
Member

tsteur commented Oct 9, 2023

Guess passing null instead of an empty string should be implemented quite easily.

We might want to check if this causes similar issues

@Jhfelectric
Copy link

Hello,
I have the same issue on my company's WAF. I don't have access myself but here is what was reported to me from the log (anonymized):

2023:10:18-12:36:08 securitysrv1-2 httpd[18459]: [security2:error] [pid 18459:tid 3861777216] [client 84.123.456.789:36647] [client 84.123.456.789] ModSecurity: Warning. Pattern match "([\\~\\!\\@\\#\\$\\%\\^\\&\\\\(\\)\\-\\+\\=\\{\\}\\[\\]\\|\\:\\;\"\\'\\\xc2\xb4\\\xe2\x80\x99\\\xe2\x80\x98\\`\\<\\>].?){8,}" at REQUEST_COOKIES:_pk_ref.6.8e6a. [file "/usr/apache/conf/waf/modsecurity_crs_sql_injection_attacks.conf"] [line "157"] [id "981172"] [rev "2"] [msg "Restricted SQL Character Anomaly Detection Alert - Total # of special characters exceeded"] [data "Matched Data: - found within REQUEST_COOKIES:_pk_ref.6.8e6a: [\x22\x22,\x22\x22,1697624257,\x22https://some.domain.tld/\x22]"] [ver "OWASP_CRS/2.2.7"] [maturity "9"] [accuracy "8"] [tag "OWASP_CRS/WEB_ATTACK/SQL_INJECTION"] [hostname "other.domain.tld"] [uri "/"] [unique_id "ZS-1GId9oroUJOVy24FkHwAAAOc"], referer: https://other.domain.tld/

My ITs have created some sort of exception for the empty string cookie.
@tsteur so more people see this occurring, and yes, a FAQ would have saved me 2 days ;)

Thanks for the hard work !
Julien

@gheisz
Copy link

gheisz commented Feb 26, 2024

We seem to have the same problem.

@Gazymodo
Copy link

Exactly same issue for us!

@Gazymodo
Copy link

Gazymodo commented Apr 2, 2024

Guess passing null instead of an empty string should be implemented quite easily.

Any idea if this will be fixed.
We are trying to solve this but may have to remove matomo for some of our services at the moment.

@sgiehl
Copy link
Member

sgiehl commented Apr 4, 2024

Hey @Gazymodo,
we currently don't have enough capacity to work on this one. I'll nevertheless create a draft PR with the changes, that might be required to fix this.
We though might not be able to test properly if that would solve the problem. If you have time to validate and test the changes, that might help a lot to get it merged.

@sgiehl sgiehl linked a pull request Apr 4, 2024 that will close this issue
11 tasks
@Gazymodo
Copy link

Gazymodo commented Apr 9, 2024

Hey @Gazymodo, we currently don't have enough capacity to work on this one. I'll nevertheless create a draft PR with the changes, that might be required to fix this. We though might not be able to test properly if that would solve the problem. If you have time to validate and test the changes, that might help a lot to get it merged.

Thank you.
I am not the matomo guy at our place but I asked to get it implemented to be able to test.
Will keep you posted.
If anyone else could do the same, it would be great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants