Horton

Thread

Commits

  1. Tighten parsing of datetime input.

  2. Fix parsing of ISO-8601 interval fields with exponential notation.

  1. Tightening DecodeNumberField's parsing rules

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-27T18:38:19Z

    Evgeniy Gorbanev reported to the security list that he'd found
a case where timestamp_in triggered an undefined-behavior
sanitizer warning, due to trying to store a float value larger
than INT_MAX into an integer variable.  We concluded that there's
no real security issue there, it's just that the C standard
doesn't say what the result should be.  So I'm moving this
discussion to the public list, because it's also not entirely
clear what the fix should be.

Evgeniy's example case was

SELECT 'j 05 t a6.424e5-'::timestamp;

which of course is garbage and draws an "invalid input syntax"
error as-expected, though not till after DecodeNumberField
has tried to stuff ".424e5 * 1000000" into an integer.  However,
I found that there are closely related cases that don't draw
any syntax error, such as

regression=# SELECT timestamp with time zone 'J2452271 T X03456-08';
      timestamptz       
------------------------
 2001-12-27 00:34:56-08
(1 row)

regression=# SELECT timestamp with time zone 'J2452271 T X03456.001e6-08';
      timestamptz       
------------------------
 2001-12-27 00:51:36-08
(1 row)

The fundamental problem here, IMO, is that DecodeNumberField assumes
without checking that its input contains only digits and perhaps a
decimal point.  In this example though, it's given whatever remains
after stripping the run-on timezone spec, that is "X03456.001e6".
That triggers both the overflow problem with the bogus ".001e6"
fraction, and a totally inappropriate reading of "X0" as "hour zero".

What I think we ought to do about this is reject strings containing
non-digits, as in the attached patch.  However, there's no doubt that
that'd make timestamp_in reject some inputs it used to accept, and
we've generally tried to avoid that --- commit f0d0394e8 for instance
is a recent effort in the direction of not breaking backward
compatibility, and it adopted a position that we should not break
cases that have a sensible interpretation, even if they're surprising.
The difference here is that I don't think there's a sensible
interpretation; or if there is, it's certainly not what the code
does today.

So what I propose we do about this is to apply the attached to HEAD
and leave the back branches alone.  Right now, inputs like this are
"garbage in, garbage out" but don't cause any real problem (upsetting
a sanitizer check isn't of concern for production use).  So I'm not
seeing that there's a good argument for back-patching.

We could alternatively apply some more-limited fix; Evgeniy's
original recommendation was just to reject if the result of
strtod() was outside the expected 0..1 range.  But I'm finding
it hard to believe that that's a great idea.

Comments?

			regards, tom lane

  2. Re: Tightening DecodeNumberField's parsing rules

    Robert Haas <robertmhaas@gmail.com> — 2025-05-27T19:51:40Z

    On Tue, May 27, 2025 at 2:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So what I propose we do about this is to apply the attached to HEAD
> and leave the back branches alone.

+1. In most cases, we pride ourselves on carefully validating the
input we receive and people on this list have been known to disparage
other products for failing to do the same. But our validation of
timestamps is notably less strict. I think that's somewhat unavoidable
given that there are multiple date-time formats that somebody might
use, but I'm in favor of not being more lax than we have to be. If
some input can't be interpreted as anything sensible, we should reject
it rather than making up a fake value. However, I agree that it's best
not to do such tightening in the back-branches.

-- 
Robert Haas
EDB: http://www.enterprisedb.com