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.

Sign up for GitHub

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

Replacing PHPass with password_compat #10219

Closed
wants to merge 1 commit into from
Closed

Replacing PHPass with password_compat #10219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions apps/files_sharing/lib/connector/publicauth.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,7 @@ protected function validateUserPass($username, $password) {
if (isset($linkItem['share_with'])) {
if ($linkItem['share_type'] == \OCP\Share::SHARE_TYPE_LINK) {
// Check Password
$forcePortable = (CRYPT_BLOWFISH != 1);
$hasher = new \PasswordHash(8, $forcePortable);
if (!$hasher->CheckPassword($password . $this->config->getSystemValue('passwordsalt', ''), $linkItem['share_with'])) {
return false;
} else {
return true;
}
return password_verify($password, $linkItem['share_with']);
Copy link
Member

Choose a reason for hiding this comment

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

How will this ever work? All hashes need to be migrated - as a result we need to ship password_compat AND phpass with the next major version and offer a proper migration pass for all apps as well.

} else {
return false;
}
Expand Down
6 changes: 1 addition & 5 deletions apps/files_sharing/lib/helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
namespace OCA\Files_Sharing;

use OC_Config;
use PasswordHash;

class Helper {

Expand Down Expand Up @@ -99,10 +98,7 @@ public static function authenticate($linkItem, $password) {
if ($password !== null) {
if ($linkItem['share_type'] == \OCP\Share::SHARE_TYPE_LINK) {
// Check Password
$forcePortable = (CRYPT_BLOWFISH != 1);
$hasher = new PasswordHash(8, $forcePortable);
if (!($hasher->CheckPassword($password.OC_Config::getValue('passwordsalt', ''),
$linkItem['share_with']))) {
if (!(password_verify($password, $linkItem['share_with']))) {
return false;
} else {
// Save item id in session for future requests
Copy link
Member

Choose a reason for hiding this comment

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

This seems also to break old passwords.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, my generated passes in the test where incorrect i will correct this.

Expand Down
48 changes: 18 additions & 30 deletions apps/files_sharing/public.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
<?php
// Load other apps for file previews
use OCA\Files_Sharing\Helper;

OC_App::loadApps();

$appConfig = \OC::$server->getAppConfig();
Expand All @@ -13,12 +11,8 @@
exit();
}

// Legacy sharing links via public.php have the token in $GET['t']
if (isset($_GET['t'])) {
$token = $_GET['t'];
}

if (isset($token)) {
$linkItem = OCP\Share::getShareByToken($token, false);
if (is_array($linkItem) && isset($linkItem['uid_owner'])) {
// seems to be a valid share
Expand Down Expand Up @@ -57,10 +51,7 @@
$password = $_POST['password'];
if ($linkItem['share_type'] == OCP\Share::SHARE_TYPE_LINK) {
// Check Password
$forcePortable = (CRYPT_BLOWFISH != 1);
$hasher = new PasswordHash(8, $forcePortable);
if (!($hasher->CheckPassword($password.OC_Config::getValue('passwordsalt', ''),
$linkItem['share_with']))) {
if (!password_verify($password, $linkItem['share_with'])) {
OCP\Util::addStyle('files_sharing', 'authenticate');
$tmpl = new OCP\Template('files_sharing', 'authenticate', 'guest');
$tmpl->assign('URL', $url);
Expand All @@ -69,11 +60,11 @@
exit();
} else {
// Save item id in session for future requests
\OC::$server->getSession()->set('public_link_authenticated', $linkItem['id']);
\OC::$session->set('public_link_authenticated', $linkItem['id']);
Copy link
Member

Choose a reason for hiding this comment

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

Why?

}
} else {
OCP\Util::writeLog('share', 'Unknown share type '.$linkItem['share_type']
.' for share id '.$linkItem['id'], \OCP\Util::ERROR);
OCP\Util::writeLog('share', 'Unknown share type ' . $linkItem['share_type']
. ' for share id ' . $linkItem['id'], \OCP\Util::ERROR);
header('HTTP/1.0 404 Not Found');
$tmpl = new OCP\Template('', '404', 'guest');
$tmpl->printPage();
Expand All @@ -82,8 +73,8 @@

} else {
// Check if item id is set in session
if ( ! \OC::$server->getSession()->exists('public_link_authenticated')
|| \OC::$server->getSession()->get('public_link_authenticated') !== $linkItem['id']
if (!\OC::$session->exists('public_link_authenticated')
|| \OC::$session->get('public_link_authenticated') !== $linkItem['id']
) {
// Prompt for password
OCP\Util::addStyle('files_sharing', 'authenticate');
Expand All @@ -95,7 +86,7 @@
}
}
$basePath = $path;
$rootName = \OC_Util::basename($path);
$rootName = basename($path);
Copy link
Member

Choose a reason for hiding this comment

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

Why?

if (isset($_GET['path']) && \OC\Files\Filesystem::isReadable($basePath . $_GET['path'])) {
$getPath = \OC\Files\Filesystem::normalizePath($_GET['path']);
$path .= $getPath;
Expand All @@ -106,15 +97,12 @@
$file = basename($path);
// Download the file
if (isset($_GET['download'])) {
if (!\OCP\App::isEnabled('files_encryption')) {
// encryption app requires the session to store the keys in
\OC::$server->getSession()->close();
}
\OC::$server->getSession()->close();
Copy link
Member

Choose a reason for hiding this comment

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

Why?

if (isset($_GET['files'])) { // download selected files
$files = urldecode($_GET['files']);
$files_list = json_decode($files);
// in case we get only a single file
if ($files_list === NULL ) {
if ($files_list === null) {
$files_list = array($files);
}
OC_Files::get($path, $files_list, $_SERVER['REQUEST_METHOD'] == 'HEAD');
Expand All @@ -123,27 +111,27 @@
}
exit();
} else {

OCP\Util::addScript('files', 'file-upload');
OCP\Util::addStyle('files_sharing', 'public');
OCP\Util::addStyle('files_sharing', 'mobile');
OCP\Util::addScript('files_sharing', 'public');
OCP\Util::addScript('files', 'fileactions');
OCP\Util::addScript('files', 'jquery.iframe-transport');
OCP\Util::addScript('files', 'jquery.fileupload');
$maxUploadFilesize=OCP\Util::maxUploadFilesize($path);
$maxUploadFilesize = OCP\Util::maxUploadFilesize($path);
$tmpl = new OCP\Template('files_sharing', 'public', 'base');
$tmpl->assign('displayName', \OCP\User::getDisplayName($shareOwner));
$tmpl->assign('filename', $file);
$tmpl->assign('directory_path', $linkItem['file_target']);
$tmpl->assign('mimetype', \OC\Files\Filesystem::getMimeType($path));
$tmpl->assign('dirToken', $linkItem['token']);
$tmpl->assign('sharingToken', $token);
$tmpl->assign('server2serversharing', Helper::isOutgoingServer2serverShareEnabled());
$tmpl->assign('protected', isset($linkItem['share_with']) ? 'true' : 'false');

$urlLinkIdentifiers= (isset($token)?'&t='.$token:'')
.(isset($_GET['dir'])?'&dir='.$_GET['dir']:'')
.(isset($_GET['file'])?'&file='.$_GET['file']:'');
$urlLinkIdentifiers = (isset($token) ? '&t=' . $token : '')
. (isset($_GET['dir']) ? '&dir=' . $_GET['dir'] : '')
. (isset($_GET['file']) ? '&file=' . $_GET['file'] : '');
// Show file list
if (\OC\Files\Filesystem::is_dir($path)) {
$tmpl->assign('dir', $getPath);
Expand All @@ -157,10 +145,10 @@
OCP\Util::addscript('files', 'keyboardshortcuts');
$files = array();
$rootLength = strlen($basePath) + 1;
$maxUploadFilesize=OCP\Util::maxUploadFilesize($path);
$maxUploadFilesize = OCP\Util::maxUploadFilesize($path);

$freeSpace=OCP\Util::freeSpace($path);
$uploadLimit=OCP\Util::uploadLimit();
$freeSpace = OCP\Util::freeSpace($path);
$uploadLimit = OCP\Util::uploadLimit();
$folder = new OCP\Template('files', 'list', '');
$folder->assign('dir', $getPath);
$folder->assign('dirToken', $linkItem['token']);
Expand All @@ -185,7 +173,7 @@
$tmpl->assign('downloadURL', OCP\Util::linkToPublic('files') . $urlLinkIdentifiers . '&download');
} else {
$tmpl->assign('downloadURL', OCP\Util::linkToPublic('files')
.$urlLinkIdentifiers.'&download&path='.urlencode($getPath));
. $urlLinkIdentifiers . '&download&path=' . urlencode($getPath));
}
}
$tmpl->printPage();
Expand Down
3 changes: 3 additions & 0 deletions config/config.sample.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
/* Prefix for the ownCloud tables in the database */
"dbtableprefix" => "",

/* Define the cost used to hash the user passwords. */
"hashingCost" => "",
Copy link

Choose a reason for hiding this comment

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

This patch does not completely remove the use of passwordsalt. E.g.
./apps/files_external/lib/config.php: $cipher->setKey(\OCP\Config::getSystemValue('passwordsalt'));
matches.

This is likely a consequence of #7794 (comment)

Copy link

Choose a reason for hiding this comment

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

Furthermore, it is required for legacy passwords anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. We need to:

  1. Deprecate the passwordsalt officially for ownCloud 8 and then remove it for ownCloud 9
  2. Migrate all places that used the password salt for encrypting stuff to the new security utils


/* Define the salt used to hash the user passwords. All your user passwords are lost if you lose this string. */
"passwordsalt" => "",

Expand Down
4 changes: 1 addition & 3 deletions lib/private/share/share.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,9 +612,7 @@ public static function shareItem($itemType, $itemSource, $shareType, $shareWith,

// Generate hash of password - same method as user passwords
if (!empty($shareWith)) {
$forcePortable = (CRYPT_BLOWFISH != 1);
$hasher = new \PasswordHash(8, $forcePortable);
$shareWith = $hasher->HashPassword($shareWith.\OC_Config::getValue('passwordsalt', ''));
$shareWith = password_hash($shareWith, PASSWORD_BCRYPT, array('cost' => OC::$server->getConfig()->getSystemValue('hashingCost')));
} else {
// reuse the already set password, but only if we change permissions
// otherwise the user disabled the password protection
Expand Down
53 changes: 27 additions & 26 deletions lib/private/user/database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,30 +33,16 @@
*
*/

require_once 'phpass/PasswordHash.php';

/**
* Class for user management in a SQL Database (e.g. MySQL, SQLite)
*/
class OC_User_Database extends OC_User_Backend {
/**
* @var PasswordHash
*/
private static $hasher = null;

private $cache = array();

private function getHasher() {
if (!self::$hasher) {
//we don't want to use DES based crypt(), since it doesn't return a hash with a recognisable prefix
$forcePortable = (CRYPT_BLOWFISH != 1);
self::$hasher = new PasswordHash(8, $forcePortable);
}
return self::$hasher;
}

/**
* Create a new user
*
* @param string $uid The username of the user to create
* @param string $password The password of the new user
* @return bool
Expand All @@ -66,8 +52,7 @@ private function getHasher() {
*/
public function createUser($uid, $password) {
if (!$this->userExists($uid)) {
$hasher = $this->getHasher();
$hash = $hasher->HashPassword($password . OC_Config::getValue('passwordsalt', ''));
$hash = password_hash($password, PASSWORD_BCRYPT, array('cost' => OC::$server->getConfig()->getSystemValue('hashingCost')));
$query = OC_DB::prepare('INSERT INTO `*PREFIX*users` ( `uid`, `password` ) VALUES( ?, ? )');
$result = $query->execute(array($uid, $hash));

Expand All @@ -79,6 +64,7 @@ public function createUser($uid, $password) {

/**
* delete a user
*
* @param string $uid The username of the user to delete
* @return bool
*
Expand All @@ -98,6 +84,7 @@ public function deleteUser($uid) {

/**
* Set password
*
* @param string $uid The username
* @param string $password The new password
* @return bool
Expand All @@ -106,8 +93,7 @@ public function deleteUser($uid) {
*/
public function setPassword($uid, $password) {
if ($this->userExists($uid)) {
$hasher = $this->getHasher();
$hash = $hasher->HashPassword($password . OC_Config::getValue('passwordsalt', ''));
$hash = password_hash($password, PASSWORD_BCRYPT, array('cost' => OC::$server->getConfig()->getSystemValue('hashingCost')));
$query = OC_DB::prepare('UPDATE `*PREFIX*users` SET `password` = ? WHERE `uid` = ?');
$result = $query->execute(array($hash, $uid));

Expand All @@ -119,6 +105,7 @@ public function setPassword($uid, $password) {

/**
* Set display name
*
* @param string $uid The username
* @param string $displayName The new display name
* @return bool
Expand All @@ -139,6 +126,7 @@ public function setDisplayName($uid, $displayName) {

/**
* get display name of the user
*
* @param string $uid user ID of the user
* @return string display name
*/
Expand All @@ -149,6 +137,7 @@ public function getDisplayName($uid) {

/**
* Get a list of all display names
*
* @return array an array of all displayNames (value) and the correspondig uids (key)
*
* Get a list of all display names and user ids.
Expand All @@ -169,6 +158,7 @@ public function getDisplayNames($search = '', $limit = null, $offset = null) {

/**
* Check if the password is correct
*
* @param string $uid The username
* @param string $password The password
* @return string
Expand All @@ -183,14 +173,21 @@ public function checkPassword($uid, $password) {
$row = $result->fetchRow();
if ($row) {
$storedHash = $row['password'];
if ($storedHash[0] === '$') { //the new phpass based hashing
$hasher = $this->getHasher();
if ($hasher->CheckPassword($password . OC_Config::getValue('passwordsalt', ''), $storedHash)) {
return $row['uid'];
}

//old sha1 based hashing
} elseif (sha1($password) === $storedHash) {
if (password_verify($password, $storedHash)) {
// Check and see if our password needs to be rehashed
if (password_needs_rehash($storedHash, PASSWORD_BCRYPT)) {
$this->setPassword($row['uid'], $password);
}
return $row['uid'];
// Check to see if we are dealing with a legacy password (phpass)
} elseif (password_verify($password . OC::$server->getConfig()->getSystemValue('passwordsalt'), $storedHash)) {
// Check and see if our password needs to be rehashed
if (password_needs_rehash($storedHash, PASSWORD_BCRYPT)) {
$this->setPassword($row['uid'], $password);
Copy link

Choose a reason for hiding this comment

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

I think you will want to generate a new hash that does not contain the passwordsalt regardless of password_needs_rehash() just like in the case below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the password is using the old method it will always need rehashing

Copy link

Choose a reason for hiding this comment

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

if the password is using the old method it will always need rehashing

That is kind of exactly my point. You already know that it needs to be rehashed, thus calling password_needs_rehash($storedHash, PASSWORD_BCRYPT) is pointless (though I assumed it might also return false in some cases).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs to be there in the off case we ever change the hashing algorithm in the future. or if the user adjusts the hashing cost

Copy link

Choose a reason for hiding this comment

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

It needs to be there in the off case we ever change the hashing algorithm in the future. or if the user adjusts the hashing cost

That seems to contradict

if the password is using the old method it will always need rehashing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure, both are perfectly valid it does need to be there for the reasons stated above and it will also require legacy passwords to be rehashed...

}
Copy link

Choose a reason for hiding this comment

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

This should actually break existing hashes because the passwordsalt is no longer appended (which almost certainly was never the correct way of doing it anyway).

return $row['uid'];
} elseif (sha1($password) === $storedHash) { //old sha1 based hashing
//upgrade to new hashing
$this->setPassword($row['uid'], $password);
return $row['uid'];
Expand All @@ -202,6 +199,7 @@ public function checkPassword($uid, $password) {

/**
* Load an user in the cache
*
* @param string $uid the username
* @return boolean
*/
Expand All @@ -226,6 +224,7 @@ private function loadUser($uid) {

/**
* Get a list of all users
*
* @return array an array of all uids
*
* Get a list of all users.
Expand All @@ -242,6 +241,7 @@ public function getUsers($search = '', $limit = null, $offset = null) {

/**
* check if a user exists
*
* @param string $uid the username
* @return boolean
*/
Expand All @@ -252,6 +252,7 @@ public function userExists($uid) {

/**
* get the user's home directory
*
* @param string $uid the username
* @return string|false
*/
Expand Down
Loading