Store credentials only in session if required #11747

LukasReschke · 2014-10-23T18:18:34Z

Fixes:

getUserSession stores passwords #7199

PVince81 · 2014-10-23T18:23:33Z

apps/files_external/lib/smb_oc.php

+		$mountpoints = \OC_Mount_Config::getAbsoluteMountPoints($params['uid']);
+		$mountpointClasses = array();
+		foreach($mountpoints as $mountpoint) {
+			$mountpointClasses[$mountpoint['class']] = '';


You could set it to true.

Good point - will change.

LukasReschke · 2014-10-24T09:08:18Z

Let's schedule this for 7.0.4 - seems like a too big change for me to merge it right away so shortly before a release.

This PR is there to store the user credentials only in the session (and therefore unprotected on the disk) in case the SMB_OC storage is used. Furthermore, the credentials are now stored encrypted instead of being plain text.

While this PR seems big it's mostly the backport of the \OC\Security\Crypto class which we already have in master and is very useful to encrypt things with a global key. I believe it's better to backport this class instead of adding yet another encryption function just for this sake.

The actual change regarding the storage is not that big: 30e39ab

@karlitschek Would that be ok for you?

karlitschek · 2014-10-24T12:03:50Z

@LukasReschke agreed

LukasReschke · 2014-11-17T11:59:23Z

@icewind1991 Could you review this please? - Thank you!
(Will have to be ported to master once merged)

icewind1991 · 2014-11-17T12:39:31Z

lib/private/security/stringutils.php

+			return hash_equals($expected, $input);
+		}
+
+		$randomString = \OC_Util::generateRandomBytes(10);


Use the public api for random strings here?

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.

Ok, didn't notice this was against stable7

icewind1991 · 2014-11-17T12:45:12Z

Code looks good 👍

LukasReschke · 2014-11-20T15:03:36Z

@MorrisJobke Care to review this? - The backport of #11947 depends on this one.

Conflicts: lib/repair/repairconfig.php

scrutinizer-notifier · 2014-11-20T15:48:38Z

The inspection completed: 7 new issues, 26 updated code elements

cedric-dufour · 2014-11-21T07:32:25Z

(coming from #11939)
Just a thought: why not allow/use the PHP session ID and the configured secret when encrypting session data? If I'm correct, the session ID is stored nowhere on disk (session files using the hashed session ID as their name). This would make much more difficult to uncover the encrypted session data, no?

LukasReschke · 2014-11-21T07:46:39Z

That's even more snake-oil. Let's not overcomplicate things. Especially because we regenerate the session a few times, then everything would be broken again = bad :-)

LukasReschke · 2014-11-21T07:52:20Z

(Especially because we're going to store it in mount.json anyways as is documented on the linked issue)

LukasReschke · 2014-11-21T07:55:11Z

See #12216

PVince81 · 2014-11-26T13:33:15Z

@Xenopathic what do you think ?

RobinMcCorkell · 2014-11-26T14:36:36Z

I like the crypto element, it certainly makes it a bit better than storing the password raw in the session! 👍 from me

ghost · 2014-11-26T15:21:47Z

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3331/
🚀 Test PASSed. 🚀

PVince81 · 2014-11-27T13:59:14Z

Tagging as 7.0 since the backport was approved.

PVince81 · 2014-11-27T14:15:07Z

Steps to test:

Create a user "user1" whose username/password is the same as some SMB storage
Mount that SMB external storage as system mount with type "SMB using OC login" (ignore the red box because the "root" user doesn't have an SMB account)
Login as "user1"
See that there is a SMB mount point and that files can be uploaded/downloaded

That's it. Worked for me. 👍

I'd like to wait for @jnfrmarks' feedback before merging this, to make sure there is still enough time to test this.

LukasReschke · 2014-12-01T10:06:11Z

@jnfrmarks We promised this for 7.0.4 to somebody. Can you please test this?

PVince81 · 2014-12-01T10:07:55Z

Let's merge it then, already got three thumbs up and backport approval.

…IfRequired Store credentials only in session if required

LukasReschke · 2014-12-01T10:10:01Z

Will port to master.

LukasReschke · 2014-12-01T10:19:36Z

Master: #12523

jnfrmarks · 2014-12-01T20:48:51Z

@PVince81 @LukasReschke

I don't understand what the bug is. I ran through the steps listed earlier on how to reproduce the issue but I don't know what the issue is :) I was able to successfully create the smb mount and navigate through the files using 7.0.3.

LukasReschke · 2014-12-01T20:51:04Z

@jnfrmarks You need to look at the PHP session files (depends on the OS where located) when logged-in with and without the patch.

But if everything works as before this means that this patch works fine - no visible changes to the user :)

jnfrmarks · 2014-12-02T15:11:22Z

@LukasReschke

So far, I have only tested this with 7.0.3 - I'm trying to reproduce the original issue. Where do I look for the php session files? My server is on either CentOS or Ubuntu (I'm testing with both)

LukasReschke · 2014-12-02T15:12:49Z

So far, I have only tested this with 7.0.3 - I'm trying to reproduce the original issue. Where do I look for the php session files? My server is on either CentOS or Ubuntu (I'm testing with both)

Depends on your operating system Ubuntu is AFAIK /var/lib/php5 and CentOS is /var/lib/php/session. I might be wrong though :-)

LukasReschke added 2 - Developing security enhancement app:files_external labels Oct 23, 2014

PVince81 reviewed Oct 23, 2014
View reviewed changes

gig13 added the blue-ticket label Nov 3, 2014

This was referenced Nov 4, 2014

Add $uid and $password templates to 'files_external' configuration #11939

Closed

Add OCP\Security\IHasher #11947

Merged

LukasReschke force-pushed the storeCredentialsOnlyInSessionIfRequired branch from 86e9d3c to 1835b4d Compare November 17, 2014 11:57

LukasReschke changed the title ~~WIP: Store credentials only in session if required~~ Store credentials only in session if required Nov 17, 2014

LukasReschke added 3 - To Review and removed 2 - Developing labels Nov 17, 2014

icewind1991 reviewed Nov 17, 2014
View reviewed changes

LukasReschke force-pushed the storeCredentialsOnlyInSessionIfRequired branch from 1835b4d to 539d7ce Compare November 20, 2014 15:03

LukasReschke added 2 commits November 20, 2014 16:05

Backport \OC\Security\Crypto to ownCloud 7

ec853da

Conflicts: lib/repair/repairconfig.php

Only store user credentials when SMB_OC storage is enabled

74b68e1

LukasReschke force-pushed the storeCredentialsOnlyInSessionIfRequired branch from 539d7ce to 74b68e1 Compare November 20, 2014 15:05

cedric-dufour mentioned this pull request Nov 26, 2014

config option for external storage #12216

Closed

15 tasks

PVince81 added the 7.0-current label Nov 27, 2014

PVince81 pushed a commit that referenced this pull request Dec 1, 2014

Merge pull request #11747 from owncloud/storeCredentialsOnlyInSession…

3917383

…IfRequired Store credentials only in session if required

PVince81 merged commit 3917383 into stable7 Dec 1, 2014

PVince81 deleted the storeCredentialsOnlyInSessionIfRequired branch December 1, 2014 10:07

LukasReschke mentioned this pull request Dec 1, 2014

Only store user credentials when SMB_OC storage is enabled #12523

Merged

LukasReschke mentioned this pull request Dec 10, 2014

Encrypt or obfuscate the username-passwords in session files for external storage #12756

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store credentials only in session if required #11747

Store credentials only in session if required #11747

LukasReschke commented Oct 23, 2014

PVince81 Oct 23, 2014

LukasReschke Oct 23, 2014

LukasReschke commented Oct 24, 2014

karlitschek commented Oct 24, 2014

LukasReschke commented Nov 17, 2014

icewind1991 Nov 17, 2014

LukasReschke Nov 17, 2014

icewind1991 Nov 17, 2014

icewind1991 commented Nov 17, 2014

LukasReschke commented Nov 20, 2014

scrutinizer-notifier commented Nov 20, 2014

cedric-dufour commented Nov 21, 2014

LukasReschke commented Nov 21, 2014

LukasReschke commented Nov 21, 2014

LukasReschke commented Nov 21, 2014

PVince81 commented Nov 26, 2014

RobinMcCorkell commented Nov 26, 2014

ghost commented Nov 26, 2014

PVince81 commented Nov 27, 2014

PVince81 commented Nov 27, 2014

LukasReschke commented Dec 1, 2014

PVince81 commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

jnfrmarks commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

jnfrmarks commented Dec 2, 2014

LukasReschke commented Dec 2, 2014

Store credentials only in session if required #11747

Store credentials only in session if required #11747

Conversation

LukasReschke commented Oct 23, 2014

PVince81 Oct 23, 2014

Choose a reason for hiding this comment

LukasReschke Oct 23, 2014

Choose a reason for hiding this comment

LukasReschke commented Oct 24, 2014

karlitschek commented Oct 24, 2014

LukasReschke commented Nov 17, 2014

icewind1991 Nov 17, 2014

Choose a reason for hiding this comment

LukasReschke Nov 17, 2014

Choose a reason for hiding this comment

icewind1991 Nov 17, 2014

Choose a reason for hiding this comment

icewind1991 commented Nov 17, 2014

LukasReschke commented Nov 20, 2014

scrutinizer-notifier commented Nov 20, 2014

cedric-dufour commented Nov 21, 2014

LukasReschke commented Nov 21, 2014

LukasReschke commented Nov 21, 2014

LukasReschke commented Nov 21, 2014

PVince81 commented Nov 26, 2014

RobinMcCorkell commented Nov 26, 2014

ghost commented Nov 26, 2014

PVince81 commented Nov 27, 2014

PVince81 commented Nov 27, 2014

LukasReschke commented Dec 1, 2014

PVince81 commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

jnfrmarks commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

jnfrmarks commented Dec 2, 2014

LukasReschke commented Dec 2, 2014