Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 630824 - dev-db/postgresql: root privilege escalation via chown (race condition) in pkg_config
Summary: dev-db/postgresql: root privilege escalation via chown (race condition) in pk...
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Security
Classification: Unclassified
Component: Auditing (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Gentoo Security
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-12 17:15 UTC by Michael Orlitzky
Modified: 2020-05-21 22:52 UTC (History)
3 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Orlitzky gentoo-dev 2017-09-12 17:15:28 UTC
The pkg_config phase for the dev-db/postgresql ebuilds calls "chown -R" on the DATA_DIR after verifying that DATA_DIR is empty:

  if [ -n "$(ls -A ${DATA_DIR} 2> /dev/null)" ] ; then
      eerror "The given directory, '${DATA_DIR}', is not empty."
      eerror "Modify DATA_DIR to point to an empty directory."
      die "${DATA_DIR} is not empty."
  fi

  einfo "Creating the data directory ..."
  if [[ ${EUID} == 0 ]] ; then
      mkdir -p "${DATA_DIR}"
      chown -Rf postgres:postgres "${DATA_DIR}"
      chmod 0700 "${DATA_DIR}"
  fi

If the DATA_DIR is actually empty, then the "-R" is harmless but also pointless. So right away, one possible solution is to remove the "-R".

However, if the DATA_DIR is not empty, then the "postgres" user can exploit the "chown -R" call to become root. That's where the race condition comes in: there is a brief period of time between where "ls" is called and where "chown -R" is called in which the "postgres" user can place a hard link in DATA_DIR. If he manages to create the hard link between "ls" and "chown" calls, then "chown -R" will hit the hard link, and give ownership of the target to the "postgres" user. That in turn can be used to gain root.

Later on in the ebuild, "su postgres" is used to do some stuff. Maybe the DATA_DIR can be created (and chmod'ed) using "su postgres" to avoid the chown call?
Comment 1 Aaron W. Swenson gentoo-dev 2018-02-11 15:57:18 UTC
commit 850efe2a5700c2ba30f9e9860dd83143cf15da34 (HEAD -> master, origin/master, origin/HEAD)
Author: Aaron W. Swenson <titanofold@gentoo.org>
Date:   Sun Feb 11 10:54:10 2018 -0500

    dev-db/postgresql: Cleanup Old and Insecure Files

    Bug: https://bugs.gentoo.org/627462
    Bug: https://bugs.gentoo.org/636978
    Bug: https://bugs.gentoo.org/630824
    Bug: https://bugs.gentoo.org/603720
    Bug: https://bugs.gentoo.org/603716
    Package-Manager: Portage-2.3.19, Repoman-2.3.6

commit 3b3ec30d0b02920ec76eeef4db2a968c3a907d23
Author: Jeroen Roovers <jer@gentoo.org>
Date:   Sun Feb 11 13:09:46 2018 +0100

    dev-db/postgresql: Stable for HPPA too.

    Package-Manager: Portage-2.3.24, Repoman-2.3.6
    RepoMan-Options: --ignore-arches
Comment 2 Aaron W. Swenson gentoo-dev 2018-03-24 09:34:40 UTC
Eh, so I didn't really fix this. I don't know why I referenced it. So here's what I think I could do:

    if [[ ${EUID} == 0 ]] ; then
        mkdir -p "$(dirname ${DATA_DIR})"
        $tmpdir = `mktemp -d` || die "Couldn't make temporary directory"
        chown postgres:postgres $tmpdir
        mv -T $tmpdir "${DATA_DIR%/}" || die
    elsif 
        # other things
    fi

We don't care what happens to the parent director{y,ies}. We just care that they're there. mktemp will create a directory with a random name that belongs to root, and it'll be created with mode 0700. We can change the permissions on that tmpdir while it's there before trying to move it to the desired location.

If the DATA_DIR exists and is nonempty, mv will fail. If DATA_DIR is a link, mv will fail. If the DATA_DIR exists, is a directory, but empty, mv will succeed replacing the DATA_DIR with the one with the proper permissions.

The only problem is BSD doesn't support -T on mv.

So, that's as close as I've gotten to making sure we always win this race.
Comment 3 Michael Orlitzky gentoo-dev 2018-03-24 11:05:34 UTC
(In reply to Aaron W. Swenson from comment #2)
>         $tmpdir = `mktemp -d` || die "Couldn't make temporary directory"
>         chown postgres:postgres $tmpdir

Since you're already giving the postgres user a shell, you could "su -c ... postgres" here to avoid the chown.
Comment 4 Aaron W. Swenson gentoo-dev 2018-03-25 03:09:33 UTC
(In reply to Michael Orlitzky from comment #3)
> (In reply to Aaron W. Swenson from comment #2)
> >         $tmpdir = `mktemp -d` || die "Couldn't make temporary directory"
> >         chown postgres:postgres $tmpdir
> 
> Since you're already giving the postgres user a shell, you could "su -c ...
> postgres" here to avoid the chown.

Yes, I could. That would save a line, maybe.

And, I could probably do the mv without the -T and instead just give the -n(o clobber) option to it. If someone nefarious were to race the script and have swapped out $(dirname $DATA_DIR) or put a symlink at DATA_DIR, the worst that would happen is a directory is created inside that location.
Comment 5 Aaron W. Swenson gentoo-dev 2018-05-20 17:53:23 UTC
All right, I think this is the best I can do:

	ebegin "Creating the data directory"
	local tmpdir
	if [[ ${EUID} == 0 ]] ; then
		tmpdir=$(su postgres -c 'mktemp -d') \
			|| die "Couldn't make temporary directory"
		mkdir -p "$(dirname ${DATA_DIR})"
		mv -n "${tmpdir}" "${DATA_DIR%/}"
		if [[ -d ${tmpdir} ]] ; then
			eend 1
			rmdir "${tmpdir}"
			die "Couldn't move temporary directory to destination"
		else
			eend 0
		fi
	else
		mkdir -p "${DATA_DIR}"
		if [[ -d ${DATA_DIR} ]] ; then
			eend 0
		else
			eend 1
			die "Couldn't make DATA_DIR"
		fi
	fi

Only when the ebuild is running as root, do we try to make sure everything is exclusive to the postgres system user. We start by making a temporary directory as postgres so that the directory starts out owned by postgres and with permission 0700. We don't care about ownership about parent directories; we just care that they are present. Because "mv -n" pretty much always returns 0 even if it didn't move anything, we check if the temporary directory still exists and die if it does.

For the non-root case, everything is owned and run as that non-root user. We just need to be sure the directory was made.

When initdb is executed in the next step, initdb will ensure the DATA_DIR is empty. If non-empty, it bails.
Comment 6 Aaron W. Swenson gentoo-dev 2018-05-21 02:14:14 UTC
And, after digging through this a bit more this whole exercise may have been pointless.

initdb/PostgreSQL has it's own implementation of "mkdir -p" that creates/sets permissions and checks things at each step.

The real answer seems to be to just scrap this section altogether, and let initdb handle everything. In fact, it seems it's been redundant since 2003.

pg_mkdir_p:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/port/pgmkdirp.c;h=d943559760d8980a1582f0bee064c0bea52b5597;hb=HEAD

initdb:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/bin/initdb/initdb.c;h=ae22e7d9fb89db67d187aa2e43cfe5db42011d9d;hb=HEAD#l2759
Comment 7 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2020-04-03 23:16:27 UTC
Unrestricting and reassigning to security@ per bug #705894
Comment 8 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2020-04-03 23:18:39 UTC
unrestricting per bug 705894
Comment 9 Sam James archtester gentoo-dev Security 2020-05-21 22:46:34 UTC
(In reply to Aaron W. Swenson from comment #6)
> And, after digging through this a bit more this whole exercise may have been
> pointless.
> 
> initdb/PostgreSQL has it's own implementation of "mkdir -p" that
> creates/sets permissions and checks things at each step.
> 
> The real answer seems to be to just scrap this section altogether, and let
> initdb handle everything. In fact, it seems it's been redundant since 2003.
> 
> pg_mkdir_p:
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/port/
> pgmkdirp.c;h=d943559760d8980a1582f0bee064c0bea52b5597;hb=HEAD
> 
> initdb:
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/bin/initdb/
> initdb.c;h=ae22e7d9fb89db67d187aa2e43cfe5db42011d9d;hb=HEAD#l2759

Do you know when this was first fixed? Anyway, this is obsolete now.