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

Logging Errors in notifications can leak the super user token #7301

Closed
mattab opened this issue Feb 26, 2015 · 13 comments
Closed

Logging Errors in notifications can leak the super user token #7301

mattab opened this issue Feb 26, 2015 · 13 comments
Assignees
@mnapoli
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone
2.11.2

Comments

@mattab
Copy link
Member

mattab commented Feb 26, 2015

The goal of this issue is to fix a security issue where the Super User token_auth can be leaked to non logged in users.

For example in my case I was not even logged in (I was anonymous user) and still, I was shown the Super User token auth!

The error that was logged while I was anonymous was: ERROR: Got invalid response from API request: http://localhost/piwik-master/index.php?module=API&method=CoreAdminHome.runScheduledTasks&format=csv&convertToUnicode=0&token_auth=9b1cefc915ff6180071fb7dcd13ec5a4&trigger=archivephp. Response was 'sendmail: fatal: open /etc/postfix/main.cf: No such file or directory task,output Piwik\Plugins\CoreAdminHome\Tasks.purgeOutdatedArchives,Time elapsed: 2.807s Piwik\Plugins\ExamplePlugin\Tasks.myTaskWithParam_anystring,Time elapsed: 0.000s Piwik\Plugins\ExamplePlugin\Tasks.myTask,Time elapsed: 0.000s Piwik\Plugins\ScheduledReports\API.sendReport_1,ERROR: An error occured while sending 'HTML Email Report - 1.2015-02-19.1.en.html' to test@test.com. Error was 'Unable to send mail. ' Piwik\Plugins\CoreAdminHome\Tasks.purgeInvalidatedArchives,Time elapsed: 0.001s Piwik\Plugins\PrivacyManager\Tasks.deleteReportData,Time elapsed: 0.002s Piwik\Plugins\PrivacyManager\Tasks.deleteLogData,Time elapsed: 0.002s Piwik\Plugins\CorePluginsAdmin\Tasks.clearAllCacheEntries,Time elapsed: 0.001s Piwik\Plugins\CorePluginsAdmin\Tasks.sendNotificationIfUpdatesAvailable,Time elapsed: 0.001s Piwik\Plugins\CoreAdminHome\Tasks.optimizeArchiveTable,Time elapsed: 0.131s Piwik\Plugins\UserCountry\GeoIPAutoUpdater.update,Time elapsed: 0.050s Piwik\Plugins\CoreUpdater\Tasks.sendNotificationIfUpdateAvailable,Time elapsed: 0.001s'

As you can see it leaks the token_auth.

This occurs because one of the scheduled task failed with an ERROR, here the error being sendmail: fatal: open /etc/postfix/main.cf: No such file or directory
@mattab mattab added Bug For errors / faults / flaws / inconsistencies etc. Critical Indicates the severity of an issue is very critical and the issue has a very high priority. c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. labels Feb 26, 2015
@mattab
Copy link
Member Author

mattab commented Feb 26, 2015

  • could it because I have this in my config? log_writers[] = "screen"
  • I'm wondering whether this error would have been logged without this?
  • this ERROR was triggered in Tracking API in piwik.php
  • I think this would only affect users that have not disabled browser trigger archiving

@mattab
Copy link
Member Author

mattab commented Feb 26, 2015

OK maybe it isn't a critical security bug as maybe this could only occur when [Tracker] debug=1 in the config? (which was set for me when I saw this error in the notification)

@mattab mattab added Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. and removed Critical Indicates the severity of an issue is very critical and the issue has a very high priority. labels Feb 26, 2015
@mattab
Copy link
Member Author

mattab commented Feb 26, 2015

one solution could be to remove XYZ whenever a notification wants to display token_auth=XYZ

@mnapoli
Copy link
Contributor

mnapoli commented Feb 26, 2015

log_writers[] = "screen" is enabled by default.

If we want to globally fix this, I'd say that nothing that gets executed as super user should log to the current user (which could be anonymous). So logging for stuff executed as SU should not go to screen (because else it could happen somewhere else, not simply logging the token auth but logging anything related to SU or system).

But is there a way to know whether we are currently in super user mode?

@mnapoli
Copy link
Contributor

mnapoli commented Feb 26, 2015

More generally I think there should be a clear isolation between the current user request and "do as super user" stuff. Mixups and info leaks could happen with the logs but also maybe with anything that stores stuff in the session, maybe the cookies, maybe user settings, maybe stuff like activity log, etc, do you see anything else? In theory nothing user-related should be shared between those 2 contexts, just like if the super user stuff was running in a separate PHP process.

E.g. imagine the activity log logs an action that was done by "doAsSuperUser" while running in an anonymous user's request. Not a security issue, but it's an example of a mixup between those 2 "contexts".

@mnapoli
Copy link
Contributor

mnapoli commented Feb 26, 2015

Back to the issue here, the token is logged to screen but also to file. If that is not OK to log the token to file, maybe we should try to avoid logging the token at all in the first place?

@mattab
Copy link
Member Author

mattab commented Feb 26, 2015

maybe we should try to avoid logging the token at all in the first place?

+1
since we can't enforce developers / humans to not log the token (also it may appear in backtrace or other places) then I can only think of a solution of replacing any hex string of 32 chars with token_auth_removed or so?

@mgazdzik
Copy link
Contributor

Just a loose thought - wouldn't that kind of approach remove also useful things from logs ?
For ex. done flags for segments (might be useful while debuging archiving), or any other thing that also is using md5 hashing ?

@mnapoli
Copy link
Contributor

mnapoli commented Feb 26, 2015

Replacing token_auth= 9b1cefc915ff6180071fb7dcd13ec5a4 with token_auth=removed then? Would it cover all cases?

@mgazdzik
Copy link
Contributor

Yes, I think. Also just got another thought - it could be configurable param in case we would like to actually see what token auth is being used (maybe for debuging scheduled reports - we could check if wrong token is reason for task failing or so). Probably there would be more cases when we would like to verify if proper token is being used for given API call

@mattab
Copy link
Member Author

mattab commented Feb 27, 2015

Replacing token_auth= 9b1cefc915ff6180071fb7dcd13ec5a4 with token_auth=removed then? Would it cover all cases?

I think so, +1

@mattab mattab added this to the Piwik 2.11.2 milestone Feb 27, 2015
@mattab mattab mentioned this issue Feb 27, 2015
Closed
@mnapoli mnapoli self-assigned this Mar 1, 2015
mnapoli added a commit that referenced this issue Mar 1, 2015
@mnapoli
#7301 Removes any token_auth that might be logged.
10bc3c8
@mnapoli
Copy link
Contributor

mnapoli commented Mar 1, 2015

I have pushed a commit that does the replacement. Should we close this?

@mattab
Copy link
Member Author

mattab commented Mar 1, 2015

Looks good to me

@mnapoli mnapoli closed this as completed Mar 1, 2015
@mattab mattab mentioned this issue Mar 4, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mnapoli mnapoli

Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Milestone
2.11.2
Development

No branches or pull requests
3 participants
@mattab @mnapoli @mgazdzik