Thread: Not quite a security hole in internal_in

I noticed the following core-dump situation in CVS HEAD:

regression=# select array_agg_finalfn(null);
server closed the connection unexpectedly       This probably means the server terminated abnormally       before or
whileprocessing the request.
 
The connection to the server was lost. Attempting reset: Failed.

(You won't see a crash if you don't have Asserts on.)  The proximate
cause of this is that array_agg_finalfn is being a bit overoptimistic
about what it can Assert:
/* cannot be called directly because of internal-type argument */Assert(fcinfo->context &&       (IsA(fcinfo->context,
AggState)||        IsA(fcinfo->context, WindowAggState)));
 
if (PG_ARGISNULL(0))    PG_RETURN_NULL();   /* returns null iff no input values */

We should switch the order of the null-test and the Assert.  However,
this brings up the question of exactly why the assumption embedded
in that comment is wrong.  You're not supposed to be able to call
internal-accepting functions from SQL, and yet here I did so.

The reason I could get past the type-safety check is that internal_in,
which normally throws an error if one tries to create a constant of type
internal, is marked STRICT in pg_proc, and so it doesn't get called for
nulls.

This would be a serious security problem if it weren't for the fact that
nearly all internal-accepting functions in the backend are also marked
STRICT, and so they won't get called in this type of scenario.  A query
to pg_proc shows that the only ones that aren't strict are

regression=# select oid::regprocedure from pg_proc where 'internal'::regtype = any (proargtypes) and not proisstrict;
             oid                   
 

----------------------------------------array_agg_transfn(internal,anyelement)array_agg_finalfn(internal)domain_recv(internal,oid,integer)
(3 rows)

The first two are new in 8.4, and the third has adequate defenses
already.  So we don't have a security hole in any released version
right now.

However, this is obviously something that could bite us in the future.
What I think we should do about it is mark internal_in as nonstrict,
so that it gets called and can throw error for a null.  Probably the
same for all the other pseudotypes in pseudotypes.c, although internal
is the only one that we consider to be a security-critical datatype.

Normally we would consider a pg_proc change as requiring a catversion
bump.  Since we are already past 8.4 beta we couldn't do that without
forcing an initdb for beta testers.  What I'd like to do about this
is change the proisstrict settings in pg_proc.h but not bump catversion.
This will ensure the fix is in place and protecting future coding,
although possibly not getting enforced in 8.4 production instances that
were upgraded from beta (if there are any such).

Comments?
        regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160


> Normally we would consider a pg_proc change as requiring a catversion
> bump.  Since we are already past 8.4 beta we couldn't do that without
> forcing an initdb for beta testers.

I think a serious issue like this warrants a bump. It seems like you are
saying that at any other time in the release cycle this would be
an automatic bump, so let's keep a consistent policy and bump it.

- --
Greg Sabino Mullane greg@turnstep.com
End Point Corporation
PGP Key: 0x14964AC8 200906091241
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkoukLkACgkQvJuQZxSWSshalACg8UfcyvTF2TxazvwwzxDNDIuM
dpEAoJYVaS8czeR79dyJOTAoXLghSgKS
=21ax
-----END PGP SIGNATURE-----

On Tue, Jun 9, 2009 at 11:31 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
>
> Normally we would consider a pg_proc change as requiring a catversion
> bump.  Since we are already past 8.4 beta we couldn't do that without
> forcing an initdb for beta testers.  What I'd like to do about this
> is change the proisstrict settings in pg_proc.h but not bump catversion.
> This will ensure the fix is in place and protecting future coding,
> although possibly not getting enforced in 8.4 production instances that
> were upgraded from beta (if there are any such).
>

why not bump it just at the final release. i don't think beta testers
are on production so they still have to initdb production servers
anyway

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Jaime Casanova <jcasanov@systemguards.com.ec> writes:
> why not bump it just at the final release.

There aren't going to be any more betas, so it's now or not at all.
I don't think we want to plan a catversion bump between RC and final.
        regards, tom lane

Tom Lane wrote:
> Normally we would consider a pg_proc change as requiring a catversion
> bump.  Since we are already past 8.4 beta we couldn't do that without
> forcing an initdb for beta testers.  What I'd like to do about this
> is change the proisstrict settings in pg_proc.h but not bump catversion.
> This will ensure the fix is in place and protecting future coding,
> although possibly not getting enforced in 8.4 production instances that
> were upgraded from beta (if there are any such).
>
>
>   

How common is this scenario? It's certainly not something I ever do.

cheers

andrew

On Tue, Jun 9, 2009 at 12:41 PM, Greg Sabino Mullane<greg@turnstep.com> wrote:
>> Normally we would consider a pg_proc change as requiring a catversion
>> bump.  Since we are already past 8.4 beta we couldn't do that without
>> forcing an initdb for beta testers.
>
> I think a serious issue like this warrants a bump. It seems like you are
> saying that at any other time in the release cycle this would be
> an automatic bump, so let's keep a consistent policy and bump it.

I agree.  We don't want people who are running beta2 to think that
nothing has changed when that's actually not the case.  If someone is
really inconvenienced by it and wants to ignore this problem, they can
find a way to bypass the check.  I suspect there probably aren't very
many such people, though.

...Robert

Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> This will ensure the fix is in place and protecting future coding,
>> although possibly not getting enforced in 8.4 production instances that
>> were upgraded from beta (if there are any such).

> How common is this scenario? It's certainly not something I ever do.

I would agree that it should be pretty darn rare.  But even so, this
is not a fix for an immediate bug but just safety against possible
future bugs.  So even if there is somebody out there who manages to miss
having the fix, I think they are not at serious risk.
        regards, tom lane

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> Can we hold it till 8.4.1? Or is that not an option?

What advantage would that have?  We certainly wouldn't wish to put a
catversion change into 8.4.1.
        regards, tom lane

"Greg Sabino Mullane" <greg@turnstep.com> writes:
>> Normally we would consider a pg_proc change as requiring a catversion
>> bump.  Since we are already past 8.4 beta we couldn't do that without
>> forcing an initdb for beta testers.

> I think a serious issue like this warrants a bump. It seems like you are
> saying that at any other time in the release cycle this would be
> an automatic bump, so let's keep a consistent policy and bump it.

This type of argument comes up all the time during beta period, and
we have made the decision both ways in the past.  There isn't a
"consistent policy" about it, it's case-by-case.

The reason we bump catversion during development cycles is to keep
developers from wasting their time chasing imaginary bugs when their
backend executable is subtly incompatible with the contents of their
databases.  (As happened more than a few times, before we invented
catversion :-(.)  The bump is "automatic" only because it's cheaper to
just do it than to think hard about whether you've created such a risk.
This change doesn't create any compatibility issues of that sort, and
unlike in development, there is a real cost to a catversion bump ---
it will force an extra initdb on beta testers, who may have loaded
databases of considerable size.

For production releases, the argument to bump catversion is to be real
sure that all 8.4 (or whatever) installations have the same initial
catalog contents.  That argument does apply here, but since this is just
a protective change and not known to be needed to prevent any live bug,
I don't think it's worth complicating beta testers' lives for.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> This would be a serious security problem if it weren't for the fact that
> nearly all internal-accepting functions in the backend are also marked
> STRICT, and so they won't get called in this type of scenario.  A query
> to pg_proc shows that the only ones that aren't strict are
> 
> regression=# select oid::regprocedure from pg_proc where 'internal'::regtype = any (proargtypes) and not
proisstrict;
>                   oid                   
> ----------------------------------------
>  array_agg_transfn(internal,anyelement)
>  array_agg_finalfn(internal)
>  domain_recv(internal,oid,integer)
> (3 rows)
> 
> The first two are new in 8.4, and the third has adequate defenses
> already.  So we don't have a security hole in any released version
> right now.

How about contrib/ ? I have this in my test 8.3.7 database:
seb=> select oid::regprocedure from pg_proc where 'internal'::regtype = any (proargtypes) and not proisstrict;
                  oid
 

---------------------------------------------------------------domain_recv(internal,oid,integer)utils_pg.gtrgm_same(utils_pg.gtrgm,utils_pg.gtrgm,internal)utils_pg.gin_extract_trgm(text,internal)utils_pg.gin_extract_trgm(text,internal,internal)utils_pg.gin_trgm_consistent(internal,internal,text)utils_pg.ghstore_compress(internal)utils_pg.ghstore_decompress(internal)utils_pg.ghstore_picksplit(internal,internal)utils_pg.ghstore_union(internal,internal)utils_pg.ghstore_same(internal,internal,internal)utils_pg.ghstore_consistent(internal,internal,integer)utils_pg.gin_extract_hstore(internal,internal)utils_pg.gin_extract_hstore_query(internal,internal,smallint)utils_pg.gin_consistent_hstore(internal,smallint,internal)utils_pg.gtrgm_consistent(utils_pg.gtrgm,internal,integer)utils_pg.gtrgm_compress(internal)utils_pg.gtrgm_decompress(internal)utils_pg.gtrgm_picksplit(internal,internal)utils_pg.gtrgm_union(bytea,internal)
(19 rows)

-- 
Sergey Burladyan

Sergey Burladyan <eshkinkot@gmail.com> writes:
> How about contrib/ ? I have this in my test 8.3.7 database:

That stuff should all be marked strict ... on the whole I'm not sure
that contrib is null-safe anyway, independently of this particular
issue.  AFAIK no one's really gone through it.
        regards, tom lane

I wrote:
> Sergey Burladyan <eshkinkot@gmail.com> writes:
>> How about contrib/ ? I have this in my test 8.3.7 database:

> That stuff should all be marked strict ... on the whole I'm not sure
> that contrib is null-safe anyway, independently of this particular
> issue.  AFAIK no one's really gone through it.

So I just did that, and found one bit of sloppiness in pg_freespacemap,
plus a whole lot of GIST/GIN support functions that aren't marked strict
and probably should be.  Will fix.  This is actually a lot closer to
being right than I would have bet on before the exercise.
        regards, tom lane