/run/unbound.pid chowned to unbound:unbound

[ChibaPet]

Under Devuan or Debian-with-sysvinit, unbound chowns its pidfile to the
daemon:

ifdef HAVE_CHOWN

                    if(chown(daemon->pidfile, cfg_uid, cfg_gid) == -1) {
                            verbose(VERB_QUERY, "cannot chown %u.%u %s: %s",
                                    (unsigned)cfg_uid, (unsigned)cfg_gid,
                                    daemon->pidfile, strerror(errno));
                    }

endif /* HAVE_CHOWN */

This is problematic, though. From start-stop-daemon's man page:

   -p, --pidfile pid-file
          Check whether a process has created the file pid-file.

          Note:  using  this  matching option alone might cause unintended
          processes to be acted on, if the old process terminated  without
          being able to remove the pid-file.

          Warning:  using  this match option with a world-writable pidfile
          or using it alone with a daemon that writes the  pidfile  as  an
          unprivileged  (non-root)  user  will  be  refused  with an error
          (since version 1.19.3) as  this  is  a  security  risk,  because
          either  any  user  can  write  to  it,  or  if  the  daemon gets
          compromised, the contents of the pidfile cannot be trusted,  and
          then  a  privileged  runner  (such as an init script executed as
          root)  would  end  up  acting  on  any  system  process.   Using
          /dev/null is exempt from these checks.

And sure enough, the shipped init script doesn't actually stop the daemon,
and restarts don't populate the pid file but do leave multiple daemons
running.

It's unclear to me why the pidfile would ever want to be chown'd. I see in
the changelogs:

26 January 2016: Wouter
- Fix #734: chown the pidfile if it resides inside the chroot.

...but I'm not sure where that buglist is to gather context. Seems like a
pidfile should explicitly not be inside a chroot under most circumstances,
unless the daemon is trying to relaunch itself chrooted...?

Either not chowning at all or having a runtime knob to control this
chowning would be useful fixes. Thoughts?

[gthess]

Chowing the pidfile is desirable for some system admins. When unbound exits (e.g., SIGTERM, unbound-control stop), it is able to clean the pidfile. That was the reason this code was introduced.

I understand that in your situation this is not desirable.
As this relies heavily on how a sysadmin would want unbound to behave based on the system unbound is installed on, I find that a configure time option would suffice. I don't see a reason for a runtime option, at least from my perspective.

Does that sound right?

[ChibaPet]

It's notable that unbound already has precedent set for some pidfile handling modification via runtime flags.

Anyway, it might be worth considering things like CVE-2020-14367:

https://seclists.org/oss-sec/2020/q3/130

I don't have the source in front of me now, but does unbound write its pidfile before or after it drops root?

[ChibaPet]

Just looked at the source, and:

    /* write new pidfile (while still root, so can be outside chroot) */

I suspect this is CVE-worthy, if you guys want to file for one.

[ret2libc]

I suspect this is CVE-worthy, if you guys want to file for one.

I'm not sure this is CVE-worthy for unbound. From its point of view, the PIDFile will be used by unbound user itself, so even in the worst case that someone was somehow able to corrupt the PIDFile, you could only kill a process owned by the unbound user (operation that could be done even without the PIDFile, if you assume an attacker has taken control of an unbound process).

The problem arises when there is crossing of a privileged boundary, e.g. if an init-like program with root privileges tries to use just that file to determine which PID to kill. There was a similar issue in systemd some time ago (CVE-2018-16888), but that was about indeed the init-like program, not the particular program itself. Systemd solved the problem by checking additional things before killing the process. It seems start-stop-daemon already provides that feature, and judging by the man page that says ".. or using it alone with a daemon that writes the pidfile as an unprivileged (non-root)..." you could provide --pidfile with e.g. --name and --user, to ensure that the killed process has the right name and user. What do you think @ChibaPet ?

[ChibaPet]

I think:

1. I'm not familiar with the range of service mechanisms out there, and
   don't feel comfortable making assumptions about guards they put in place.

2. When I was reading the code, it seemed like the PID was written by
   unbound before it dropped root privs. I can't see how this is any different
   from the symlink attack described this year in CVE-2020-14367. The attack
   is described here.

   https://seclists.org/oss-sec/2020/q3/130

Here's an excerpt from your code:

    /* write new pidfile (while still root, so can be outside chroot) */

#ifdef HAVE_KILL
if(cfg->pidfile && cfg->pidfile[0] && need_pidfile) {
writepid(daemon->pidfile, getpid());
if(cfg->username && cfg->username[0] && cfg_uid != (uid_t)-1 &&
pidinchroot) {

Note that you're not removing the existing PID file before writing a new
one, hence are vulnerable to the same issue, completely independent of the
init system used. If that made this CVE-worthy for chrony, it makes it
CVE-worthy for unbound, unless I've missed something.

[ret2libc]

Sorry, since you were talking about start-stop-daemon and Debian-with-sysvinit in your first message I was under the impression the issue was somehow related to PIDFile and the init program. However, for CVE-2020-14367 it doesn't really matter it's actually the PIDFile, it could have just been any other file as the content does not matter, but it matters that it may overwrite files owned by root.

That indeed may be a problem for unbound as well. By the way, it's not my code as I'm from Red Hat :)

[ChibaPet]

Ah, apologies for the confusion. Their code, then. (Red Hat here too.)

[gthess]

Hi @ChibaPet, sorry for the pause on the discussion but I was unavailable the last couple of weeks.

At first I though you wanted an option to specifically stop chowning the file in the chroot but it seems you are more concerned
about the security implications. You have a point about the symlink attack but let me explain what is happening wrt chowning.

There are three configurable outcomes in relation to chowning the pid file:
A. chroot is used and pidfile in chroot -> chown (default; secure)
B. chroot is used and pidfile outside of chroot -> no chown (secure)
C. chroot is not used -> chown (vulnerable)

A is the default configuration. Even if unbound is compromised the user will not be able to symlink to files outside of the chroot.

B is what I expect most init systems to do as the pidfile is usually created in a well known directory (if at all).

C might decrease the hardening gained by dropping to the Unbound user privileges, but at this point unbound is configured outside of a security context (chroot).

For the A and C cases the chown is there because people wanted unbound to be able to clean the pidfile itself.

As you've noticed before and as you see now unbound can be run with various configurations wrt userid, pidfile, chroot and subsequently chown.

I find this different from the chrony case, as the chown was the default and only behavior of the program.

As the default (and safe; when using chroot) configuration is not vulnerable I don't believe this is CVE-worthy. However we would like to harden that part of the code similar to the chrony fix. We are currently in the process of releasing the next version, but we can look at this right after that.

Does that cover your question? Do you still believe this is CVE-worthy?

[ret2libc]

As the default (and safe; when using chroot) configuration is not vulnerable I don't believe this is CVE-worthy.

Please consider that at least on Fedora/RHEL Unbound has a default configuration with chroot: "", which means chroot is not enabled. I'm not sure about other distributions.

Also, my understanding is that it doesn't really matter whether you chown or not. The code in unbound.c first writes the pid (with writepid) then it chown it, if necessary, then it chroot in the right directory and at last it drop privileges. The issue is in the very first step, the writing of the pid file, which is done as root and outside of the chroot. If the unbound user has the privilege to write files in the directory where the PIDFile will be placed, he can put there a symlink to a file owned by root. During the writepid function, unbound still has root privileges and access to the full file system, so it can write the PID on whatever file.

Even considering the case A, where there is chroot and an attacker is able to compromise unbound, he can create a symlink to ../../../../../ and then trigger a unbound restart or something (e.g. crash unbound so systemd will restart it). During the next start, while writing the pidfile unbound will follow the symlink and write the PID in the wrong file, because as said above writepid is executed before dropping privileges and before chrooting.

I agree this would still require the attacker a relevant amount of privilege, but still it would cross an important privilege boundary which the root one. I hope I have not missed any important step in the code.

[ChibaPet]

Considering the code:

#ifdef HAVE_KILL
        /* true if pidfile is inside chrootdir, or nochroot */
        pidinchroot = need_pidfile && (!(cfg->chrootdir && cfg->chrootdir[0]) ||
                                (cfg->chrootdir && cfg->chrootdir[0] &&
                                strncmp(cfg->pidfile, cfg->chrootdir,
                                strlen(cfg->chrootdir))==0));

        /* check old pid file before forking */
        if(cfg->pidfile && cfg->pidfile[0] && need_pidfile) {
                /* calculate position of pidfile */
                if(cfg->pidfile[0] == '/')
                        daemon->pidfile = strdup(cfg->pidfile);
                else    daemon->pidfile = fname_after_chroot(cfg->pidfile, 
                                cfg, 1);
                if(!daemon->pidfile)
                        fatal_exit("pidfile alloc: out of memory");
                checkoldpid(daemon->pidfile, pidinchroot);
        }
#endif

        /* daemonize because pid is needed by the writepid func */
        if(!debug_mode && cfg->do_daemonize) {
                detach();
        }

        /* write new pidfile (while still root, so can be outside chroot) */
#ifdef HAVE_KILL
        if(cfg->pidfile && cfg->pidfile[0] && need_pidfile) {
                writepid(daemon->pidfile, getpid());
                if(cfg->username && cfg->username[0] && cfg_uid != (uid_t)-1 &&
                        pidinchroot) {

I might be missing some subtlety, but it seems like you're calling
writepid() a line before you check pidinchroot, near the bottom there.

[gthess]

Hi @ChibaPet, @ret2libc

You are absolutely right. I was approaching the problem from a "taking over a running unbound process" perspective (e.g., remote code execution) where the process is bounded by the chroot.

If a malicious user manages to take control of the 'unbound' user and creates said symlink, then unbound itself will indeed facilitate the symlink attack in a future (re)start.

Although this is a serious issue and we would like to harden unbound against it, we don't deem it as CVE-worthy for unbound. We normally request CVEs for specific unbound vulnerabilities and these mainly include over-the-wire direct exploits for the running unbound instance.

As this issue depends more on the local system side we are not eager to follow the CVE route. This does not mean however that we are trying to downplay the issue, as it will be properly highlighted via our common release channels and digital outreach when we release the fix.

I hope this addresses this issue (with a fix coming right after the current release cycle).

We are happy to hear your thoughts on this.

[ChibaPet]

I guess my question for you is, how does this differ from CVE-2020-14367? If it's the same, and if it's CVE-worthy for Chrony, how is it not for Unbound?

[ret2libc]

@gthess about the fix for the issue, https://git.tuxfamily.org/chrony/chrony.git/commit/util.c?id=7a4c396bba8f92a3ee8018620983529152050c74 and https://git.tuxfamily.org/chrony/chrony.git/commit/main.c?id=e18903a6b56341481a2e08469c0602010bf7bfe3 seem to be commits for chrony issue. However, I would suggest to use open with O_PATH and O_NOFOLLOW, check uid/gid with fstat and write the PID in that file only if it belongs to the unbound user/group and it is not a link. Also, use fchownat() instead of chown on the file name.

[gthess]

@ChibaPet As this is not a direct and/or remote vulnerability to unbound, if we were to discover the issue ourselves or the chrony CVE wouldn't exist we wouldn't go the CVE route. If you still believe that this should be a CVE we are happy to follow that route, although not with a targeted security release for the issue.

@ret2libc thanks for the suggestions. Indeed going from fopen to open with the correct flags is also our approach to the issue.
However I am wary about the O_PATH and O_NOFOLLOW flags as they are not guaranteed to be present on all systems (the same for fchownat()).

I was thinking of solving the issue with a different approach: create a random temp file (with O_CREAT and O_EXCL) and rename() it to the actual pid file. That would replace any potential symlink and seems more portable to me.

[ret2libc]

@ChibaPet As this is not a direct and/or remote vulnerability to unbound, if we were to discover the issue ourselves or the chrony CVE wouldn't exist we wouldn't go the CVE route. If you still believe that this should be a CVE we are happy to follow that route, although not with a targeted security release for the issue.

@ChibaPet If you want to go the CVE route, I think at this point the best thing would be to request the CVE directly to MITRE (unless @gthess you don't usually get CVEs from someone else, maybe GitHub). From my point of view it is reasonable to get a CVE as there is indeed privileged boundary crossing, even though the flaw is probably of low impact as it requires "high privileges" (access to the unbound user somehow).

@ret2libc thanks for the suggestions. Indeed going from fopen to open with the correct flags is also our approach to the issue.
However I am wary about the O_PATH and O_NOFOLLOW flags as they are not guaranteed to be present on all systems (the same for fchownat()).

I was thinking of solving the issue with a different approach: create a random temp file (with O_CREAT and O_EXCL) and rename() it to the actual pid file. That would replace any potential symlink and seems more portable to me.

I guess that should work as well. Maybe also add a note somewhere in doc or in some example about making sure the directory where the pidfile is written to cannot be removed/modified by the unbound user, otherwise you could still have links followed in the mid-components of the destination path during rename.

[gthess]

For the CVE request, if we go that route, we are currently in the process of becoming a CNA ourselves.
We should be able to issue it ourselves prior to the next release.

[ret2libc]

Just to know what to expect/do: @ChibaPet did you request a CVE? Do you intend to do so? @gthess did you receive anything?

[ChibaPet]

Hi, sorry for my having been quiet. I haven't requested one myself. I'm more an outside observer than a stakeholder, and I thought @gthess 's comment from October 14th might indicate their interest in doing this themselves. It seems like it'd be appropriate for a CVE to exist certainly.

[gthess]

Hi both, we will indeed go for a CVE for this.
We recently became a CNA so we will handle the procedure ourselves. The CVE and fix are scheduled for the upcoming release.

[ChibaPet]

That's great news. Thank you.

[ret2libc]

CVE-2020-28935 seems to have been assigned to this issue, right @gthess ? Thanks.

[gthess]

Correct. I will update the CVE record close to the release.

[gthess]

This issue was resolved in 1.13.0; closing.