Skip to content

Commit b9b21ac

Browse files
tglsfdctglsfdc
committed
In extensions, don't replace objects not belonging to the extension.
Previously, if an extension script did CREATE OR REPLACE and there was an existing object not belonging to the extension, it would overwrite the object and adopt it into the extension. This is problematic, first because the overwrite is probably unintentional, and second because we didn't change the object's ownership. Thus a hostile user could create an object in advance of an expected CREATE EXTENSION command, and would then have ownership rights on an extension object, which could be modified for trojan-horse-type attacks. Hence, forbid CREATE OR REPLACE of an existing object unless it already belongs to the extension. (Note that we've always forbidden replacing an object that belongs to some other extension; only the behavior for previously-free-standing objects changes here.) For the same reason, also fail CREATE IF NOT EXISTS when there is an existing object that doesn't belong to the extension. Our thanks to Sven Klemm for reporting this problem. Security: CVE-2022-2625
1 parent 7e29a79 commit b9b21ac

21 files changed

+539
-52
lines changed

‎doc/src/sgml/extend.sgml

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1319,17 +1319,6 @@ SELECT * FROM pg_extension_update_paths('<replaceable>extension_name</replaceabl
13191319
trusted if it depends on another one, unless that other one is always
13201320
installed in <literal>pg_catalog</literal>.
13211321
</para>
1322-
1323-
<para>
1324-
Do <emphasis>not</emphasis> use <command>CREATE OR REPLACE
1325-
FUNCTION</command>, except in an update script that must change the
1326-
definition of a function that is known to be an extension member
1327-
already. (Likewise for other <literal>OR REPLACE</literal> options.)
1328-
Using <literal>OR REPLACE</literal> unnecessarily not only has a risk
1329-
of accidentally overwriting someone else's function, but it creates a
1330-
security hazard since the overwritten function would still be owned by
1331-
its original owner, who could modify it.
1332-
</para>
13331322
</sect3>
13341323
</sect2>
13351324

‎src/backend/catalog/pg_collation.c

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,25 @@ CollationCreate(const char *collname, Oid collnamespace,
7676
* friendlier error message. The unique index provides a backstop against
7777
* race conditions.
7878
*/
79-
if (SearchSysCacheExists3(COLLNAMEENCNSP,
80-
PointerGetDatum(collname),
81-
Int32GetDatum(collencoding),
82-
ObjectIdGetDatum(collnamespace)))
79+
oid = GetSysCacheOid3(COLLNAMEENCNSP,
80+
Anum_pg_collation_oid,
81+
PointerGetDatum(collname),
82+
Int32GetDatum(collencoding),
83+
ObjectIdGetDatum(collnamespace));
84+
if (OidIsValid(oid))
8385
{
8486
if (quiet)
8587
return InvalidOid;
8688
else if (if_not_exists)
8789
{
90+
/*
91+
* If we are in an extension script, insist that the pre-existing
92+
* object be a member of the extension, to avoid security risks.
93+
*/
94+
ObjectAddressSet(myself, CollationRelationId, oid);
95+
checkMembershipInCurrentExtension(&myself);
96+
97+
/* OK to skip */
8898
ereport(NOTICE,
8999
(errcode(ERRCODE_DUPLICATE_OBJECT),
90100
collencoding == -1
@@ -114,16 +124,19 @@ CollationCreate(const char *collname, Oid collnamespace,
114124
* so we take a ShareRowExclusiveLock earlier, to protect against
115125
* concurrent changes fooling this check.
116126
*/
117-
if ((collencoding == -1 &&
118-
SearchSysCacheExists3(COLLNAMEENCNSP,
119-
PointerGetDatum(collname),
120-
Int32GetDatum(GetDatabaseEncoding()),
121-
ObjectIdGetDatum(collnamespace))) ||
122-
(collencoding != -1 &&
123-
SearchSysCacheExists3(COLLNAMEENCNSP,
124-
PointerGetDatum(collname),
125-
Int32GetDatum(-1),
126-
ObjectIdGetDatum(collnamespace))))
127+
if (collencoding == -1)
128+
oid = GetSysCacheOid3(COLLNAMEENCNSP,
129+
Anum_pg_collation_oid,
130+
PointerGetDatum(collname),
131+
Int32GetDatum(GetDatabaseEncoding()),
132+
ObjectIdGetDatum(collnamespace));
133+
else
134+
oid = GetSysCacheOid3(COLLNAMEENCNSP,
135+
Anum_pg_collation_oid,
136+
PointerGetDatum(collname),
137+
Int32GetDatum(-1),
138+
ObjectIdGetDatum(collnamespace));
139+
if (OidIsValid(oid))
127140
{
128141
if (quiet)
129142
{
@@ -132,6 +145,14 @@ CollationCreate(const char *collname, Oid collnamespace,
132145
}
133146
else if (if_not_exists)
134147
{
148+
/*
149+
* If we are in an extension script, insist that the pre-existing
150+
* object be a member of the extension, to avoid security risks.
151+
*/
152+
ObjectAddressSet(myself, CollationRelationId, oid);
153+
checkMembershipInCurrentExtension(&myself);
154+
155+
/* OK to skip */
135156
table_close(rel, NoLock);
136157
ereport(NOTICE,
137158
(errcode(ERRCODE_DUPLICATE_OBJECT),

‎src/backend/catalog/pg_depend.c

Lines changed: 67 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -170,22 +170,23 @@ recordMultipleDependencies(const ObjectAddress *depender,
170170

171171
/*
172172
* If we are executing a CREATE EXTENSION operation, mark the given object
173-
* as being a member of the extension. Otherwise, do nothing.
173+
* as being a member of the extension, or check that it already is one.
174+
* Otherwise, do nothing.
174175
*
175176
* This must be called during creation of any user-definable object type
176177
* that could be a member of an extension.
177178
*
178-
* If isReplace is true, the object already existed (or might have already
179-
* existed), so we must check for a pre-existing extension membership entry.
180-
* Passing false is a guarantee that the object is newly created, and so
181-
* could not already be a member of any extension.
179+
* isReplace must be true if the object already existed, and false if it is
180+
* newly created. In the former case we insist that it already be a member
181+
* of the current extension. In the latter case we can skip checking whether
182+
* it is already a member of any extension.
182183
*
183184
* Note: isReplace = true is typically used when updating an object in
184-
* CREATE OR REPLACE and similar commands. The net effect is that if an
185-
* extension script uses such a command on a pre-existing free-standing
186-
* object, the object will be absorbed into the extension. If the object
187-
* is already a member of some other extension, the command will fail.
188-
* This behavior is desirable for cases such as replacing a shell type.
185+
* CREATE OR REPLACE and similar commands. We used to allow the target
186+
* object to not already be an extension member, instead silently absorbing
187+
* it into the current extension. However, this was both error-prone
188+
* (extensions might accidentally overwrite free-standing objects) and
189+
* a security hazard (since the object would retain its previous ownership).
189190
*/
190191
void
191192
recordDependencyOnCurrentExtension(const ObjectAddress *object,
@@ -203,6 +204,12 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
203204
{
204205
Oid oldext;
205206

207+
/*
208+
* Side note: these catalog lookups are safe only because the
209+
* object is a pre-existing one. In the not-isReplace case, the
210+
* caller has most likely not yet done a CommandCounterIncrement
211+
* that would make the new object visible.
212+
*/
206213
oldext = getExtensionOfObject(object->classId, object->objectId);
207214
if (OidIsValid(oldext))
208215
{
@@ -216,6 +223,13 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
216223
getObjectDescription(object, false),
217224
get_extension_name(oldext))));
218225
}
226+
/* It's a free-standing object, so reject */
227+
ereport(ERROR,
228+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
229+
errmsg("%s is not a member of extension \"%s\"",
230+
getObjectDescription(object, false),
231+
get_extension_name(CurrentExtensionObject)),
232+
errdetail("An extension is not allowed to replace an object that it does not own.")));
219233
}
220234

221235
/* OK, record it as a member of CurrentExtensionObject */
@@ -227,6 +241,49 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
227241
}
228242
}
229243

244+
/*
245+
* If we are executing a CREATE EXTENSION operation, check that the given
246+
* object is a member of the extension, and throw an error if it isn't.
247+
* Otherwise, do nothing.
248+
*
249+
* This must be called whenever a CREATE IF NOT EXISTS operation (for an
250+
* object type that can be an extension member) has found that an object of
251+
* the desired name already exists. It is insecure for an extension to use
252+
* IF NOT EXISTS except when the conflicting object is already an extension
253+
* member; otherwise a hostile user could substitute an object with arbitrary
254+
* properties.
255+
*/
256+
void
257+
checkMembershipInCurrentExtension(const ObjectAddress *object)
258+
{
259+
/*
260+
* This is actually the same condition tested in
261+
* recordDependencyOnCurrentExtension; but we want to issue a
262+
* differently-worded error, and anyway it would be pretty confusing to
263+
* call recordDependencyOnCurrentExtension in these circumstances.
264+
*/
265+
266+
/* Only whole objects can be extension members */
267+
Assert(object->objectSubId == 0);
268+
269+
if (creating_extension)
270+
{
271+
Oid oldext;
272+
273+
oldext = getExtensionOfObject(object->classId, object->objectId);
274+
/* If already a member of this extension, OK */
275+
if (oldext == CurrentExtensionObject)
276+
return;
277+
/* Else complain */
278+
ereport(ERROR,
279+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
280+
errmsg("%s is not a member of extension \"%s\"",
281+
getObjectDescription(object, false),
282+
get_extension_name(CurrentExtensionObject)),
283+
errdetail("An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one that it already owns.")));
284+
}
285+
}
286+
230287
/*
231288
* deleteDependencyRecordsFor -- delete all records with given depender
232289
* classId/objectId. Returns the number of records deleted.

‎src/backend/catalog/pg_operator.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -864,7 +864,7 @@ makeOperatorDependencies(HeapTuple tuple,
864864

865865
/* Dependency on extension */
866866
if (makeExtensionDep)
867-
recordDependencyOnCurrentExtension(&myself, true);
867+
recordDependencyOnCurrentExtension(&myself, isUpdate);
868868

869869
return myself;
870870
}

‎src/backend/catalog/pg_type.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -546,8 +546,11 @@ TypeCreate(Oid newTypeOid,
546546
* rebuild should be true if this is a pre-existing type. We will remove
547547
* existing dependencies and rebuild them from scratch. This is needed for
548548
* ALTER TYPE, and also when replacing a shell type. We don't remove any
549-
* existing extension dependency, though (hence, if makeExtensionDep is also
550-
* true and the type belongs to some other extension, an error will occur).
549+
* existing extension dependency, though; hence, if makeExtensionDep is also
550+
* true and we're in an extension script, an error will occur unless the
551+
* type already belongs to the current extension. That's the behavior we
552+
* want when replacing a shell type, which is the only case where both flags
553+
* are true.
551554
*/
552555
void
553556
GenerateTypeDependencies(HeapTuple typeTuple,

‎src/backend/commands/createas.c

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -393,19 +393,31 @@ bool
393393
CreateTableAsRelExists(CreateTableAsStmt *ctas)
394394
{
395395
Oid nspid;
396+
Oid oldrelid;
397+
ObjectAddress address;
396398
IntoClause *into = ctas->into;
397399

398400
nspid = RangeVarGetCreationNamespace(into->rel);
399401

400-
if (get_relname_relid(into->rel->relname, nspid))
402+
oldrelid = get_relname_relid(into->rel->relname, nspid);
403+
if (OidIsValid(oldrelid))
401404
{
402405
if (!ctas->if_not_exists)
403406
ereport(ERROR,
404407
(errcode(ERRCODE_DUPLICATE_TABLE),
405408
errmsg("relation \"%s\" already exists",
406409
into->rel->relname)));
407410

408-
/* The relation exists and IF NOT EXISTS has been specified */
411+
/*
412+
* The relation exists and IF NOT EXISTS has been specified.
413+
*
414+
* If we are in an extension script, insist that the pre-existing
415+
* object be a member of the extension, to avoid security risks.
416+
*/
417+
ObjectAddressSet(address, RelationRelationId, oldrelid);
418+
checkMembershipInCurrentExtension(&address);
419+
420+
/* OK to skip */
409421
ereport(NOTICE,
410422
(errcode(ERRCODE_DUPLICATE_TABLE),
411423
errmsg("relation \"%s\" already exists, skipping",

‎src/backend/commands/foreigncmds.c

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -855,13 +855,22 @@ CreateForeignServer(CreateForeignServerStmt *stmt)
855855
ownerId = GetUserId();
856856

857857
/*
858-
* Check that there is no other foreign server by this name. Do nothing if
859-
* IF NOT EXISTS was enforced.
858+
* Check that there is no other foreign server by this name. If there is
859+
* one, do nothing if IF NOT EXISTS was specified.
860860
*/
861-
if (GetForeignServerByName(stmt->servername, true) != NULL)
861+
srvId = get_foreign_server_oid(stmt->servername, true);
862+
if (OidIsValid(srvId))
862863
{
863864
if (stmt->if_not_exists)
864865
{
866+
/*
867+
* If we are in an extension script, insist that the pre-existing
868+
* object be a member of the extension, to avoid security risks.
869+
*/
870+
ObjectAddressSet(myself, ForeignServerRelationId, srvId);
871+
checkMembershipInCurrentExtension(&myself);
872+
873+
/* OK to skip */
865874
ereport(NOTICE,
866875
(errcode(ERRCODE_DUPLICATE_OBJECT),
867876
errmsg("server \"%s\" already exists, skipping",
@@ -1126,6 +1135,10 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
11261135
{
11271136
if (stmt->if_not_exists)
11281137
{
1138+
/*
1139+
* Since user mappings aren't members of extensions (see comments
1140+
* below), no need for checkMembershipInCurrentExtension here.
1141+
*/
11291142
ereport(NOTICE,
11301143
(errcode(ERRCODE_DUPLICATE_OBJECT),
11311144
errmsg("user mapping for \"%s\" already exists for server \"%s\", skipping",

‎src/backend/commands/schemacmds.c

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -112,14 +112,25 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString,
112112
* the permissions checks, but since CREATE TABLE IF NOT EXISTS makes its
113113
* creation-permission check first, we do likewise.
114114
*/
115-
if (stmt->if_not_exists &&
116-
SearchSysCacheExists1(NAMESPACENAME, PointerGetDatum(schemaName)))
115+
if (stmt->if_not_exists)
117116
{
118-
ereport(NOTICE,
119-
(errcode(ERRCODE_DUPLICATE_SCHEMA),
120-
errmsg("schema \"%s\" already exists, skipping",
121-
schemaName)));
122-
return InvalidOid;
117+
namespaceId = get_namespace_oid(schemaName, true);
118+
if (OidIsValid(namespaceId))
119+
{
120+
/*
121+
* If we are in an extension script, insist that the pre-existing
122+
* object be a member of the extension, to avoid security risks.
123+
*/
124+
ObjectAddressSet(address, NamespaceRelationId, namespaceId);
125+
checkMembershipInCurrentExtension(&address);
126+
127+
/* OK to skip */
128+
ereport(NOTICE,
129+
(errcode(ERRCODE_DUPLICATE_SCHEMA),
130+
errmsg("schema \"%s\" already exists, skipping",
131+
schemaName)));
132+
return InvalidOid;
133+
}
123134
}
124135

125136
/*

‎src/backend/commands/sequence.c

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,14 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
145145
RangeVarGetAndCheckCreationNamespace(seq->sequence, NoLock, &seqoid);
146146
if (OidIsValid(seqoid))
147147
{
148+
/*
149+
* If we are in an extension script, insist that the pre-existing
150+
* object be a member of the extension, to avoid security risks.
151+
*/
152+
ObjectAddressSet(address, RelationRelationId, seqoid);
153+
checkMembershipInCurrentExtension(&address);
154+
155+
/* OK to skip */
148156
ereport(NOTICE,
149157
(errcode(ERRCODE_DUPLICATE_TABLE),
150158
errmsg("relation \"%s\" already exists, skipping",

‎src/backend/commands/statscmds.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,10 @@ CreateStatistics(CreateStatsStmt *stmt)
181181
{
182182
if (stmt->if_not_exists)
183183
{
184+
/*
185+
* Since stats objects aren't members of extensions (see comments
186+
* below), no need for checkMembershipInCurrentExtension here.
187+
*/
184188
ereport(NOTICE,
185189
(errcode(ERRCODE_DUPLICATE_OBJECT),
186190
errmsg("statistics object \"%s\" already exists, skipping",

‎src/backend/commands/view.c

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
190190
CommandCounterIncrement();
191191

192192
/*
193-
* Finally update the view options.
193+
* Update the view's options.
194194
*
195195
* The new options list replaces the existing options list, even if
196196
* it's empty.
@@ -203,8 +203,22 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
203203
/* EventTriggerAlterTableStart called by ProcessUtilitySlow */
204204
AlterTableInternal(viewOid, atcmds, true);
205205

206+
/*
207+
* There is very little to do here to update the view's dependencies.
208+
* Most view-level dependency relationships, such as those on the
209+
* owner, schema, and associated composite type, aren't changing.
210+
* Because we don't allow changing type or collation of an existing
211+
* view column, those dependencies of the existing columns don't
212+
* change either, while the AT_AddColumnToView machinery took care of
213+
* adding such dependencies for new view columns. The dependencies of
214+
* the view's query could have changed arbitrarily, but that was dealt
215+
* with inside StoreViewQuery. What remains is only to check that
216+
* view replacement is allowed when we're creating an extension.
217+
*/
206218
ObjectAddressSet(address, RelationRelationId, viewOid);
207219

220+
recordDependencyOnCurrentExtension(&address, true);
221+
208222
/*
209223
* Seems okay, so return the OID of the pre-existing view.
210224
*/

0 commit comments

Comments
 (0)