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

Jump to bottom

When verifying a nonce, an empty referrer should be allowed. even though expectedReferrerHost is specified #20061

Closed
wants to merge 6 commits into from
Closed

When verifying a nonce, an empty referrer should be allowed. even though expectedReferrerHost is specified #20061

wants to merge 6 commits into from

Conversation

kl-he
Copy link
Contributor

@kl-he kl-he commented Nov 29, 2022

Description:

if an expectedReferrerHost is passed, the nonce-check will fail, if there is no referrer. This was introduced in #17228, as a fix when using GoogleAnalyticsImporter.
While using GoogleAnalyticsImporter, we are seeing a referrer being sent by the browser, so this this will cause the check to fail, as the empty referrer doesn't match google.com.

This fix allows empty referrers again, while also doing expectedReferrerHost and/or localhost, when referrer is specified.

Tests added as integration tests.
./console tests:run --testsuite integration --filter NonceTest

Review

@kl-he
Allow empty referrer with expected referrer set
9177880 
Expected referrer would fail a nonce-check, if the referrer was empty or
local. Introduced in 715ded1/matomo-org#17228.
This fix "reverts" to a pure nonce-check, if referrer is empty or
local.

Issue encountered using the the GoogleAnalyticsImporter, also referred
to in matomo-org#17228.
@kl-he
Fixed misunderstanding with localhost-handling
66e5d48 
Previous fix would allow localhost, even though expectedReferrerHost
specified something else.

If a referrer is present, it must fail if it is not localhost or
expectedReferrerHost.
@kl-he
Updated some comments to match what is happening
181824e
@sgiehl
Copy link
Member

sgiehl commented Dec 1, 2022

Hi @kl-he
Thanks for contributing this PR. That's very appreciated.
The code imho looks fine and it's awesome you also added some tests.

I'm not 100% sure about possible security implications of removing that check. In theory that would now allow to simply bypass the expected referrer check by suppressing the referrer header.
On the other side I guess there might be browsers or addons, that suppress that header on purpose for privacy reasons maybe...
Looking at the code before #17228, it looks like empty referrer were accepted as well. So might be fine to simply merge this one.

@tsteur Any thoughts or objections on that one?

@sgiehl sgiehl added this to the 4.13.1 milestone Dec 1, 2022
@tsteur
Copy link
Member

tsteur commented Dec 2, 2022

@kl-he could you maybe explain the use case a bit more why you need the change? This will help understand things.

@mattab mattab modified the milestones: 4.13.1, 5.0.0 Dec 5, 2022
@kl-he
Copy link
Contributor Author

kl-he commented Dec 5, 2022

We are in the opposite situation as diosmosis was in, when #17228 was submitted.

We are not seeing a referrer being sent (tried with different browsers and distros), when authenticating the Google Analytics Importer plugin, resulting in a failed authorization attempt.

Without #17228 we would not have had a problem, as it allowed empty referrers, as @sgiehl correctly points out.

This pull-request will allow the empty referrers again, but still do the domain-check, if a referrer is passed.

If you go ahead with this pull-request, it might make sense to rename $expectedReferrerHost to $allowedReferrerHost.

@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 Dec 20, 2022
@sgiehl
Copy link
Member

sgiehl commented Dec 21, 2022

@tsteur any further notes on this one? I'm otherwise tempted to merge it.

@tsteur
Copy link
Member

tsteur commented Dec 21, 2022

@sgiehl figured it's best to do a search on OWASP's thoughts on this so checked https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#verifying-origin-with-standard-headers and https://owasp.org/www-community/attacks/csrf

Maybe there's a redirect that gets rid of the referrer.
So it seems fine to merge. However, as suggested by @kl-he we could indeed rename the variable to like allowedReferrerHost and we should make clear in the comments for the method how it behaves and that this is expected behaviour that when no referrer is there that it's not checked.

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Dec 22, 2022
@sgiehl
Copy link
Member

sgiehl commented Dec 23, 2022

@kl-he could you maybe apply those suggestions

@kl-he
Renamed expectedReferrer to allowedReferrer
2b1bad0 
Renamed expectedReferrerHost to allowedReferrerHost.
Also updated doc-blocks and comments, to reflect the changes.
@kl-he
Updated comments
7e46a87
@kl-he
Merged 4.x-dev
3038bbd
@kl-he
Copy link
Contributor Author

kl-he commented Dec 27, 2022

@sgiehl Done

@mattab mattab added the 5.0.0 label Jan 4, 2023
@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 Jan 19, 2023
sgiehl
sgiehl requested changes Jan 19, 2023
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left a comment regarding a failing test. Otherwise this looks good to me
@kl-he could you adjust the test and resolve the conflicts with or branches, then I'll try to merge this one before the next release.

tests/PHPUnit/Integration/NonceTest.php

public function testVerifyNonceWithErrorMessage_validNonceAndLocalReferrerWithNoAllowedReferrer_expectEmptyString()
{
$this->setReferrer('http://app'); // The "local" host when running via CLI.
Copy link
Member

Choose a reason for hiding this comment

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

I guess this test is meant to test with the configured hostname as referrer. Might be better to fetch the host with Config::getHostname() so it works on all instances.

Suggested change
$this->setReferrer('http://app'); // The "local" host when running via CLI.
$this->setReferrer('http://' . \Piwik\Config::getHostname()); // The "local" host when running via CLI.

@MatomoForumNotifications
Copy link

This pull request has been mentioned on Matomo forums. There might be relevant details there:

https://forum.matomo.org/t/google-analytics-importer/48279/2

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Feb 28, 2023
@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 Mar 15, 2023
@sgiehl sgiehl mentioned this pull request Mar 16, 2023
Merged
11 tasks
@sgiehl
Copy link
Member

sgiehl commented Mar 16, 2023

Not mergable anymore due to conflicts. Created #20479 to replace this PR.

@sgiehl sgiehl closed this Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl requested changes

Assignees
No one assigned
Labels
Stale The label used by the Close Stale Issues action
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kl-he @sgiehl @tsteur @MatomoForumNotifications @mattab