Skip to content

Commit 5721da7

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 9a8df33 commit 5721da7

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
@@ -78,15 +78,25 @@ CollationCreate(const char *collname, Oid collnamespace,
7878
* friendlier error message. The unique index provides a backstop against
7979
* race conditions.
8080
*/
81-
if (SearchSysCacheExists3(COLLNAMEENCNSP,
82-
PointerGetDatum(collname),
83-
Int32GetDatum(collencoding),
84-
ObjectIdGetDatum(collnamespace)))
81+
oid = GetSysCacheOid3(COLLNAMEENCNSP,
82+
Anum_pg_collation_oid,
83+
PointerGetDatum(collname),
84+
Int32GetDatum(collencoding),
85+
ObjectIdGetDatum(collnamespace));
86+
if (OidIsValid(oid))
8587
{
8688
if (quiet)
8789
return InvalidOid;
8890
else if (if_not_exists)
8991
{
92+
/*
93+
* If we are in an extension script, insist that the pre-existing
94+
* object be a member of the extension, to avoid security risks.
95+
*/
96+
ObjectAddressSet(myself, CollationRelationId, oid);
97+
checkMembershipInCurrentExtension(&myself);
98+
99+
/* OK to skip */
90100
ereport(NOTICE,
91101
(errcode(ERRCODE_DUPLICATE_OBJECT),
92102
collencoding == -1
@@ -116,16 +126,19 @@ CollationCreate(const char *collname, Oid collnamespace,
116126
* so we take a ShareRowExclusiveLock earlier, to protect against
117127
* concurrent changes fooling this check.
118128
*/
119-
if ((collencoding == -1 &&
120-
SearchSysCacheExists3(COLLNAMEENCNSP,
121-
PointerGetDatum(collname),
122-
Int32GetDatum(GetDatabaseEncoding()),
123-
ObjectIdGetDatum(collnamespace))) ||
124-
(collencoding != -1 &&
125-
SearchSysCacheExists3(COLLNAMEENCNSP,
126-
PointerGetDatum(collname),
127-
Int32GetDatum(-1),
128-
ObjectIdGetDatum(collnamespace))))
129+
if (collencoding == -1)
130+
oid = GetSysCacheOid3(COLLNAMEENCNSP,
131+
Anum_pg_collation_oid,
132+
PointerGetDatum(collname),
133+
Int32GetDatum(GetDatabaseEncoding()),
134+
ObjectIdGetDatum(collnamespace));
135+
else
136+
oid = GetSysCacheOid3(COLLNAMEENCNSP,
137+
Anum_pg_collation_oid,
138+
PointerGetDatum(collname),
139+
Int32GetDatum(-1),
140+
ObjectIdGetDatum(collnamespace));
141+
if (OidIsValid(oid))
129142
{
130143
if (quiet)
131144
{
@@ -134,6 +147,14 @@ CollationCreate(const char *collname, Oid collnamespace,
134147
}
135148
else if (if_not_exists)
136149
{
150+
/*
151+
* If we are in an extension script, insist that the pre-existing
152+
* object be a member of the extension, to avoid security risks.
153+
*/
154+
ObjectAddressSet(myself, CollationRelationId, oid);
155+
checkMembershipInCurrentExtension(&myself);
156+
157+
/* OK to skip */
137158
table_close(rel, NoLock);
138159
ereport(NOTICE,
139160
(errcode(ERRCODE_DUPLICATE_OBJECT),

‎src/backend/catalog/pg_depend.c

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

167167
/*
168168
* If we are executing a CREATE EXTENSION operation, mark the given object
169-
* as being a member of the extension. Otherwise, do nothing.
169+
* as being a member of the extension, or check that it already is one.
170+
* Otherwise, do nothing.
170171
*
171172
* This must be called during creation of any user-definable object type
172173
* that could be a member of an extension.
173174
*
174-
* If isReplace is true, the object already existed (or might have already
175-
* existed), so we must check for a pre-existing extension membership entry.
176-
* Passing false is a guarantee that the object is newly created, and so
177-
* could not already be a member of any extension.
175+
* isReplace must be true if the object already existed, and false if it is
176+
* newly created. In the former case we insist that it already be a member
177+
* of the current extension. In the latter case we can skip checking whether
178+
* it is already a member of any extension.
178179
*
179180
* Note: isReplace = true is typically used when updating an object in
180-
* CREATE OR REPLACE and similar commands. The net effect is that if an
181-
* extension script uses such a command on a pre-existing free-standing
182-
* object, the object will be absorbed into the extension. If the object
183-
* is already a member of some other extension, the command will fail.
184-
* This behavior is desirable for cases such as replacing a shell type.
181+
* CREATE OR REPLACE and similar commands. We used to allow the target
182+
* object to not already be an extension member, instead silently absorbing
183+
* it into the current extension. However, this was both error-prone
184+
* (extensions might accidentally overwrite free-standing objects) and
185+
* a security hazard (since the object would retain its previous ownership).
185186
*/
186187
void
187188
recordDependencyOnCurrentExtension(const ObjectAddress *object,
@@ -199,6 +200,12 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
199200
{
200201
Oid oldext;
201202

203+
/*
204+
* Side note: these catalog lookups are safe only because the
205+
* object is a pre-existing one. In the not-isReplace case, the
206+
* caller has most likely not yet done a CommandCounterIncrement
207+
* that would make the new object visible.
208+
*/
202209
oldext = getExtensionOfObject(object->classId, object->objectId);
203210
if (OidIsValid(oldext))
204211
{
@@ -212,6 +219,13 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
212219
getObjectDescription(object, false),
213220
get_extension_name(oldext))));
214221
}
222+
/* It's a free-standing object, so reject */
223+
ereport(ERROR,
224+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
225+
errmsg("%s is not a member of extension \"%s\"",
226+
getObjectDescription(object, false),
227+
get_extension_name(CurrentExtensionObject)),
228+
errdetail("An extension is not allowed to replace an object that it does not own.")));
215229
}
216230

217231
/* OK, record it as a member of CurrentExtensionObject */
@@ -223,6 +237,49 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
223237
}
224238
}
225239

240+
/*
241+
* If we are executing a CREATE EXTENSION operation, check that the given
242+
* object is a member of the extension, and throw an error if it isn't.
243+
* Otherwise, do nothing.
244+
*
245+
* This must be called whenever a CREATE IF NOT EXISTS operation (for an
246+
* object type that can be an extension member) has found that an object of
247+
* the desired name already exists. It is insecure for an extension to use
248+
* IF NOT EXISTS except when the conflicting object is already an extension
249+
* member; otherwise a hostile user could substitute an object with arbitrary
250+
* properties.
251+
*/
252+
void
253+
checkMembershipInCurrentExtension(const ObjectAddress *object)
254+
{
255+
/*
256+
* This is actually the same condition tested in
257+
* recordDependencyOnCurrentExtension; but we want to issue a
258+
* differently-worded error, and anyway it would be pretty confusing to
259+
* call recordDependencyOnCurrentExtension in these circumstances.
260+
*/
261+
262+
/* Only whole objects can be extension members */
263+
Assert(object->objectSubId == 0);
264+
265+
if (creating_extension)
266+
{
267+
Oid oldext;
268+
269+
oldext = getExtensionOfObject(object->classId, object->objectId);
270+
/* If already a member of this extension, OK */
271+
if (oldext == CurrentExtensionObject)
272+
return;
273+
/* Else complain */
274+
ereport(ERROR,
275+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
276+
errmsg("%s is not a member of extension \"%s\"",
277+
getObjectDescription(object, false),
278+
get_extension_name(CurrentExtensionObject)),
279+
errdetail("An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one that it already owns.")));
280+
}
281+
}
282+
226283
/*
227284
* deleteDependencyRecordsFor -- delete all records with given depender
228285
* 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
@@ -548,8 +548,11 @@ TypeCreate(Oid newTypeOid,
548548
* rebuild should be true if this is a pre-existing type. We will remove
549549
* existing dependencies and rebuild them from scratch. This is needed for
550550
* ALTER TYPE, and also when replacing a shell type. We don't remove any
551-
* existing extension dependency, though (hence, if makeExtensionDep is also
552-
* true and the type belongs to some other extension, an error will occur).
551+
* existing extension dependency, though; hence, if makeExtensionDep is also
552+
* true and we're in an extension script, an error will occur unless the
553+
* type already belongs to the current extension. That's the behavior we
554+
* want when replacing a shell type, which is the only case where both flags
555+
* are true.
553556
*/
554557
void
555558
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
@@ -859,13 +859,22 @@ CreateForeignServer(CreateForeignServerStmt *stmt)
859859
ownerId = GetUserId();
860860

861861
/*
862-
* Check that there is no other foreign server by this name. Do nothing if
863-
* IF NOT EXISTS was enforced.
862+
* Check that there is no other foreign server by this name. If there is
863+
* one, do nothing if IF NOT EXISTS was specified.
864864
*/
865-
if (GetForeignServerByName(stmt->servername, true) != NULL)
865+
srvId = get_foreign_server_oid(stmt->servername, true);
866+
if (OidIsValid(srvId))
866867
{
867868
if (stmt->if_not_exists)
868869
{
870+
/*
871+
* If we are in an extension script, insist that the pre-existing
872+
* object be a member of the extension, to avoid security risks.
873+
*/
874+
ObjectAddressSet(myself, ForeignServerRelationId, srvId);
875+
checkMembershipInCurrentExtension(&myself);
876+
877+
/* OK to skip */
869878
ereport(NOTICE,
870879
(errcode(ERRCODE_DUPLICATE_OBJECT),
871880
errmsg("server \"%s\" already exists, skipping",
@@ -1130,6 +1139,10 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
11301139
{
11311140
if (stmt->if_not_exists)
11321141
{
1142+
/*
1143+
* Since user mappings aren't members of extensions (see comments
1144+
* below), no need for checkMembershipInCurrentExtension here.
1145+
*/
11331146
ereport(NOTICE,
11341147
(errcode(ERRCODE_DUPLICATE_OBJECT),
11351148
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
@@ -149,6 +149,14 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
149149
RangeVarGetAndCheckCreationNamespace(seq->sequence, NoLock, &seqoid);
150150
if (OidIsValid(seqoid))
151151
{
152+
/*
153+
* If we are in an extension script, insist that the pre-existing
154+
* object be a member of the extension, to avoid security risks.
155+
*/
156+
ObjectAddressSet(address, RelationRelationId, seqoid);
157+
checkMembershipInCurrentExtension(&address);
158+
159+
/* OK to skip */
152160
ereport(NOTICE,
153161
(errcode(ERRCODE_DUPLICATE_TABLE),
154162
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
@@ -184,6 +184,10 @@ CreateStatistics(CreateStatsStmt *stmt)
184184
{
185185
if (stmt->if_not_exists)
186186
{
187+
/*
188+
* Since stats objects aren't members of extensions (see comments
189+
* below), no need for checkMembershipInCurrentExtension here.
190+
*/
187191
ereport(NOTICE,
188192
(errcode(ERRCODE_DUPLICATE_OBJECT),
189193
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)