Thread: Fwd: race in pg_ctl start -w
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
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> 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.
This makes the readfile function very usage-specific though. The fix
I was thinking about was to modify the second loop to force it to fall
out once the predetermined number of lines had been read.
Or maybe we should use just one loop with realloc, instead of reading
the file twice.
>> 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.
Yeah, I would prefer to keep the code the same in initdb and pg_ctl,
just because somebody is likely to copy one or the other in future.
The fixed-size-buffer solution does not work for initdb.
regards, tom lane
On 11.10.2012 20:29, Tom Lane wrote: > Heikki Linnakangas<hlinnaka@iki.fi> writes: >> 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. > > This makes the readfile function very usage-specific though. The fix > I was thinking about was to modify the second loop to force it to fall > out once the predetermined number of lines had been read. > > Or maybe we should use just one loop with realloc, instead of reading > the file twice. Hmm, starting with 9.3, postmaster can not only create and append to the end of file, it can also inject a line in the middle, shifting the following lines forwards. In theory, if a new line is injected into the middle of the file between fgets() calls, readfile() could read part of the same line twice. Not sure what consequences that could have; pg_ctl might try to connect to wrong address or socket directory. Although in practice, fgets() is buffered, and the buffer is probably large enough to hold the whole file, so it probably gets slurped into memory as one unit anyway. Then again, I don't think read/write on a file is guaranteed to be atomic either, so I guess there's always the theoretical possibility of a partial read. This makes me a bit uncomfortable with the 9.3 change that postmaster.pid file is no longer strictly append-only (commit c9b0cbe9). Could we delay appending the socket directory and listen address information to the file until we know both, and then append both in one call after that? Gah, how can a trivial thing like this be so complicated.. - Heikki
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> Hmm, starting with 9.3, postmaster can not only create and append to the
> end of file, it can also inject a line in the middle, shifting the
> following lines forwards. In theory, if a new line is injected into the
> middle of the file between fgets() calls, readfile() could read part of
> the same line twice. Not sure what consequences that could have; pg_ctl
> might try to connect to wrong address or socket directory.
Hm. IIRC, the postmaster is careful to write the whole thing in a
single write() call, which in principle is atomic. Perhaps you're
right that we'd better have pg_ctl read it in a single read() to
ensure that it sees a consistent file state. Otherwise we're making
assumptions about what sort of buffering underlies the stdio functions.
> Then again, I don't think read/write on a
> file is guaranteed to be atomic either, so I guess there's always the
> theoretical possibility of a partial read.
I think it is as long as the file is less than a bufferload.
> This makes me a bit uncomfortable with the 9.3 change that
> postmaster.pid file is no longer strictly append-only (commit c9b0cbe9).
> Could we delay appending the socket directory and listen address
> information to the file until we know both, and then append both in one
> call after that?
IIRC, there were compatibility reasons for doing it that way, so I'm
disinclined to change it.
regards, tom lane
On 11.10.2012 22:36, Tom Lane wrote: > Heikki Linnakangas<hlinnakangas@vmware.com> writes: >> Hmm, starting with 9.3, postmaster can not only create and append to the >> end of file, it can also inject a line in the middle, shifting the >> following lines forwards. In theory, if a new line is injected into the >> middle of the file between fgets() calls, readfile() could read part of >> the same line twice. Not sure what consequences that could have; pg_ctl >> might try to connect to wrong address or socket directory. > > Hm. IIRC, the postmaster is careful to write the whole thing in a > single write() call, which in principle is atomic. Perhaps you're > right that we'd better have pg_ctl read it in a single read() to > ensure that it sees a consistent file state. Otherwise we're making > assumptions about what sort of buffering underlies the stdio functions. Ok, changed it to slurp the whole file to memory with one read() call. Dave, did this silence the static analysis tool you used? - Heikki
On 10/15/2012 4:06 AM, Heikki Linnakangas wrote: > On 11.10.2012 22:36, Tom Lane wrote: >> Heikki Linnakangas<hlinnakangas@vmware.com> writes: >>> Hmm, starting with 9.3, postmaster can not only create and append to >>> the >>> end of file, it can also inject a line in the middle, shifting the >>> following lines forwards. In theory, if a new line is injected into the >>> middle of the file between fgets() calls, readfile() could read part of >>> the same line twice. Not sure what consequences that could have; pg_ctl >>> might try to connect to wrong address or socket directory. >> >> Hm. IIRC, the postmaster is careful to write the whole thing in a >> single write() call, which in principle is atomic. Perhaps you're >> right that we'd better have pg_ctl read it in a single read() to >> ensure that it sees a consistent file state. Otherwise we're making >> assumptions about what sort of buffering underlies the stdio functions. > > Ok, changed it to slurp the whole file to memory with one read() call. > > Dave, did this silence the static analysis tool you used? > > - Heikki Heikki, It's happy about the overruns. It did flag an issue where the file descriptor can leak when the various early returns get taken. - Dave
Dave Vitek wrote: > Heikki, >=20 > It's happy about the overruns. It did flag an issue where the file > descriptor can leak when the various early returns get taken. This is a common problem with static analysers; they don't realise we don't care about the leaked resource because the program is shortly going to terminate anyway. We (used to?) have plenty of false positives in initdb as reported in the Coverity scanner, for example. --=20 =C1lvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 18.10.2012 22:15, Alvaro Herrera wrote: > Dave Vitek wrote: > >> Heikki, >> >> It's happy about the overruns. It did flag an issue where the file >> descriptor can leak when the various early returns get taken. > > This is a common problem with static analysers; they don't realise we > don't care about the leaked resource because the program is shortly > going to terminate anyway. We (used to?) have plenty of false positives > in initdb as reported in the Coverity scanner, for example. Actually, this was a real leak. I should've put close() calls to the cases where the file is empty, or fstat() fails. It doesn't matter when the function is called only once, but in the "pg_ctl start -w" case it's called repeatedly to poll for postmaster startup. Fixed, and I also changed it to not return the last line if it doesn't end in a newline, per Tom's suggestion. - Heikki