Thread: Probably security hole in postgresql-7.4.1
I work at Coverity where we use static analysis to find bugs in
software. I ran a security checker over postgresql-7.4.1 and I think I
found a security hole. I'm not familiar with the postgres source, so
this report may be false. My interpretation of the code follows.
I'd appreciate your feedback,
Ken Ashcraft
In the code below, fld_size gets copied in from a user specified file.
It is passed as the 'needed' parameter to enlargeStringInfo(). If
needed is a very large positive value, the addition 'needed += str->len
+ 1;' could cause an overflow, making needed a negative number.
enlargeStringInfo() would quickly return, thinking that enough memory
was present. However, the call to CopyGetData() uses the large,
positive fld_size, resulting in a buffer overflow, assuming that the
user's file has enough data.
/home/kash/user-progs/postgres/postgresql-7.4.1/src/backend/commands/copy.c:2050:CopyReadBinaryAttribute: ERROR:TAINT:
2030:2050:Passingunbounded user value "fld_size" as arg 1 to function "enlargeStringInfo", which uses it unsafely in
model[SOURCE_MODEL=(lib,CopyGetInt32,user,taint)] [SINK_MODEL=(lib,enlargeStringInfo,user,trustingsink)] [BOUNDS= Upper
boundon line 2040] [PATH=]
bool *isnull)
{int32 fld_size;Datum result;
Start --->fld_size = CopyGetInt32();
... DELETED 14 lines ...
/* reset attribute_buf to empty, and load raw data in it */attribute_buf.len = 0;attribute_buf.data[0] =
'\0';attribute_buf.cursor= 0;
Error --->enlargeStringInfo(&attribute_buf, fld_size);
CopyGetData(attribute_buf.data, fld_size);if (CopyGetEof())
-------------------
enlargeStringInfo(StringInfo str, int needed)
{int newlen;
needed += str->len + 1; /* total space required now */if (needed <= str->maxlen) return;
/*got enough space already */
/* * We don't want to allocate just a little more space with each * append; for efficiency, double the buffer size each
timeit * overflows. Actually, we might need to more than double it if * 'needed' is big... */newlen = 2 *
str->maxlen;while(needed > newlen) newlen = 2 * newlen;
str->data = (char *) repalloc(str->data, newlen);
str->maxlen = newlen;
}
Ken Ashcraft <ken@coverity.com> writes:
> I work at Coverity where we use static analysis to find bugs in
> software. I ran a security checker over postgresql-7.4.1 and I think I
> found a security hole.
>
> In the code below, fld_size gets copied in from a user specified file.
> It is passed as the 'needed' parameter to enlargeStringInfo(). If
> needed is a very large positive value, the addition 'needed += str->len
> + 1;' could cause an overflow, making needed a negative number.
I've applied a patch that fixes this issue, as well as the related one
that enlargeStringInfo could go into an infinite loop.
Although the path of control you identify doesn't seem very threatening
(since one must already be superuser to execute COPY from a file), the
same sort of problem could be triggered by sending a malformed data
packet, thus opening up the problem to anyone who can get past the
initial postmaster authentication check. So this is more severe than we
first thought.
If you are looking to improve your checker, you might want to look into
why it only found this path for bad data, and not the path leading from
the client connection socket. Seems like it should've found that too.
Thanks for the report!
regards, tom lane
Tom Lane wrote: > Ken Ashcraft <ken@coverity.com> writes: > > I work at Coverity where we use static analysis to find bugs in > > software. I ran a security checker over postgresql-7.4.1 and I think I > > found a security hole. > > > > In the code below, fld_size gets copied in from a user specified file. > > It is passed as the 'needed' parameter to enlargeStringInfo(). If > > needed is a very large positive value, the addition 'needed += str->len > > + 1;' could cause an overflow, making needed a negative number. > > I've applied a patch that fixes this issue, as well as the related one > that enlargeStringInfo could go into an infinite loop. > > Although the path of control you identify doesn't seem very threatening > (since one must already be superuser to execute COPY from a file), the > same sort of problem could be triggered by sending a malformed data > packet, thus opening up the problem to anyone who can get past the > initial postmaster authentication check. So this is more severe than we > first thought. > > If you are looking to improve your checker, you might want to look into > why it only found this path for bad data, and not the path leading from > the client connection socket. Seems like it should've found that too. Should we be thinking about a 7.4.3? I think we are waiting for pg_autovacuum fixes though. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Should we be thinking about a 7.4.3?
I'm not panicking over this particular bug ... but it does seem like
we've accumulated enough fixes since 7.4.2 that it may be time to start
thinking about another dot-release. Maybe set a date towards the end of
the month?
regards, tom lane
Tom Lane wrote:
>Bruce Momjian <pgman@candle.pha.pa.us> writes:
>
>
>>Should we be thinking about a 7.4.3?
>>
>>
>
>I'm not panicking over this particular bug ... but it does seem like
>we've accumulated enough fixes since 7.4.2 that it may be time to start
>thinking about another dot-release. Maybe set a date towards the end of
>the month?
>
> regards, tom lane
>
>
Industry practices dictate that we do issue SOMETHING now. The bug is
now public, and can be exploited.
This does not necessarily have to be 7.4.3. We can issue 7.4.2.1,
containing only this fix, so that people who need to expose their
database are not left open to attacks.
Also, if we want greater flexibility in handling these cases in the
future, we should set up an invite-only list for reporting security
bugs, and advertise it on the web site as the place to report security
issues. Had this vulnerability been reported there, we could reasonably
hold on without releasing a fix until 7.4.3 was ready.
If you need help in that list, I have a lot of experience with code
security, but very little experience with the Postgresql code. Also, it
would be a good idea to invite all the distro-packagers to be on that list.
Shachar
--
Shachar Shemesh
Lingnu Open Source Consulting
http://www.lingnu.com/
Shachar Shemesh <psql@shemesh.biz> writes: > Also, if we want greater flexibility in handling these cases in the future, we > should set up an invite-only list for reporting security bugs, and advertise it > on the web site as the place to report security issues. Had this vulnerability > been reported there, we could reasonably hold on without releasing a fix until > 7.4.3 was ready. A lot of people would be unhappy with that approach. A) they don't know the people on the invite-only list and have no basis to trust them and B) Often when a white hat reports the problem the black hats have known about it for much longer already. -- greg
On Wed, May 12, 2004 at 10:46:00 +0300, Shachar Shemesh <psql@shemesh.biz> wrote: > Industry practices dictate that we do issue SOMETHING now. The bug is > now public, and can be exploited. The description of the problem indicates that it can only be exploited after you have authenticated to the database. Since people who can connect to a postgres database can already cause denial of service attacks, this problem isn't a huge deal. It makes breaches in other programs (web server process especially) worse and provides another way for authorized users to cause problems. A release should probably be made soon, as a way to advertise the problem so that people are aware of it and can take appropiate steps. I don't think that this problem warrants bypassing normal minor release proceedure.
Greg Stark <gsstark@mit.edu> writes:
> Shachar Shemesh <psql@shemesh.biz> writes:
>> Also, if we want greater flexibility in handling these cases in the future, we
>> should set up an invite-only list for reporting security bugs,
> A lot of people would be unhappy with that approach. A) they don't know the
> people on the invite-only list and have no basis to trust them and B) Often
> when a white hat reports the problem the black hats have known about it for
> much longer already.
Past procedure for sensitive bugs has been for people to send reports to
the core committee (pgsql-core at postgresql dot org). If you don't
trust us you probably shouldn't be using Postgres ;-)
As per other comments, I don't find this bug compelling enough to
justify an instant release. Also, the original reporter is still
running his analysis tool and has found some other things that might
be worth patching. (Again, nothing compelling yet... but ...)
I'm inclined to wait a bit longer and see if we can't include some
more fixes in 7.4.3.
regards, tom lane
Bruno Wolff III wrote:
>On Wed, May 12, 2004 at 10:46:00 +0300,
> Shachar Shemesh <psql@shemesh.biz> wrote:
>
>
>>Industry practices dictate that we do issue SOMETHING now. The bug is
>>now public, and can be exploited.
>>
>>
>
>The description of the problem indicates that it can only be exploited
>after you have authenticated to the database. Since people who can
>connect to a postgres database can already cause denial of service
>attacks, this problem isn't a huge deal.
>
My take on this is different. To me, a DoS is a nuisance, but an
arbitrary code execution vulnerability means information leak, and a
major escalation (from which further escalation may be possible).
> It makes breaches in other
>programs (web server process especially) worse and provides another
>way for authorized users to cause problems.
>
>
Not to mention being another chain.
>A release should probably be made soon, as a way to advertise the problem
>so that people are aware of it and can take appropiate steps. I don't think
>that this problem warrants bypassing normal minor release proceedure.
>
>
Ok. How about an official patch against 7.4.2 that fixes it, so that
packagers can make their own informed decision. Also, has anybody
checked what other versions are affected? Is 7.3? 7.2? Some people can't
afford to upgrade due to data inconsistancy.
For those reasons I suggested a seperate mailing list.
Shachar
--
Shachar Shemesh
Lingnu Open Source Consulting
http://www.lingnu.com/
Shachar Shemesh <psql@shemesh.biz> writes: > Ok. How about an official patch against 7.4.2 that fixes it, so that > packagers can make their own informed decision. The "official patch" is available to anyone who wants it from our CVS server. http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/lib/stringinfo.c.diff?r1=1.36&r2=1.36.4.1 BTW, all the principal packagers read this list and have doubtless made their informed decisions already ... > Also, has anybody checked what other versions are affected? Nothing before 7.4, at least by the known implications of this issue. Again, if we wait a while and let Ken keep running his analysis tool, he might turn up other stuff we need to fix. Maybe even stuff that needs a fix much worse than this does. >>>Industry practices dictate that we do issue SOMETHING now. The bug is >>>now public, and can be exploited. I frankly think that this discussion is emblematic of all the worst tendencies of the security community. Have you forgotten the fable about the boy who cried "wolf"? If you demand a Chinese fire drill for every issue that could conceivably be exploited, you'll soon find yourself unable to get peoples' attention for problems that are really serious. I repeat: in my estimation this is not a bug that needs a fix yesterday. AFAICS it would be very difficult to cause more than a nuisance DOS with it, and there are plenty of other ways for authenticated database users to cause those. regards, tom lane
On Wed, May 12, 2004 at 23:36:49 +0300, Shachar Shemesh <psql@shemesh.biz> wrote: > > My take on this is different. To me, a DoS is a nuisance, but an > arbitrary code execution vulnerability means information leak, and a > major escalation (from which further escalation may be possible). A DOS is generally more than a nuisance in production environments. In most cases you aren't going to be giving direct access to your DB to people that aren't fairly trusted. The exception may be some of the web hosting places that provide DB backed web pages. > Not to mention being another chain. This isn't very significant as you have to authenticate to the DB first to exploit it. That's a lot less of a problem than something directly accessible by anyone from the net such as a web server. > Ok. How about an official patch against 7.4.2 that fixes it, so that > packagers can make their own informed decision. Also, has anybody > checked what other versions are affected? Is 7.3? 7.2? Some people can't > afford to upgrade due to data inconsistancy. Hasn't it already been committed to 7.4 stable? If so, just grab an update from CVS. Something should probably be done about 7.2 and 7.3 if the same bug exists in those versions. Nobody should be running anything earlier than that.
Tom Lane wrote: >Shachar Shemesh <psql@shemesh.biz> writes: > > >>Also, has anybody checked what other versions are affected? >> >> > >Nothing before 7.4, at least by the known implications of this issue. >Again, if we wait a while and let Ken keep running his analysis tool, >he might turn up other stuff we need to fix. Maybe even stuff that >needs a fix much worse than this does. > > > and also >I frankly think that this discussion is emblematic of all the worst >tendencies of the security community. Have you forgotten the fable >about the boy who cried "wolf"? > > I totally agree. That's why I suggested preventing the automatic public disclosure for Ken's next bugs, as well as anyone else's. This way, if we do need a few extra days, we can have them while still limiting the window of exposure. >I repeat: in my estimation this is not a bug that needs a fix yesterday. >AFAICS it would be very difficult to cause more than a nuisance DOS with >it, and there are plenty of other ways for authenticated database users >to cause those. > > I'm sorry. Maybe it's spending too many years in the security industry (I've been Check Point's "oh my god we have a security problem" process manager for over two years). Maybe it's knowing how to actually exploit these problems. Maybe it's just seeing many of the good guys (OpenBSD's Theo included) fall flat on their faces after saying "This is a DoS only". In my book, a buffer overrun=arbitrary code execution. For a now famous example of a bug declared "non exploitable", followed by an exploit, see http://www.theinquirer.net/?article=4053. I have been on the mailing lists at the time. The problem was declared "unexploitable on i386" by some of the best known names in the security industry of the time. > regards, tom lane > > Please. I'm not saying "Release now". I'm saying "get a mechanism for smarter handling of future events". Shachar -- Shachar Shemesh Lingnu Open Source Consulting http://www.lingnu.com/
On Thu, May 13, 2004 at 00:54:19 +0300, Shachar Shemesh <psql@shemesh.biz> wrote: > > > I'm sorry. Maybe it's spending too many years in the security industry > (I've been Check Point's "oh my god we have a security problem" process > manager for over two years). Maybe it's knowing how to actually exploit > these problems. Maybe it's just seeing many of the good guys (OpenBSD's > Theo included) fall flat on their faces after saying "This is a DoS > only". In my book, a buffer overrun=arbitrary code execution. But it is still a local user exploit, not a remote user exploit. That makes a big difference in how the bug should be treated.
> Ken Ashcraft <ken@coverity.com> writes: >> I work at Coverity where we use static analysis to find bugs in >> software. I ran a security checker over postgresql-7.4.1 and I think I >> found a security hole. >> >> In the code below, fld_size gets copied in from a user specified file. >> It is passed as the 'needed' parameter to enlargeStringInfo(). If >> needed is a very large positive value, the addition 'needed += str->len >> + 1;' could cause an overflow, making needed a negative number. > > I've applied a patch that fixes this issue, as well as the related one > that enlargeStringInfo could go into an infinite loop. > > Although the path of control you identify doesn't seem very threatening > (since one must already be superuser to execute COPY from a file), the > same sort of problem could be triggered by sending a malformed data > packet, thus opening up the problem to anyone who can get past the > initial postmaster authentication check. So this is more severe than we > first thought. > Great. Thanks for the feedback. If it is serious, is an advisory in order? Ken
"Ken Ashcraft" <ken@coverity.com> writes:
>> ... thus opening up the problem to anyone who can get past the
>> initial postmaster authentication check. So this is more severe than we
>> first thought.
> Great. Thanks for the feedback. If it is serious, is an advisory in order?
No, we'll just push out the fix as part of the next update version
(though that may happen a little sooner than it would have otherwise).
Sensible people don't give direct database connections to untrustworthy
users in the first place, since there are so many ways you can cause
problems if you can issue random SQL commands ...
regards, tom lane