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

Only allow widgetize and token usage for view users #16263

Merged
merged 21 commits into from Aug 10, 2020
Merged

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Aug 4, 2020

Not really needed this change I guess but might be better to allow authentication using tokens only for users when the used token has only view access. This may be helpful if otherwise a user tries to embed some screens using token_auth.

Should there be any regressions that we can't fix we could undo it again.

@tsteur tsteur added the Needs Review PRs that need a code review label Aug 4, 2020
@tsteur tsteur added this to the 4.0.0 milestone Aug 4, 2020
@diosmosis
Copy link
Member

build is failing, looks like this has some other side effects

@tsteur tsteur marked this pull request as draft August 6, 2020 01:15
@tsteur tsteur added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Aug 6, 2020
@tsteur tsteur marked this pull request as ready for review August 7, 2020 08:19
@tsteur tsteur added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Aug 7, 2020
@tsteur tsteur added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Aug 8, 2020
@tsteur
Copy link
Member Author

tsteur commented Aug 8, 2020

@diosmosis tests should pass now

Request::reloadAuthUsingTokenAuth();
if (($module !== 'API' || ($action && $action !== 'index')) && Piwik::isUserHasSomeWriteAccess() && !Common::isPhpCliMode()) {
Copy link
Member

Choose a reason for hiding this comment

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

would it be a good idea to move ($module !== 'API' || ($action && $action !== 'index')) and && !Common::isPhpCliMode() to another method? something like isTokenAuthLimitedToWriteAccess()

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 changed it. will merge if tests pass.

Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

left one comment, otherwise looks good to me

@tsteur tsteur merged commit b090937 into 4.x-dev Aug 10, 2020
@tsteur tsteur deleted the widgetviewaccess branch August 10, 2020 22:39
@mattab mattab added the c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. label Sep 29, 2020
@attritionorg
Copy link

@tsteur Can you summarize what security impact this has, or what privileges can be gained by exploiting this?

@tsteur
Copy link
Member Author

tsteur commented Oct 1, 2020

@attritionorg I assume you refer to when someone uses the token of a write, admin or super user? If the widgetized URL is shared with other users and that URL includes the token, then people who that token is shared with would gain the same privileges over the API. As a best practice we're now enforcing to use only tokens for view users. So if someone was to embed the widget into an internal wiki or so for all employees then they only have "view" access. Not sure if this roughly answers the question?

@attritionorg
Copy link

I think it does, thank you @tsteur. The initial language in this PR made it sound like this could be an exploitable security issue so was looking for clarity.

@mattab
Copy link
Member

mattab commented Nov 4, 2020

fyi added mention in the guide: https://matomo.org/docs/embed-matomo-reports/

Note: for security reasons, embedding the reports will only work when you use a token of the "View" permission (if you use a "Write" or "Admin" permission token an error message will be displayed instead.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants