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

Using auth tokens of "write" or "admin" permissions when embedding #16554

Closed
mattab opened this issue Oct 10, 2020 · 6 comments · Fixed by #16595
Closed

Using auth tokens of "write" or "admin" permissions when embedding #16554

mattab opened this issue Oct 10, 2020 · 6 comments · Fixed by #16595
Assignees
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Oct 10, 2020

the new security feature in #16263 would break use cases for users who need to be able to create/update entities like goals and custom dimensions from within an iframe. This is the case when users embed the whole Matomo App within their products. as quickly described in this faq). Currently they would get the message This user has at least some write access. Only tokens of users who have only view access can be used..

Initially was thinking we could have yet another INI setting, but actually it looks better/easier to accept write/admin tokens whenever enable_framed_pages=1 or enable_framed_settings=1 are set in the config?

(need to be added to RC as this would otherwise be a BC break for some)

@mattab mattab added this to the 4.0.0-RC milestone Oct 10, 2020
@mattab
Copy link
Member Author

mattab commented Oct 10, 2020

At same time, could we change the message from This user has at least some write access. Only tokens of users who have only view access can be used. to This user has write or admin access to the website. When embedding reports, for security reasons only view access (read-only) auth tokens can be used. (or better! maybe could use the "permission" wording used eg. https://matomo.org/faq/general/faq_26910/)

@tsteur
Copy link
Member

tsteur commented Oct 11, 2020

Initially was thinking we could have yet another INI setting, but actually it looks better/easier to accept write/admin tokens whenever enable_framed_pages=1 or enable_framed_settings=1 are set in the config?

We won't do this because those things aren't related. Also embedding manage goals or so is not officially supported by our widgets that can be embedded.

It's likely a use case we don't want to support. There could be some setting if REALLY needed like [General]allow_risky_widget_authentication_using_higher_privileges=1 or something but I prefer not to start supporting this suddenly. (just an example the name, not sure what to use best)

It wouldn't even be a BC break as we never supported this AFAIK. Maybe what should be used in this case is rather the logme feature or so if the entire UI is embedded.

@mattab
Copy link
Member Author

mattab commented Oct 12, 2020

We did support this use case and several people rely on it. That's why we have the feature enable_framed_pages=1. it lets people embed the whole Matomo app without any restrictions that may exist in Widgetize. A new INI setting sounds good too, as long as there's some option for people who need it.

@tsteur
Copy link
Member

tsteur commented Oct 12, 2020

If we continue to support this (which I would maybe not recommend) then the risks will need to mentioned clearly eg that the token will appear in logs and everything. It's not really secure in various ways (token appearing in logs, not clear who makes changes / anyone being able to misuse things in many ways, etc) plus it can create a lot of work for us to support this as it means we cannot always rely on a session etc.

Also we need to mention on HackerOne that we exclude anything related this config flag (and enable_framed_pages etc) from the security bounty program

@mattab
Copy link
Member Author

mattab commented Oct 13, 2020

it would be ideal if we could keep this feature as it helps people embed Matomo app within their own backoffice (for example in combination with a custom theme, we've seen some really great looking integrations before). Also some people want to embed Matomo App within iframe, including the login form (the token_auth support is not needed in this case, but they need the iframe - they may also use the logme technique).

So we need to be way more transparent of all the disadvantages (which we weren't quite aware of before) and sounds good to mention clearly the security risks:

  • In the INI global config file - todo
  • in the FAQ at https://matomo.org/faq/troubleshooting/faq_147/ - just now edited it a bit to clarify. anything to add?
  • Also updated https://matomo.org/faq/how-to/faq_92/ to redirect to the other faq
  • mention on HackerOne that we exclude anything related this config flag (and enable_framed_pages etc) - todo
  • btw we should still block "Super Users" tokens from being used even when enable_framed_pages is set (similarly to how logme doesn't allow super users)

@tsteur
Copy link
Member

tsteur commented Oct 13, 2020

@mattab there may be many more security issues. Eg around sessions etc. It's basically quite unpredictable. I wouldn't even be surprised if a user stayed logged in even if the user itself was removed etc.

@tsteur tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Oct 19, 2020
@diosmosis diosmosis self-assigned this Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants