#1764 Daemon needs to check config file security

3.2.1
closed-fixed
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 - 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

     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks