Fix: Client DOM XSS

[thiagoalvescosta]

Description:

The method getPathName embeds untrusted data in generated output with href, at line 2675 of piwik.js. This untrusted data is embedded into the output without proper sanitization or encoding, enabling an attacker to inject malicious code into the generated web-page.

Review

• Functional review done
• Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• Security review done
• Wording review done
• Code review done
• Tests were added if useful/possible
• Reviewed for breaking changes
• Developer changelog updated if needed
• Documentation added if needed
• Existing documentation updated if needed

[sgiehl]

@thiagoalvescosta Thanks cor creating this PR. There is a reason why we don't/can't use more modern constructs like new Url. Our tracking code should work correctly also in older browsers, that might not yet support such things.

Also I'm unable to follow the security problem you are trying to mitigate with that. The created a element isn't placed anywhere, therefor it should only be a in memory thing and can't be clicked. Therefor I can't see a reason how that would allow XSS.

[thiagoalvescosta]

The getPathName method embeds untrusted data into an href attribute, even if the generated a element remains in memory and is not added to the visible DOM. While this might seem harmless at first, it still poses a security risk. Below is a detailed analysis and justification for addressing the vulnerability:

Real Risk of XSS (Cross-Site Scripting):

Even though the a element is not actively rendered in the DOM, there is still potential for exploitation. For instance, accidental references or future code changes could expose the element or its properties, making them accessible to attackers.

Trusting unvalidated data is inherently risky, as attackers are constantly finding ways to leverage seemingly insignificant vulnerabilities.

Example of Possible Exploitation:

The value embedded in the href attribute could be used by other JavaScript functions unintentionally. For instance, the property href might be extracted or manipulated elsewhere, enabling DOM XSS or open redirection attacks.

Proposed Mitigation:

Always sanitize and encode all data before embedding it into the DOM or manipulating it. Even if new URL is ruled out for compatibility reasons, custom sanitization and encoding can be implemented.

A safer implementation example:

var url = someUntrustedInput; // Untrusted input
var safeUrl = encodeURI(url); // Sanitization and encoding
var anchorElement = document.createElement("a");
anchorElement.href = safeUrl;

Additionally, implementing a robust Content Security Policy (CSP) would further mitigate risks by restricting the execution of untrusted scripts.

Importance of Preventive Measures:

DOM XSS vulnerabilities are often exploited in unpredictable ways by attackers. Even if this issue does not seem critical today, adopting proactive security measures ensures the system remains resilient against future threats or changes in code that might inadvertently introduce vulnerabilities.

Compatibility with Legacy Browsers:

If older browser support is a key concern, fallback mechanisms can be introduced. For example, detecting the availability of modern methods and, if absent, applying simpler alternatives.

This defense emphasizes the risks and provides practical recommendations to resolve the issue. It also highlights the importance of robust security practices to mitigate vulnerabilities effectively.

[thiagoalvescosta]

@sgiehl I created another code considering old browsers.

[sgiehl]

Wouldn't that cause a double encoding? If you e.g. have a url like https://matomo.org/path%20test/?x=yt, this code would then result in /path%2520test/ as pathname.
Also I guess some automatic encoding also happens there. If you use parser.href = 'https://matomo.org/path test/?x=y'; (without calling encodeURI) the path name will return /path%20test/

[thiagoalvescosta]

You are correct. Since I didn't have any cases with paths with special characters, I didn't pay attention to that. I made a new version. Simpler, but it passes the Checkmarx AST scanner.

I made a secure sanitize function:

           function sanitizeURL(url) {
                if (!/^https?:\/\//i.test(url)) {
                    throw "Invalid URL";
                }

                return url.replace(/[<>]/g, "");
            }

[michalkleiner]

@thiagoalvescosta how would that handle e.g. mailto:[email protected]?subject=hello or tel:123456789 links? For those the function should return an empty string, with your suggested change it would throw an exception.

[thiagoalvescosta]

From what I understood, you just wanted the path of a given base URL. But if tel: or mailto: are important, I changed it and it is passing Checkmarx AST scanner successfully:

            function sanitizeURL(url) {
                if (/^javascript:/i.test(url)) {
                    throw "Invalid URL";
                }

                return url.replace(/[<>]/g, "");
            }