fix user enumeration with logging #27011

Merged
merged 4 commits into from Jan 25, 2017

Projects

None yet

4 participants

@Peter-Prochaska
Contributor

Description

Fixed user enumeration

Related Issue

#23595

Motivation and Context

instead of exceptions, we are logging

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@Peter-Prochaska Peter-Prochaska fix user enumeration with logging
d2f47ac
@CLAassistant
CLAassistant commented Jan 24, 2017 edited

CLA assistant check
All committers have signed the CLA.

@PVince81

Code changes look good, I do miss the user name in the log.

- ISecureRandom::CHAR_LOWER.
- ISecureRandom::CHAR_UPPER);
- $this->config->setUserValue($user, 'owncloud', 'lostpassword', $this->timeFactory->getTime() .':'. $token);
+ if (!empty($email)) {
@PVince81
PVince81 Jan 24, 2017 Collaborator

Note that in PHP empty("0") is true. Usually that won't happen with email addresses.

You might want to change it but I'm fine if you don't as it's part of the old code.

@jvillafanez
jvillafanez Jan 24, 2017 Contributor

The getEMailAddress() function should have documented what is the expected result if there is no email set (maybe null?). I'd make the condition to match that expectation.

- $link = $this->urlGenerator->linkToRouteAbsolute('core.lost.resetform', ['userId' => $user, 'token' => $token]);
+ $link = $this->urlGenerator->linkToRouteAbsolute('core.lost.resetform', ['userId' => $user, 'token' => $token]);
@PVince81
PVince81 Jan 24, 2017 Collaborator

weird indent, please use tabs

core/Controller/LostController.php
+ ));
+ }
+ } else {
+ $this->logger->error('Could not send reset email because there is no email address for this username.', ['app' => 'core']);
@PVince81
PVince81 Jan 24, 2017 Collaborator

please add the user in the log message, could be useful for fail2ban or so

core/Controller/LostController.php
+ $this->logger->error('Could not send reset email because there is no email address for this username.', ['app' => 'core']);
+ }
+ } else {
+ $this->logger->error('Couldn\'t send reset email because User does not exist.', ['app' => 'core']);
@PVince81
PVince81 Jan 24, 2017 Collaborator

same here, add the user name in the message

@PVince81
Collaborator

@Peter-Prochaska please also adjust the related unit tests, they are failing.

Peter-Prochaska added some commits Jan 24, 2017
@Peter-Prochaska Peter-Prochaska fixed unittests
151a29b
@Peter-Prochaska Peter-Prochaska fixed unittests
d3407c3
- $msg = $tmpl->fetchPage();
+ $tmpl = new \OC_Template('core', 'lostpassword/email');
+ $tmpl->assign('link', $link);
+ $msg = $tmpl->fetchPage();
@PVince81
PVince81 Jan 24, 2017 Collaborator

please use tabs, not spaces

@PVince81
Collaborator

@Peter-Prochaska thanks for fixing the unit tests. Can you also address the review comments ?

@Peter-Prochaska Peter-Prochaska space to tabs and added username in error message
c303974
@PVince81
Collaborator

Retested, works 👍

Let's merge after Jenkins passes then move on to the backports.

@PVince81
Collaborator

Travis is unlikely to fail, but due to tech issues on their side it might take a while to catch up.

Since Jenkins passed it is very likely that it will all be green.

@Peter-Prochaska please prepare the backport PRs to all relevant branches.

@PVince81 PVince81 merged commit f15c3c2 into master Jan 25, 2017

4 checks passed

Scrutinizer No new issues
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the fix_owncloud_user_enumeration_vulnerbility_#23595 branch Jan 25, 2017
@PVince81
Collaborator

Merged.

@Peter-Prochaska please backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment