#1764 Daemon needs to check config file security

3.2.1
closed-fixed
Anton Pak
5
2013-06-20
2012-09-10
dr_mohan
No

Right now /etc/openhpi/openhpi.conf file is packaged with 0640 permissions and root,root access. But daemon does not check for these permissions before proceeding. More over it accepts a different config file with -c option. Daemon needs to check the permission of the config file before proceeding. If the permissions are not set correctly it needs to exit.
This bug, suggested by Anton, is something all of us agree on.

Discussion

  • dr_mohan
    dr_mohan
    2012-09-14

    New patch uploaded. Tested only on RHEL 6.2. Testing it on other platforms like SuSE, Windows etc could help

     
  • Anton Pak
    Anton Pak
    2012-09-14

    1) Suggest using geteuid() and getegid() instead of CONF_UID and CONF_GID.
    2) Not sure if the code works on win32. May be it makes sense to check file permissions only for unix system?

     
  • dr_mohan
    dr_mohan
    2012-09-18

    Will change the code with #ifndef _WIN32. We package the file as root:root and 644 permissions, so not sure if geteuid and seteuid could be set to some other user other than root and the file could be accessed while it is not root.

     
  • dr_mohan
    dr_mohan
    2012-09-18

    If there are calls to get the UID and GID of root, I could use that along with geteuid() and getegid(). I could not find any. New patch is attached. Could you test it with WIN32?

     
  • Anton Pak
    Anton Pak
    2012-09-19

    My point is that the file permissions shall be compared with user credentials that openhpid is currently running.

    Two examples:

    1) OpenHPI can be installed in the system and any user is able to prepare config file and run it.
    Any user can can /usr/sbin/openhpid -c /home/user/openhpi.conf.

    2) I often run "./configure --prefix=/home/avpak/X.Y.Z; make; make install" under my user account.
    This installs openhpi-X.Y.Z in /home/avpak/X.Y.Z and all files belong to me.

     
  • dr_mohan
    dr_mohan
    2012-09-20

    I agree with your point. This makes me thinking may be we need to restrict the access of the file only to the user, forbidding group and other access completely. As root:root is different from user:group. Many users may be in the same group exposing the file to many people.
    Instead of packaging that file as 640 we need to package it as 600. What do you think?

     
  • Anton Pak
    Anton Pak
    2012-09-20

    Agreed.

     
  • dr_mohan
    dr_mohan
    2012-09-21

    Uploaded the latest patch with geteuid. Could you please test it? It works on RHEL.

     
  • dr_mohan
    dr_mohan
    2012-09-21

    Non _WIN32, 600 permissions and geteuid()

     
  • Anton Pak
    Anton Pak
    2012-09-24

    Your patch works!
    I made several small changes. See uploaded file.
    Also what do you think about the following checks:
    - if the path points to regular file. (Not a device, pipe, socket, or symlink)
    - if group/other write permissions are not set for the parent dir.

     
  • dr_mohan
    dr_mohan
    2012-09-25

    Yes, they are good suggestions. Let me try to add them.
    In case where the conf file is owned by root, instead of just saying UID shall be geteuid(), we could say run as sudo or create/change the conf file to be owned by uid geteuid() if this is a user specific file.
    When I talked to couple of users, found that they think differently. Their tendency is to make it to work by changing the permissions even if it is risky.

     
  • dr_mohan
    dr_mohan
    2012-10-19

    With non-root user and directory checks

     
  • dr_mohan
    dr_mohan
    2012-10-19

    Added a new patch. This takes care of all of the situations. Could you please check.

     
  • Anton Pak
    Anton Pak
    2012-10-29

    Looks good for me.
    I'd only used PATH_MAX from limits.h instead of OH_MAX_TEXT_BUFFER_LENGTH.
    Thanks for the patch!

     
  • dr_mohan
    dr_mohan
    2012-10-30

    • milestone: 3074952 --> 3.3.x
    • status: open --> closed-fixed
     
  • dr_mohan
    dr_mohan
    2012-10-30

    Fixed in checkin #7521

     
  • dr_mohan
    dr_mohan
    2012-10-30

    patch that was checked in

     
  • dr_mohan
    dr_mohan
    2013-06-20

    • Group: 3.3.x --> 3.2.1
     
  • Tariq Shureih
    Tariq Shureih
    2013-06-20

    *ATTENTION**
    This account is disabled and is no longer accessed by the recipient.
    Please remove it from your address book.

    Thanks