search

mohandev2 / openhpi_old

Other
0 stars 0 forks source link

umask(0) makes created files world readable/writeable #1916

Closed mohandev2 closed 6 years ago

mohandev2 commented 8 years ago

Seems the daemon code hard-codes umask to 0 causing all created files (eg. logs) to have 0666 permissions, which looks wrong. The daemon rather should be restrictive or leave the default umask on the user.

https://sourceforge.net/p/openhpi/code/HEAD/tree/openhpi/trunk/openhpid/openhpid-posix.cpp#l298

Reported by: sharkcz

Original Ticket: openhpi/bugs/1916

mohandev2 commented 8 years ago

to reproduce add

handler libdyn_simulator {
        entity_root = "{SYSTEM_CHASSIS,9}"
        logflags = "file stdout"
        logfile = "/var/log/dynsim"
        logfile_max = "5"
}

into /etc/openhpi/openhpi.conf and check the state after starting the daemon

[root@rawhide ~]# ls -al /var/log/dynsim*.log
-rw-rw-rw-. 1 root root 120 Sep 13 11:07 /var/log/dynsim00.log

with the proposed patch applied it looks as

[root@rawhide ~]# ls -al /var/log/dynsim*.log
-rw-r--r--. 1 root root 120 Sep 13 12:11 /var/log/dynsim03.log

Original comment by: sharkcz

mohandev2 commented 8 years ago

systemd allows to manage the default umask on service level, see https://www.freedesktop.org/software/systemd/man/systemd.exec.html#UMask=

Original comment by: sharkcz

mohandev2 commented 8 years ago

Patch looks good. Only problem is if the user has a different umask set (not default 0022) and starts the openhpid -c .... then the file will be created based on the non-default permissions. This might be surprise to the user. Is there a way to check the umask? If it is not 0022 (default) we could warn the user.

Original comment by: mohandev2

mohandev2 commented 8 years ago

umask() returns the previous umask value, but I would rather skip setting it completely.

utils/uid_utils.c is the second place that sets umask in openhpi code base

Original comment by: sharkcz

mohandev2 commented 8 years ago

I agree with you. utils/uid_utils.c sets the value back though. So may be we need to just give a message to the user if the umask is not 022 and continue. Please let me know if the attached patch is ok.

Original comment by: mohandev2

mohandev2 commented 8 years ago

Looks good to me, maybe the CRIT level could be lowered, because running with a stricter umask, eg. 027, should not be sanctioned.

Original comment by: sharkcz

mohandev2 commented 8 years ago

Original comment by: mohandev2

mohandev2 commented 8 years ago

Fixed in checkin #7673

Original comment by: mohandev2

mohandev2 commented 7 years ago

Original comment by: mohandev2