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

SMB/CIFS password visible in processlist #6092

Closed
Ewoxx opened this issue Nov 27, 2013 · 26 comments · Fixed by #10673
Closed

SMB/CIFS password visible in processlist #6092

Ewoxx opened this issue Nov 27, 2013 · 26 comments · Fixed by #10673

Comments

@Ewoxx
Copy link

Ewoxx commented Nov 27, 2013

When using external storage "SMB/CIFS" username and password becomes visible to users logged in on the server.

The password is visible when listing processes on the server (ps -ef)

This is a running process:
sh -c smbclient -N -U 'DOMAIN/UID%PASSWORD' -O 'TCP_NODELAY IPTOS_LOWDELAY SO_KEEPALIVE SO_RCVBUF=8192 SO_SNDBUF=8192' -O 'TCP_NODELAY IPTOS_LOWDELAY SO_KEEPALIVE SO_RCVBUF=8192 SO_SNDBUF=8192' -d 0 '//sambaserver/share' -c 'dir "UID\Documents\Download\desktop.ini"' 2>/dev/null

@PVince81
Copy link
Contributor

There might be a way to pass the password to smbclient using popen's stdin

@bantu
Copy link

bantu commented Dec 1, 2013

@PVince81 Not sure whether smbclient accepts password from stdin, but it supports reading credentials from a file. The option is -A, --authentication-file=FILE Get the credentials from a file.

@PVince81
Copy link
Contributor

PVince81 commented Dec 2, 2013

It probably does, @icewind1991 did it in his PR.
But good to know!

@Ewoxx
Copy link
Author

Ewoxx commented Dec 16, 2013

Just out of curiosity, where are we on this issue? Just installed v6.0 and its still there, is it awaiting commit? :)

@PVince81
Copy link
Contributor

There is a full rewrite of the smbclient storage in the PR above.
Before that PR can be merged it needs to be reviewed and regression tested.

@Ewoxx
Copy link
Author

Ewoxx commented Dec 17, 2013

Sounds good!
How does it work after that? Will release an update with a bunch of fixes and this one in it?

@PVince81
Copy link
Contributor

Since it's a full rewrite it will probably be released as part of OC 7.

@icewind1991 would it be possible to extract the password hiding/piping approach of your PR and apply it to OC 6's
smb4php ?

@icewind1991
Copy link
Contributor

Not without significant work I think.

It may be safer to backport the whole thing to oC6 once it's properly tested

@Ewoxx you can test the new SMB backend by copying the files_external app from https://github.com/owncloud/core/tree/smb/apps into your installation, it should work just fine in oC6.
(Make sure to backup any important data, that everything works for me is no guarantee until we've done more testing)

@Ewoxx
Copy link
Author

Ewoxx commented Dec 19, 2013

Not sure if i did something wrong. But id didn't work.
This is what I did:
cd /var/www
wget https://github.com/owncloud/core/archive/smb.zip
unzip smp.zip
mv /var/www/html/owncloud/apps/files_external .
cp -rv /var/www/core-smb/apps/files_external /var/www/html/owncloud/apps
chown -R apache: /var/www/html/owncloud/apps/files_external
service httpd restart

The effect is that the "External storage support" application disappear from OC and also the mount.

Any idea?

@PVince81
Copy link
Contributor

I think this is not backportable.
Also the SMB client rewrite didn't make it into OC 7.
So will have to go to OC 8.

@PVince81 PVince81 modified the milestones: ownCloud 8, ownCloud 7 Jun 20, 2014
@segdy
Copy link

segdy commented Jul 3, 2014

This is sad. Is there an intermediate fix for this?
Actually this causes me sleepless nights to know that the password appears in the "publicly" visible process list.

@PVince81
Copy link
Contributor

PVince81 commented Jul 3, 2014

Suggestions:

  1. try mounting the SMB storage directly on the Linux filesystem (using "cifs").
    For example mount it to "/mnt/smbstorage1".
    Then in ownCloud, create a "Local" external storage that points to "/mnt/smbstorage1".

This only works if it's the admin who is mounting the storage, not the users mounting them themselves.

  1. You could also hack smb4php/smb.php yourself and try hard-coding the option -A like @bantu suggested here: SMB/CIFS password visible in processlist #6092 (comment)
    But it means you need to have a local file containing the passwords of all the SMB mounts.

  2. the clean alternative: need to fix smb4php/smb.php (which is a third party library) to not pass the password directly on the command line but pass it by writing to the process' stdin. It could be a matter of calling fwrite() with the password on the process handle after the popen() call. And also remove the password from the SMB URL that is built.

@PVince81
Copy link
Contributor

PVince81 commented Jul 3, 2014

@Xenopathic would you have time to have a look at my suggestion from point 3) ?

@segdy
Copy link

segdy commented Jul 3, 2014

1-2 are no options because I need to dynamically mount with user credentials.

  1. would be great to implement!!

@RobinMcCorkell
Copy link
Member

@PVince81 I'm approaching release for my current project, so I'll see what I can do next week. Although perhaps it would be better to focus efforts on getting the SMB rewrite done? I heard it mentioned that the best solution would be a PHP native module to hook into SMB, rather than needing smbclient - I could do that?

@PVince81
Copy link
Contributor

PVince81 commented Jul 3, 2014

@Xenopathic also approaching OC 7 release now, so I have time next week as well.

The current rewrite by @icewind1991 is NOT using a native approach. It's just a better way to wrap the smbclient command. The different is that instead of running a single smbclient command for every FS call it will call it once in SHELL mode and pipe a sequence of commands directly. So it's faster.
And I guess that it might do the same with the password (piping).

The fix I referred to at step 3) might be easier than thought, so might be worth a try.
At least so that @segdy can sleep better.

@RobinMcCorkell
Copy link
Member

@PVince81 I understand that the rewrite by @icewind1991 pipes commands to smbclient, what I was saying is that I heard the ultimate best approach would be a PHP native module, so that smbclient is not required at all. I've had a look, and it seems fairly straight forward to do.

@PVince81
Copy link
Contributor

PVince81 commented Jul 4, 2014

Yes, the ultimate approach 😄

@segdy
Copy link

segdy commented Jul 5, 2014

I want to add another very problematic thing (that can be fixed instantly): The password is also written in plaintext to data/owncloud.log!

@ghost
Copy link

ghost commented Jul 5, 2014

@segdy Which OC version are you running? Normally no passwords should be written to the logfiles: #6053, #5218

@segdy
Copy link

segdy commented Jul 5, 2014

Hmm, weird. In my owncloud.log I have

{"reqId":"53b7591f67f7a","app":"PHP","message":"popen(TZ=UTC smbclient -N  -U 'user%TopSecretPassword' -O 'TCP_NODELAY IPTOS_LOWDELAY SO_KEEPALIVE SO_RCVBUF=8192 SO_SNDBUF=8192' ...

My version:

# dpkg -l | grep owncloud
ii  owncloud                           7.0.0rc1                           all          ownCloud Server - Private file sync and share server
ii  owncloud-mysql                     5.0.14.a+dfsg-1~bpo70+2            all          meta-package providing MySQL dependencies for ownCloud

@ghost
Copy link

ghost commented Jul 5, 2014

Ah, those commits are just removing a username and password if they are logged in a format like:

Think it could be useful to track this in a separate issue.

@segdy
Copy link

segdy commented Jul 5, 2014

Ok, done: #9467

@svensol
Copy link

svensol commented Sep 9, 2014

v7.0.2 shows passwords in plaintext when running "top" or "ps aux" when using the OC pass-through authentication.

is that supposed to be fixed?

@PVince81
Copy link
Contributor

PVince81 commented Sep 9, 2014

No. There is work in progress in OC 8 to provide a native SMB external storage (#9480) which does not rely on the smbclient command any more.

@PVince81 PVince81 added the triage label Sep 9, 2014
@svensol
Copy link

svensol commented Sep 9, 2014

Ok... thanks. I'll keep an eye open for v8 :-)

@craigpg craigpg removed the triage label Sep 19, 2014
@craigpg craigpg modified the milestones: ownCloud 7 backlog, ownCloud 8 Sep 19, 2014
@DeepDiver1975 DeepDiver1975 modified the milestones: ownCloud 7 backlog, backlog Jan 8, 2015
@MorrisJobke MorrisJobke modified the milestones: 8.1-next, backlog Feb 17, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.