moinwiki / moin

MoinMoin Wiki Development (2.0+), unstable, for production please use 1.9.x.
https://moinmo.in/
Other
311 stars 91 forks source link

auth/smb_mount.py: subprocess_popen_with_shell_equals_true security issue #1815

Open UlrichB22 opened 1 day ago

UlrichB22 commented 1 day ago

The first issue when running code analysis with 'bandit' as mentioned in #318 against moin is

subprocess_popen_with_shell_equals_true: subprocess call with shell=True identified, security issue.
Test ID: B602
Severity: HIGH
Confidence: HIGH
CWE: [CWE-78](https://cwe.mitre.org/data/definitions/78.html)
File: [src/moin/v](file:///home/hu/Documents/Source/moinwiki/moin2_git/moin/src/moin/auth/smb_mount.py)
Line number: 94
More info: https://bandit.readthedocs.io/en/1.8.0/plugins/b602_subprocess_popen_with_shell_equals_true.html

93              env["PASSWD"] = password.encode(self.coding)
94          subprocess.call(cmd.encode(self.coding), env=env, shell=True)
95

According to the comment in the header of auth/smb_mount.py the code is not complete. In the configure.rst docs there is a TODO 'check if SMBMount still works as documented'.

Is this module still needed? If it is a draft we should remove it, it can still be found in the git history.

sebix commented 1 day ago

And in case it is needed in the future, have a look at https://github.com/nextcloud/user_external/blob/master/lib/SMB.php

RogerHaase commented 22 hours ago

I vaguely remember using SMB to share files under MS DOS in the 1980's, haven't used it since. My vote would be to delete it, maybe add an enhancement issue (or just label this issue as an enhancement) in case a user of SMB appears.

ThomasWaldmann commented 22 hours ago

@RogerHaase SMB or more recently called CIFS is the usual way windows machines share files, so it is not just DOS. :-)

The idea of that auth method was that a (windows) user has configured all users on some windows machine and wants to re-use these user accounts/passwords also for the (intranet) wiki.

Not sure if anybody uses it like that though.