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

Store credentials only in session if required #11747

Merged
merged 2 commits into from
Dec 1, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 27 additions & 3 deletions apps/files_external/lib/smb_oc.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,16 @@
class SMB_OC extends \OC\Files\Storage\SMB {
private $username_as_share;

/**
* @param array $params
* @throws \Exception
*/
public function __construct($params) {
if (isset($params['host']) && \OC::$session->exists('smb-credentials')) {
$host=$params['host'];
$this->username_as_share = ($params['username_as_share'] === 'true');

$params_auth = \OC::$session->get('smb-credentials');
$params_auth = json_decode(\OC::$server->getCrypto()->decrypt(\OC::$session->get('smb-credentials')), true);
$user = \OC::$session->get('loginname');
$password = $params_auth['password'];

Expand All @@ -44,14 +48,34 @@ public function __construct($params) {
}
}

public static function login( $params ) {
\OC::$session->set('smb-credentials', $params);
/**
* Intercepts the user credentials on login and stores them
* encrypted inside the session if SMB_OC storage is enabled.
* @param array $params
*/
public static function login($params) {
$mountpoints = \OC_Mount_Config::getAbsoluteMountPoints($params['uid']);
$mountpointClasses = array();
foreach($mountpoints as $mountpoint) {
$mountpointClasses[$mountpoint['class']] = true;
}
if(isset($mountpointClasses['\OC\Files\Storage\SMB_OC'])) {
\OC::$session->set('smb-credentials', \OC::$server->getCrypto()->encrypt(json_encode($params)));
}
}

/**
* @param string $path
* @return boolean
*/
public function isSharable($path) {
return false;
}

/**
* @param bool $isPersonal
* @return bool
*/
public function test($isPersonal = true) {
if ($isPersonal) {
if ($this->stat('')) {
Expand Down
6 changes: 6 additions & 0 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -831,4 +831,10 @@
"style-src 'self' 'unsafe-inline'; frame-src *; img-src *; ".
"font-src 'self' data:; media-src *",

/**
* Secret used by ownCloud for various purposes, e.g. to encrypt data. If you
* lose this string there will be data corruption.
*/
'secret' => 'ICertainlyShouldHaveChangedTheDefaultSecret',
Copy link
Contributor

Choose a reason for hiding this comment

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

Way to go!

Copy link
Member Author

Choose a reason for hiding this comment

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

🎫


);
4 changes: 3 additions & 1 deletion lib/private/repair.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

use OC\Hooks\BasicEmitter;
use OC\Hooks\Emitter;
use OC\Repair\RepairConfig;

class Repair extends BasicEmitter {
/**
Expand Down Expand Up @@ -69,7 +70,8 @@ public function addStep($repairStep) {
*/
public static function getRepairSteps() {
return array(
new \OC\Repair\RepairMimeTypes()
new \OC\Repair\RepairMimeTypes(),
new RepairConfig(),
);
}

Expand Down
130 changes: 130 additions & 0 deletions lib/private/security/crypto.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
<?php
/**
* Copyright (c) 2014 Lukas Reschke <[email protected]>
* This file is licensed under the Affero General Public License version 3 or
* later.
* See the COPYING-README file.
*/


namespace OC\Security;

use Crypt_AES;
use Crypt_Hash;
use OCP\Security\ICrypto;
use OCP\Security\StringUtils;
use OCP\IConfig;

/**
* Class Crypto provides a high-level encryption layer using AES-CBC. If no key has been provided
* it will use the secret defined in config.php as key. Additionally the message will be HMAC'd.
*
* Usage:
* $encryptWithDefaultPassword = \OC::$server->getCrypto()->encrypt('EncryptedText');
* $encryptWithCustompassword = \OC::$server->getCrypto()->encrypt('EncryptedText', 'password');
*
* @package OC\Security
*/
class Crypto implements ICrypto {
/** @var Crypt_AES $cipher */
private $cipher;
/** @var int */
private $ivLength = 16;
/** @var IConfig */
private $config;

/**
* @param IConfig $config
*/
function __construct(IConfig $config) {
$this->cipher = new Crypt_AES();
$this->config = $config;
}

/**
* Custom implementation of hex2bin since the function is only available starting
* with PHP 5.4
*
* @TODO Remove this once 5.3 support for ownCloud is dropped
* @param $message
* @return string
*/
protected static function hexToBin($message) {
if (function_exists('hex2bin')) {
return hex2bin($message);
}

return pack("H*", $message);
}

/**
* @param string $message The message to authenticate
* @param string $password Password to use (defaults to `secret` in config.php)
* @return string Calculated HMAC
*/
public function calculateHMAC($message, $password = '') {
if($password === '') {
$password = $this->config->getSystemValue('secret');
}

// Append an "a" behind the password and hash it to prevent reusing the same password as for encryption
$password = hash('sha512', $password . 'a');

$hash = new Crypt_Hash('sha512');
$hash->setKey($password);
return $hash->hash($message);
}

/**
* Encrypts a value and adds an HMAC (Encrypt-Then-MAC)
* @param string $plaintext
* @param string $password Password to encrypt, if not specified the secret from config.php will be taken
* @return string Authenticated ciphertext
*/
public function encrypt($plaintext, $password = '') {
if($password === '') {
$password = $this->config->getSystemValue('secret');
}
$this->cipher->setPassword($password);

$iv = \OC_Util::generateRandomBytes($this->ivLength);
$this->cipher->setIV($iv);

$ciphertext = bin2hex($this->cipher->encrypt($plaintext));
$hmac = bin2hex($this->calculateHMAC($ciphertext.$iv, $password));

return $ciphertext.'|'.$iv.'|'.$hmac;
}

/**
* Decrypts a value and verifies the HMAC (Encrypt-Then-Mac)
* @param string $authenticatedCiphertext
* @param string $password Password to encrypt, if not specified the secret from config.php will be taken
* @return string plaintext
* @throws \Exception If the HMAC does not match
*/
public function decrypt($authenticatedCiphertext, $password = '') {
if($password === '') {
$password = $this->config->getSystemValue('secret');
}
$this->cipher->setPassword($password);

$parts = explode('|', $authenticatedCiphertext);
if(sizeof($parts) !== 3) {
throw new \Exception('Authenticated ciphertext could not be decoded.');
}

$ciphertext = self::hexToBin($parts[0]);
$iv = $parts[1];
$hmac = self::hexToBin($parts[2]);

$this->cipher->setIV($iv);

if(!StringUtils::equals($this->calculateHMAC($parts[0].$parts[1], $password), $hmac)) {
throw new \Exception('HMAC does not match.');
}

return $this->cipher->decrypt($ciphertext);
}

}
46 changes: 46 additions & 0 deletions lib/private/security/stringutils.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?php
/**
* Copyright (c) 2014 Lukas Reschke <[email protected]>
* This file is licensed under the Affero General Public License version 3 or
* later.
* See the COPYING-README file.
*/

namespace OC\Security;

class StringUtils {

/**
* Compares whether two strings are equal. To prevent guessing of the string
* length this is done by comparing two hashes against each other and afterwards
* a comparison of the real string to prevent against the unlikely chance of
* collisions.
*
* Be aware that this function may leak whether the string to compare have a different
* length.
*
* @param string $expected The expected value
* @param string $input The input to compare against
* @return bool True if the two strings are equal, otherwise false.
*/
public static function equals($expected, $input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Call it hashEquals for clarity?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is only used mostly for crypto, how about just moving this to the Crypto class ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Call it hashEquals for clarity?

Ewww, I believe that hashEquals is a really bad name. I don't understand why new functions get called such a cryptic name. Other languages are going with something as "ConstantTimeCompare".

This is named after the Symfony 2 Security Components and I'm kinda okay with it tbh.

If this is only used mostly for crypto, how about just moving this to the Crypto class ?

Actually there are some other use-cases for this as well though there are not that many, but I'd like to keep this in this class as it makes things easier in case we want to add more such security functions in the future.


if(!is_string($expected) || !is_string($input)) {
return false;
}

if(function_exists('hash_equals')) {
return hash_equals($expected, $input);
}

$randomString = \OC_Util::generateRandomBytes(10);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the public api for random strings here?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's for stable7 where the interface not exists. - Master will have this changed. I don't think it makes a difference if we use the non-public API here for that. - It's doomed to die anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, didn't notice this was against stable7


if(hash('sha512', $expected.$randomString) === hash('sha512', $input.$randomString)) {
if($expected === $input) {
return true;
}
}

return false;
}
}
13 changes: 13 additions & 0 deletions lib/private/server.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use OC\Files\Node\Root;
use OC\Files\View;
use OCP\IServerContainer;
use OC\Security\Crypto;

/**
* Class Server
Expand Down Expand Up @@ -199,6 +200,9 @@ function __construct() {
$this->registerService('Search', function ($c) {
return new Search();
});
$this->registerService('Crypto', function ($c) {
return new Crypto(\OC::$server->getConfig());
});
$this->registerService('Db', function ($c) {
return new Db();
});
Expand Down Expand Up @@ -479,6 +483,15 @@ function getSearch() {
return $this->query('Search');
}

/**
* Returns a Crypto instance
*
* @return \OCP\Security\ICrypto
*/
function getCrypto() {
return $this->query('Crypto');
}

/**
* Returns an instance of the db facade
*
Expand Down
1 change: 1 addition & 0 deletions lib/private/setup.php
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ public static function install($options) {
//generate a random salt that is used to salt the local user passwords
$salt = OC_Util::generateRandomBytes(30);
OC_Config::setValue('passwordsalt', $salt);
OC_Config::setValue('secret', OC_Util::generateRandomBytes(96));

//write the config file
OC_Config::setValue('trusted_domains', $trustedDomains);
Expand Down
46 changes: 46 additions & 0 deletions lib/public/security/icrypto.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?php
/**
* Copyright (c) 2014 Lukas Reschke <[email protected]>
* This file is licensed under the Affero General Public License version 3 or
* later.
* See the COPYING-README file.
*/

namespace OCP\Security;

/**
* Class Crypto provides a high-level encryption layer using AES-CBC. If no key has been provided
* it will use the secret defined in config.php as key. Additionally the message will be HMAC'd.
*
* Usage:
* $encryptWithDefaultPassword = \OC::$server->getCrypto()->encrypt('EncryptedText');
* $encryptWithCustomPassword = \OC::$server->getCrypto()->encrypt('EncryptedText', 'password');
*
* @package OCP\Security
*/
interface ICrypto {

/**
* @param string $message The message to authenticate
* @param string $password Password to use (defaults to `secret` in config.php)
* @return string Calculated HMAC
*/
public function calculateHMAC($message, $password = '');

/**
* Encrypts a value and adds an HMAC (Encrypt-Then-MAC)
* @param string $plaintext
* @param string $password Password to encrypt, if not specified the secret from config.php will be taken
* @return string Authenticated ciphertext
*/
public function encrypt($plaintext, $password = '');

/**
* Decrypts a value and verifies the HMAC (Encrypt-Then-Mac)
* @param string $authenticatedCiphertext
* @param string $password Password to encrypt, if not specified the secret from config.php will be taken
* @return string plaintext
* @throws \Exception If the HMAC does not match
*/
public function decrypt($authenticatedCiphertext, $password = '');
}
25 changes: 25 additions & 0 deletions lib/public/security/stringutils.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php
/**
* Copyright (c) 2014 Lukas Reschke <[email protected]>
* This file is licensed under the Affero General Public License version 3 or
* later.
* See the COPYING-README file.
*/


namespace OCP\Security;

class StringUtils {
/**
* Compares whether two strings are equal. To prevent guessing of the string
* length this is done by comparing two hashes against each other and afterwards
* a comparison of the real string to prevent against the unlikely chance of
* collisions.
* @param string $expected The expected value
* @param string $input The input to compare against
* @return bool True if the two strings are equal, otherwise false.
*/
public static function equals($expected, $input) {
return \OC\Security\StringUtils::equals($expected, $input);
}
}
Loading