Prevent passwords being set with empty strings #32261

settermjd · 2018-08-07T08:37:13Z

Description

Currently, if an attempt is made to set a password with an, effectively, empty string it will be accepted
without question. This PR seeks to prevent that from happening, by ensuring that passwords must be set to a meaningful/valid value.

Related Issue

Fixes https://github.com/owncloud/security-tracker/issues/282.

Motivation and Context

It prevents a user's password from being set to an empty string.

How Has This Been Tested?

Unit tests have been added to cover the changes.

Screenshots (if appropriate):

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)
Technical debt
Tests

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

phil-davis · 2018-08-07T09:16:21Z

lib/private/User/Database.php

 	 */
 	public function setPassword($uid, $password) {
+		if (empty($password)) {


This will prevent someone setting their password to the string char "0".
I guess that is not such a bad restriction on valid password strings.

True. But on reading after coming to understand the code better and reading through https://github.com/owncloud/security-tracker/issues/282 in light of that information, it appears to me that this PR isn't required, as the code works as expected.

Turns out, this PR's still in the game after further discussion with @PVince81.

please avoid using empty() in any case due to its nasty implicit nature.
if you intend to check for "0" then add an explicit check for it

Feedback addressed @PVince81.

codecov · 2018-08-07T10:37:23Z

Codecov Report

Merging #32261 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #32261      +/-   ##
============================================
+ Coverage     64.09%    64.1%   +<.01%     
- Complexity    18661    18670       +9     
============================================
  Files          1177     1177              
  Lines         70247    70255       +8     
  Branches       1270     1270              
============================================
+ Hits          45026    45035       +9     
+ Misses        24851    24850       -1     
  Partials        370      370

Flag	Coverage Δ	Complexity Δ
#javascript	`52.89% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`65.37% <100%> (ø)`	`18670 <2> (+9)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/Database.php	`72.5% <100%> (+0.46%)`	`44 <0> (+1)`	⬆️
lib/public/Util.php	`57.75% <100%> (+0.68%)`	`79 <2> (+2)`	⬆️
lib/private/User/User.php	`88.88% <100%> (+0.75%)`	`74 <0> (+6)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 364af70...1424934. Read the comment docs.

PVince81 · 2018-08-08T13:48:03Z

lib/private/Traits/EmptyStringTrait.php

+
+namespace OC\Traits;
+
+trait EmptyStringTrait {


@DeepDiver1975 thoughts on this ? does PHP 7.2 bring something ?

@DeepDiver1975?

@settermjd can you remove this trait ? this feels a bit overengineered

maybe put this as a local method on the class then. I'd rather move this forward to get it merged than spend a long time trying to decide where this method needs to go

I'm slightly reluctant as it'd have to be copied into both Database and User, as they don't share a common base class and the functionality's not logically specific to either. However, if that's the call, just say and I'll make it happen.

should we then rather move it to \OCP\Util then ? then any app can use it.
a Trait feel a bit like overkill

@settermjd please move this to \OCP\Util

Odd, I thought I'd already done this.

PVince81 · 2018-08-08T13:48:41Z

lib/private/User/Database.php

 	 */
 	public function setPassword($uid, $password) {
+		if ($this->isEmpty($password)) {
+			throw new ArgumentNotSetException('Password cannot be empty');


we usually use \InvalidArgumentException

PVince81

sounds like a bit much adding all these new traits and classes just for this simple thing

PVince81 · 2018-08-08T13:50:14Z

tests/lib/Traits/PasswordTrait.php

+	/**
+	 * @return array A list of items equivalent to an empty password value
+	 */
+	public function providesEmptyPasswordData() {


feels like overkill to have a trait just for this

maybe we could move the test set to the TestCase base class and put common data providers there, but then it would be providesEmptyValues() and not specific to passwords

Sounds fine to me. The main motivation was not to have duplicated code. But if having a similar method in TestCase achieves the same objective, them I'm 💯% behind it!

settermjd · 2018-08-23T06:59:26Z

@PVince81, is it ok to go now?

PVince81

please move the utility function to \OCP\Util

settermjd · 2018-09-04T12:06:35Z

@PVince81, the function's been moved. Thanks for the push.

PVince81 · 2018-09-04T14:28:12Z

please fix the styles, see https://drone.owncloud.com/owncloud/core/10097/69 or run make test-php-style

PVince81 · 2018-09-04T19:30:58Z

@ownclouders rebase

ownclouders · 2018-09-04T19:31:00Z

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

ownclouders · 2018-09-04T19:32:05Z

Automated rebase with GitMate.io was successful! 🎉

PVince81 · 2018-09-04T20:58:50Z

squashed. I hope this will also nudge Drone

PVince81 · 2018-09-04T21:00:18Z

stable10: #32581

hopeful that there were no more testing issues then...

phil-davis · 2018-09-05T02:31:04Z

Some drone problem in the install-server step of all the sub-jobs. I restarted drone just now.

settermjd · 2018-09-05T06:25:14Z

@PVince81, do you still want #32261 (comment) done?

PVince81 · 2018-09-05T07:06:16Z

@settermjd I already fixed those myself

PVince81 · 2018-09-05T07:06:46Z

restarted the builds

PVince81 · 2018-09-05T13:12:32Z

seems rebasing did not help:

federatedfilesharing enabled
+ php occ a:l
Enabled:
  - comments: 0.3.0
  - dav: 0.3.2
  - federatedfilesharing: 0.3.1
  - federation: 0.1.0
  - files: 1.5.2
  - files_external: 0.7.1
  - files_sharing: 0.11.0
  - files_trashbin: 0.9.1
  - files_versions: 1.3.0
  - provisioning_api: 0.5.0
  - systemtags: 0.3.0
  - updatenotification: 0.2.1
+ php occ a:e testing
testing not found

maybe resend the PR separately ?

settermjd · 2018-09-05T13:28:06Z

@PVince81, I don't understand what you're asking in #32261 (comment).

Previously, if an attempt was made to set a password with an, effectively, empty string the setPassword function would accept it without question. This commit prevents that from happening by throwing an exception if an empty password string is supplied. What this commit doesn't do is check any of the knock-on of upstream effects of this change. One thing to bear in mind; No test in the current test suite appears to fail as a result. That doesn't mean that everything's ok though. So further work needs to be done to ensure that no bugs appear as a result of this change.

phil-davis · 2018-09-05T14:09:17Z

@settermjd I rebased it locally and force pushed here. CI is running again, let's see.

phil-davis · 2018-09-05T14:36:39Z

CI passing 🎆

settermjd added Type:Bug enhancement security p3-medium Normal priority WIP labels Aug 7, 2018

settermjd added this to the development milestone Aug 7, 2018

settermjd self-assigned this Aug 7, 2018

ownclouders added the Hardening label Aug 7, 2018

phil-davis reviewed Aug 7, 2018

View reviewed changes

settermjd force-pushed the stop-setting-empty-passwords branch from b36c1f7 to 14ad9cf Compare August 7, 2018 10:52

PVince81 reviewed Aug 8, 2018

View reviewed changes

PVince81 suggested changes Aug 8, 2018

View reviewed changes

PVince81 suggested changes Aug 31, 2018

View reviewed changes

ownclouders force-pushed the stop-setting-empty-passwords branch from d395407 to e3e358d Compare September 4, 2018 19:31

PVince81 added 4 - To release and removed Hardening labels Sep 4, 2018

PVince81 force-pushed the stop-setting-empty-passwords branch from e3e358d to 8a04a79 Compare September 4, 2018 20:58

PVince81 mentioned this pull request Sep 4, 2018

[stable10] Throw an exception when attempting to set an empty password #32581

Merged

phil-davis force-pushed the stop-setting-empty-passwords branch from 8a04a79 to 1424934 Compare September 5, 2018 14:08

PVince81 merged commit fd56631 into master Sep 5, 2018

PVince81 deleted the stop-setting-empty-passwords branch September 5, 2018 15:40

This was referenced Sep 7, 2018

[10.0.10RC1] password cannot be "0" on the "New User Registration Page" #32626

Closed

User password cannot be "0" owncloud-archive/documentation#4413

Closed

lock bot locked as resolved and limited conversation to collaborators Sep 5, 2019

Prevent passwords being set with empty strings #32261

Prevent passwords being set with empty strings #32261

Conversation

settermjd commented Aug 7, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

settermjd Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

settermjd commented Aug 23, 2018

PVince81 left a comment

Choose a reason for hiding this comment

settermjd commented Sep 4, 2018

PVince81 commented Sep 4, 2018 • edited by phil-davis Loading

PVince81 commented Sep 4, 2018

ownclouders commented Sep 4, 2018

ownclouders commented Sep 4, 2018

PVince81 commented Sep 4, 2018

PVince81 commented Sep 4, 2018

phil-davis commented Sep 5, 2018

settermjd commented Sep 5, 2018

PVince81 commented Sep 5, 2018

PVince81 commented Sep 5, 2018

PVince81 commented Sep 5, 2018

settermjd commented Sep 5, 2018 • edited Loading

phil-davis commented Sep 5, 2018

phil-davis commented Sep 5, 2018

codecov bot commented Aug 7, 2018 •

edited

Loading

PVince81 Aug 21, 2018 •

edited

Loading

settermjd Aug 21, 2018 •

edited

Loading

PVince81 commented Sep 4, 2018 •

edited by phil-davis

Loading

settermjd commented Sep 5, 2018 •

edited

Loading