Обсуждение: 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
On Tue, Jun 9, 2009 at 11:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrew Dunstan <andrew@dunslane.net> writes:> Tom Lane wrote:I would agree that it should be pretty darn rare. But even so, this
>> 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.
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.
Can we hold it till 8.4.1? Or is that not an option?
Best regards,
--
Lets call it Postgres
EnterpriseDB http://www.enterprisedb.com
gurjeet[.singh]@EnterpriseDB.com
singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
Mail sent from my BlackLaptop device
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