Обсуждение: [HACKERS] Code quality issues in ICU patch
icu_to_uchar() and icu_from_uchar(), and perhaps other places, are
touchingly naive about integer overflow hazards in buffer size
calculations. I call particular attention to this bit in
icu_from_uchar():
len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, ucnv_getMaxCharSize(icu_converter));
The ICU man pages say that that macro is defined as
#define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) (((int32_t)(length)+10)*(int32_t)(maxCharSize))
which means that getting this to overflow (resulting in
probably-exploitable memory overruns) would be about as hard as taking
candy from a baby.
I also notice that the general approach to handling ICU-reported
error conditions is like
if (U_FAILURE(status)) ereport(ERROR, (errmsg("ucnv_fromUChars failed: %s",
u_errorName(status))));
This lacks an errcode() setting, which is contrary to project policy,
and the error message violates our message style guidelines.
I don't particularly feel like fixing these things myself, but
somebody needs to; the overflow issues in particular are stop-ship
security hazards.
regards, tom lane
On Fri, Jun 23, 2017 at 12:31:40PM -0400, Tom Lane wrote: > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are > touchingly naive about integer overflow hazards in buffer size > calculations. I call particular attention to this bit in > icu_from_uchar(): > > len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, ucnv_getMaxCharSize(icu_converter)); > > The ICU man pages say that that macro is defined as > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) (((int32_t)(length)+10)*(int32_t)(maxCharSize)) > > which means that getting this to overflow (resulting in > probably-exploitable memory overruns) would be about as hard as > taking candy from a baby. So it kicks off really loud and persistent alarms, and isn't as easy as you thought, even taking this into account? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On 6/23/17 12:31, Tom Lane wrote: > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are > touchingly naive about integer overflow hazards in buffer size > calculations. I call particular attention to this bit in > icu_from_uchar(): > > len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, ucnv_getMaxCharSize(icu_converter)); > > The ICU man pages say that that macro is defined as > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) (((int32_t)(length)+10)*(int32_t)(maxCharSize)) > > which means that getting this to overflow (resulting in > probably-exploitable memory overruns) would be about as hard as taking > candy from a baby. Here is a patch that should address this. (I don't think the overruns were exploitable. You'd just get a buffer overflow error from the ucnv_* function.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 6/23/17 12:31, Tom Lane wrote:
>> icu_to_uchar() and icu_from_uchar(), and perhaps other places, are
>> touchingly naive about integer overflow hazards in buffer size
>> calculations.
> Here is a patch that should address this.
Ah, I was about to suggest the same thing, but I was coming at it from
the standpoint of not requiring buffers several times larger than
necessary, which could in itself cause avoidable palloc failures.
I was going to suggest a small variant actually: run the conversion
function an extra time only if the string is long enough to make the
space consumption interesting, say
if (nbytes < 1024){ /* if it's short, feel free to waste a bit of space */ len_uchar = 2 * nbytes + 1; /* max
lengthper docs */}else{ /* calculate exact space needed */ status = U_ZERO_ERROR; len_uchar =
ucnv_toUChars(icu_converter,NULL, 0, buff, nbytes, &status); if (U_FAILURE(status) && status !=
U_BUFFER_OVERFLOW_ERROR) ...}*buff_uchar = palloc(len_uchar * sizeof(**buff_uchar));
In this way the extra cycles would seldom be paid in practice.
> (I don't think the overruns were exploitable. You'd just get a buffer
> overflow error from the ucnv_* function.)
Hm, good point. But we might still hit avoidable failures with strings
that are a significant fraction of 1GB.
regards, tom lane
On Sat, Jun 24, 2017 at 10:03:25AM -0400, Peter Eisentraut wrote: > On 6/23/17 12:31, Tom Lane wrote: > > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are > > touchingly naive about integer overflow hazards in buffer size > > calculations. I call particular attention to this bit in > > icu_from_uchar(): > > > > len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, ucnv_getMaxCharSize(icu_converter)); > > > > The ICU man pages say that that macro is defined as > > > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) (((int32_t)(length)+10)*(int32_t)(maxCharSize)) > > > > which means that getting this to overflow (resulting in > > probably-exploitable memory overruns) would be about as hard as taking > > candy from a baby. > > Here is a patch that should address this. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Peter, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On Sun, Jun 25, 2017 at 09:28:51PM -0700, Noah Misch wrote: > On Sat, Jun 24, 2017 at 10:03:25AM -0400, Peter Eisentraut wrote: > > On 6/23/17 12:31, Tom Lane wrote: > > > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are > > > touchingly naive about integer overflow hazards in buffer size > > > calculations. I call particular attention to this bit in > > > icu_from_uchar(): > > > > > > len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, ucnv_getMaxCharSize(icu_converter)); > > > > > > The ICU man pages say that that macro is defined as > > > > > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) (((int32_t)(length)+10)*(int32_t)(maxCharSize)) > > > > > > which means that getting this to overflow (resulting in > > > probably-exploitable memory overruns) would be about as hard as taking > > > candy from a baby. > > > > Here is a patch that should address this. > > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Peter, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. > > [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On 6/24/17 11:51, Tom Lane wrote: > Ah, I was about to suggest the same thing, but I was coming at it from > the standpoint of not requiring buffers several times larger than > necessary, which could in itself cause avoidable palloc failures. > > I was going to suggest a small variant actually: run the conversion > function an extra time only if the string is long enough to make the > space consumption interesting, say I had thought about something like that, too, but my concern is that we then have double the code paths to test. I have run some performance tests and I couldn't detect any differences between the variants. So unless someone has any other insights, I think I'll go with the proposed patch by tomorrow. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 6/30/17 08:13, Peter Eisentraut wrote: > On 6/24/17 11:51, Tom Lane wrote: >> Ah, I was about to suggest the same thing, but I was coming at it from >> the standpoint of not requiring buffers several times larger than >> necessary, which could in itself cause avoidable palloc failures. >> >> I was going to suggest a small variant actually: run the conversion >> function an extra time only if the string is long enough to make the >> space consumption interesting, say > > I had thought about something like that, too, but my concern is that we > then have double the code paths to test. I have run some performance > tests and I couldn't detect any differences between the variants. So > unless someone has any other insights, I think I'll go with the proposed > patch by tomorrow. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services