Bug 40227 - Add diagnostic test for file and socket permissions
Add diagnostic test for file and socket permissions
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC - System diagnostic
UCS 4.1
Other Linux
: P5 normal (vote)
: UCS 4.2-2-errata
Assigned To: Lukas Oyen
Florian Best
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-11 15:24 CET by Florian Best
Modified: 2017-09-20 15:03 CEST (History)
2 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?:
ISV affected?:
Ticket number:
Bug group (optional): Security
Max CVSS v3 score:
oyen: Patch_Available+


Attachments
40227-diagnostic-file-permissions-420.patch (9.18 KB, patch)
2017-06-12 17:12 CEST, Lukas Oyen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Best univentionstaff 2015-12-11 15:24:34 CET
We should add a check for the System Diagnostic UMC module which checks if specific files sockets have the correct owners and permissions set:

/etc/machine.secret
/etc/ldap.secret
/etc/univention/ssl/*
/var/run/slapd/ldapi (→ Bug #39811)
/var/run/univention/management-console/*.socket
/var/run/memcached-univention-self-service.socket
/var/run/univention-saml/memcached.socket

Especially the SSL certificate renewal might cause permission problems if done manually by users.
Comment 1 Florian Best univentionstaff 2015-12-11 15:35:54 CET
Also the file: $(ucr get saml/idp/certificate/privatekey)
Comment 2 Florian Best univentionstaff 2015-12-11 17:46:27 CET
/etc/idp-ldap-user.secret
Comment 3 Florian Best univentionstaff 2015-12-14 17:47:07 CET
Also:
/var/run/uvmm.socket

/var/cache/univention-appcenter/*.pkl
/var/cache/univention-config/cache
/var/cache/univention-directory-listener/well-known-sid-name-mapping_modrdn.pickle
/var/cache/univention-quota/*
/var/cache/univention-mail-cyrus/old_dn
/var/cache/univention-mail-cyrus/cyrus-mailboxrename.pickle
/var/cache/univention-management-console/acls
/var/cache/univention-directory-listener/nfs-shares.oldObject
/var/cache/univention-directory-listener/samba-shares.oldObject
/var/lib/univention-connector/ad
/etc/univention/connector/s4internal.sqlite
/etc/univention/*/internal.cfg
/var/lib/univention-nagios/check_univention_replication.cache
Comment 4 Florian Best univentionstaff 2016-04-14 07:54:28 CEST
/etc/libnss-ldap.conf (might contain machine.secret)
Comment 5 Florian Best univentionstaff 2016-07-19 17:38:49 CEST
/etc/pam_ldap.secret
Comment 6 Lukas Oyen univentionstaff 2017-06-12 17:12:17 CEST
Created attachment 8916 [details]
40227-diagnostic-file-permissions-420.patch

Add a check to ensure the existence of certain files and the correct owner:group and mode. This is quite rigid and does not allow anything else than the default.

Note: currently ACLs are not considered.
Comment 7 Florian Best univentionstaff 2017-06-12 17:14:15 CEST
very nice!
Comment 8 Lukas Oyen univentionstaff 2017-08-01 16:31:07 CEST
Committed in r81622 - r81623 (advisory r81649).
Comment 9 Florian Best univentionstaff 2017-08-01 18:47:52 CEST
Every directory in /var/cache/univention-* is tested for 0755? This seems wrong to me:

+		else:
+			yield check_file(path, 'root', 'root', 0755)
Comment 10 Florian Best univentionstaff 2017-08-02 12:25:55 CEST
REOPEN: On a DC Slave:

[2017-08-02 05:57:30.000303] E     Überprüfe Datei Berechtigungen
[2017-08-02 05:57:30.000380] E     File '/etc/ldap.secret' does not exist.
[2017-08-02 05:57:30.000457] E     File '/etc/idp-ldap-user.secret' does not exist.
[2017-08-02 05:57:30.000537] E     File '/etc/univention/ssl/openssl.cnf' does not exist.
[2017-08-02 05:57:30.000614] E     File '/etc/univention/ssl/password' does not exist.
[2017-08-02 05:57:30.000692] E     File '/etc/univention/ssl/ucs-sso.autotest095.local' does not exist.
[2017-08-02 05:57:30.000774] E     Datei '/etc/univention/ssl/slave095.autotest095.local' hat den Besitzer 'root:DC Backup Hosts', während 'slave095$:DC Backup Hosts' erwartet war.
Comment 11 Florian Best univentionstaff 2017-08-02 12:27:16 CEST
On a Memberserver:

[2017-08-02 04:49:01.153653] E     Überprüfe Datei Berechtigungen
[2017-08-02 04:49:01.153727] E     File '/etc/ldap.secret' does not exist.
[2017-08-02 04:49:01.153801] E     File '/etc/idp-ldap-user.secret' does not exist.
[2017-08-02 04:49:01.153877] E     Datei '/etc/univention/ssl' hat den Besitzer 'root:root', während 'root:DC Backup Hosts' erwartet war.
[2017-08-02 04:49:01.153953] E     File '/etc/univention/ssl/openssl.cnf' does not exist.
[2017-08-02 04:49:01.154030] E     File '/etc/univention/ssl/password' does not exist.
[2017-08-02 04:49:01.154107] E     Datei '/etc/univention/ssl/ucsCA' hat den Besitzer 'root:root', während 'root:DC Backup Hosts' erwartet war.
[2017-08-02 04:49:01.154183] E     File '/etc/univention/ssl/ucs-sso.autotest096.local' does not exist.
[2017-08-02 04:49:01.154263] E     Datei '/etc/univention/ssl/member096.autotest096.local' hat den Besitzer 'root:root', während 'member096$:DC Backup
Comment 12 Lukas Oyen univentionstaff 2017-08-02 16:39:57 CEST
Reworked in r81712 - 81714. This captures the current status and might be changed.
Comment 13 Florian Best univentionstaff 2017-08-02 18:37:33 CEST
Please add a check for /var/tmp/univention-management-console-frontend. This is the directory where file uploads are temporary stored and is very security sensitive.
Comment 14 Lukas Oyen univentionstaff 2017-08-03 14:55:49 CEST
(In reply to Florian Best from comment #13)
> Please add a check for /var/tmp/univention-management-console-frontend. This
> is the directory where file uploads are temporary stored and is very
> security sensitive.

Done in r81761.
Comment 15 Arvid Requate univentionstaff 2017-08-07 13:51:03 CEST
Ah, sorry for reopening, checking this one would be cool too:

root@samba4dc:~$ ls -ld /var/lock/sysvol-sync-dir 
-rw-rw-r-- 1 root DC Slave Hosts 0 Aug  7 13:45 /var/lock/sysvol-sync-dir


Lately we had a customer who had changed the group and also taken away the read permission for it. That way, sysvol-sync.sh always failed to acquire a lock.
Comment 16 Lukas Oyen univentionstaff 2017-08-08 14:41:41 CEST
(In reply to Arvid Requate from comment #15)
> root@samba4dc:~$ ls -ld /var/lock/sysvol-sync-dir 
> -rw-rw-r-- 1 root DC Slave Hosts 0 Aug  7 13:45 /var/lock/sysvol-sync-dir


Done in r81895.
Comment 17 Florian Best univentionstaff 2017-08-08 18:18:05 CEST
REOPEN: For a lot of files the check only checks the specific directory mode instead of the critical files.
We should test both, directory mode (that ensures nobody can write new/non-existing files inside the directories) and file mode (which ensures e.g. pickle files aren't modifiable by users).

(In reply to Florian Best from comment #3)
> Also:
> /var/run/uvmm.socket
OK

> /var/cache/univention-appcenter/*.pkl
→ doesn't exists anymore
> /var/cache/univention-config/cache
REOPEN: not detected 
> /var/cache/univention-directory-listener/well-known-sid-name-mapping_modrdn.
> pickle
REOPEN: not detected 
> /var/cache/univention-quota/*
REOPEN: not detected 
> /var/cache/univention-mail-cyrus/old_dn
REOPEN: not detected 
> /var/cache/univention-mail-cyrus/cyrus-mailboxrename.pickle
REOPEN: not detected 
> /var/cache/univention-management-console/acls
REOPEN: /var/cache/univention-management-console/acls/* should be checked, too.
> /var/cache/univention-directory-listener/nfs-shares.oldObject
REOPEN: not detected 
> /var/cache/univention-directory-listener/samba-shares.oldObject
REOPEN: not detected 
> /var/lib/univention-connector/ad
REOPEN: not detected
This should actually be recursive as there are a lot of pickle files.

The same for /var/lib/univention-connector/ad/tmp/

> /etc/univention/connector/s4internal.sqlite
OK
> /etc/univention/*/internal.cfg
REOPEN: not detected.
It seems (in the code I can see this) this is still used in connector and AD connector.
> /var/lib/univention-nagios/check_univention_replication.cache
REOPEN: not detected
Comment 18 Lukas Oyen univentionstaff 2017-08-09 09:30:22 CEST
(In reply to Florian Best from comment #17)
> REOPEN: For a lot of files the check only checks the specific directory mode
> instead of the critical files.
> We should test both, directory mode (that ensures nobody can write
> new/non-existing files inside the directories) and file mode (which ensures
> e.g. pickle files aren't modifiable by users).

I can implement all those if really wanted. But I think this is not a hardening tool/security check.

This should serve as an overview of commonly wrong permissions on files that configure stuff. And not perform safety checks on implementation details.

Keep in mind, that with your changes, if someone decides `/var/cache/univention-mail-cyrus/old_dn` needs slightly different permissions and omits updating this module, several customers could be confused (in the best case).
Comment 19 Florian Best univentionstaff 2017-08-09 12:13:21 CEST
(In reply to Lukas Oyen from comment #18)
> (In reply to Florian Best from comment #17)
> > REOPEN: For a lot of files the check only checks the specific directory mode
> > instead of the critical files.
> > We should test both, directory mode (that ensures nobody can write
> > new/non-existing files inside the directories) and file mode (which ensures
> > e.g. pickle files aren't modifiable by users).
> 
> I can implement all those if really wanted. But I think this is not a
> hardening tool/security check.
Hm, that was my reasoning why I created this bug: To detect security issues.

> This should serve as an overview of commonly wrong permissions on files that
> configure stuff. And not perform safety checks on implementation details.
I don't think so. Everything is still working if those permissions are wrong. Except that the system is vulnerable.

> Keep in mind, that with your changes, if someone decides
> `/var/cache/univention-mail-cyrus/old_dn` needs slightly different
> permissions and omits updating this module, several customers could be
> confused (in the best case).

Nobody should decide that he changes the permissions of /var/cache/univention-mail-cyrus/old_dn.
The file contains pickle which allows code execution as root user.
We had various vulnerabilities due to this (Bug #40245, Bug #40246, Bug #36452).
Comment 20 Lukas Oyen univentionstaff 2017-08-09 12:27:42 CEST
(In reply to Florian Best from comment #19)
> Nobody should decide that he changes the permissions of
> /var/cache/univention-mail-cyrus/old_dn.
> The file contains pickle which allows code execution as root user.
> We had various vulnerabilities due to this (Bug #40245, Bug #40246, Bug
> #36452).

This is an example of a possible change by some developer due to some new requirements.

I think it is plainly wrong to check developer errors with a tool distributed to the customer. This is something CI should cover. As long as the error originates in univention configuration files.

In cases where errors are manually introduced (or by third-party tools), diagnostic checks could test common cases.
Comment 21 Lukas Oyen univentionstaff 2017-08-14 13:21:04 CEST
Updated in r82098 to fix the Jenkins errors.
Comment 22 Lukas Oyen univentionstaff 2017-08-23 10:15:17 CEST
Updated in r82381 / r82408. Jenkins is finally happy.
Comment 23 Lukas Oyen univentionstaff 2017-09-04 11:24:14 CEST
(In reply to Florian Best from comment #19)
> Nobody should decide that he changes the permissions of
> /var/cache/univention-mail-cyrus/old_dn.
> The file contains pickle which allows code execution as root user.
> We had various vulnerabilities due to this (Bug #40245, Bug #40246, Bug
> #36452).

We need a decision if this diagnostic module should cover developer errors. I still think not (see comment 20), but say the word and I'll implement it.
Comment 24 Lukas Oyen univentionstaff 2017-09-04 13:28:46 CEST
As discussed per mail: let's leave it like it is.
Comment 25 Florian Best univentionstaff 2017-09-04 15:24:15 CEST
OK
Comment 26 Erik Damrose univentionstaff 2017-09-20 15:03:46 CEST
<http://errata.software-univention.de/ucs/4.2/166.html>