Fwd: race in pg_ctl start -w - Mailing list pgsql-bugs

Forwarding this to pgsql-bugs, since this isn't a security issue, as
pg_ctl can only be called an admin. My replies inline.

> -------- Original Message --------
> Subject: [pgsql-security] race in pg_ctl start -w
> Date: Thu, 11 Oct 2012 12:39:02 -0400
> From: Dave Vitek <dvitek@grammatech.com>
> To: <security@postgresql.org>
>
> Hi,
>
> I don't really think this is a security issue since pg_ctl isn't really
> an appealing target, but in theory you could hijack the process if you
> got very very lucky.  Feel free to forward this to the bugs list if you
> decide it isn't sensitive.
>
> We recently ran into a crash when using pg_ctl to start the postgres
> daemon.  The command was:
> pg_ctl start -w -t 600 -D
>
> It was a segv inside the body of malloc and didn't want to repro.
>
> The good news is, we develop a static analysis tool called CodeSonar for
> detecting bugs like this.  It has been flagging the problem since 2009,
> but we've generally been ignoring reports in third-party code (there are
> a lot of them).  We analyze postgres on a regular basis because it is
> used by CS's web UI.

So, the problem is that there's a race condition in the readfile()
function that pg_ctl uses to read postmaster.pid. It does essentially this:

1. open postmaster.pid
2. Read through the file one byte at a time, and count newlines.
3. Allocate buffers to hold that many lines.
4. Rewind and read the file again, this time copying the lines to the
allocated buffers

The race condition is that if another process (postmaster) changes the
file between steps 2 and 4, so that there are now more lines, reading
the file again can overrun the buffers allocated. In particular that can
happen at postmaster startup, when postmaster initially creates the file
empty, and then does a write() to fill it in.

It seems very unlucky if you actually hit that race condition, but I
guess it's not impossible.

A straightforward fix would be to just allocate one large-enough buffer
to begin with, e.g 8k, and read the whole file in one go. I'll write up
a patch for that.

> It also flagged a very similar issue (looks like the code was copied &
> pasted) in initdb.c.  I haven't looked into whether that one is as
> likely to be subject to a race as the pg_ctl one, but it could be
> problematic as well.

I don't think it's a problem for initdb, because the files that it reads
should not change on the fly. Nevertheless, I guess we might as well add
a check there.

> Here are the bug reports:
> http://www.grammatech.com/products/codesonar/examples/pg_ctl_race.html
> http://www.grammatech.com/products/codesonar/examples/initdb_race.html
>
> I've just saved static HTML above, so none of the links, navigation
> features, or images will work.  The files are posted on the website
> because they seemed a bit big for sending to a list.  I haven't shared
> them with anyone else outside the company.
>
> - Dave

-. Heikki