Skip to content

Commit 5579726

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 e12f5fd commit 5579726

21 files changed

+540
-50
lines changed

‎doc/src/sgml/extend.sgml

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,17 +1109,6 @@ SELECT * FROM pg_extension_update_paths('<replaceable>extension_name</replaceabl
11091109
<varname>search_path</varname>. However, no mechanism currently exists
11101110
to require that.
11111111
</para>
1112-
1113-
<para>
1114-
Do <emphasis>not</emphasis> use <command>CREATE OR REPLACE
1115-
FUNCTION</command>, except in an update script that must change the
1116-
definition of a function that is known to be an extension member
1117-
already. (Likewise for other <literal>OR REPLACE</literal> options.)
1118-
Using <literal>OR REPLACE</literal> unnecessarily not only has a risk
1119-
of accidentally overwriting someone else's function, but it creates a
1120-
security hazard since the overwritten function would still be owned by
1121-
its original owner, who could modify it.
1122-
</para>
11231112
</sect3>
11241113
</sect2>
11251114

‎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: 69 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -124,15 +124,23 @@ recordMultipleDependencies(const ObjectAddress *depender,
124124

125125
/*
126126
* If we are executing a CREATE EXTENSION operation, mark the given object
127-
* as being a member of the extension. Otherwise, do nothing.
127+
* as being a member of the extension, or check that it already is one.
128+
* Otherwise, do nothing.
128129
*
129130
* This must be called during creation of any user-definable object type
130131
* that could be a member of an extension.
131132
*
132-
* If isReplace is true, the object already existed (or might have already
133-
* existed), so we must check for a pre-existing extension membership entry.
134-
* Passing false is a guarantee that the object is newly created, and so
135-
* could not already be a member of any extension.
133+
* isReplace must be true if the object already existed, and false if it is
134+
* newly created. In the former case we insist that it already be a member
135+
* of the current extension. In the latter case we can skip checking whether
136+
* it is already a member of any extension.
137+
*
138+
* Note: isReplace = true is typically used when updating a object in
139+
* CREATE OR REPLACE and similar commands. We used to allow the target
140+
* object to not already be an extension member, instead silently absorbing
141+
* it into the current extension. However, this was both error-prone
142+
* (extensions might accidentally overwrite free-standing objects) and
143+
* a security hazard (since the object would retain its previous ownership).
136144
*/
137145
void
138146
recordDependencyOnCurrentExtension(const ObjectAddress *object,
@@ -150,6 +158,12 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
150158
{
151159
Oid oldext;
152160

161+
/*
162+
* Side note: these catalog lookups are safe only because the
163+
* object is a pre-existing one. In the not-isReplace case, the
164+
* caller has most likely not yet done a CommandCounterIncrement
165+
* that would make the new object visible.
166+
*/
153167
oldext = getExtensionOfObject(object->classId, object->objectId);
154168
if (OidIsValid(oldext))
155169
{
@@ -163,6 +177,13 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
163177
getObjectDescription(object),
164178
get_extension_name(oldext))));
165179
}
180+
/* It's a free-standing object, so reject */
181+
ereport(ERROR,
182+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
183+
errmsg("%s is not a member of extension \"%s\"",
184+
getObjectDescription(object),
185+
get_extension_name(CurrentExtensionObject)),
186+
errdetail("An extension is not allowed to replace an object that it does not own.")));
166187
}
167188

168189
/* OK, record it as a member of CurrentExtensionObject */
@@ -174,6 +195,49 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
174195
}
175196
}
176197

198+
/*
199+
* If we are executing a CREATE EXTENSION operation, check that the given
200+
* object is a member of the extension, and throw an error if it isn't.
201+
* Otherwise, do nothing.
202+
*
203+
* This must be called whenever a CREATE IF NOT EXISTS operation (for an
204+
* object type that can be an extension member) has found that an object of
205+
* the desired name already exists. It is insecure for an extension to use
206+
* IF NOT EXISTS except when the conflicting object is already an extension
207+
* member; otherwise a hostile user could substitute an object with arbitrary
208+
* properties.
209+
*/
210+
void
211+
checkMembershipInCurrentExtension(const ObjectAddress *object)
212+
{
213+
/*
214+
* This is actually the same condition tested in
215+
* recordDependencyOnCurrentExtension; but we want to issue a
216+
* differently-worded error, and anyway it would be pretty confusing to
217+
* call recordDependencyOnCurrentExtension in these circumstances.
218+
*/
219+
220+
/* Only whole objects can be extension members */
221+
Assert(object->objectSubId == 0);
222+
223+
if (creating_extension)
224+
{
225+
Oid oldext;
226+
227+
oldext = getExtensionOfObject(object->classId, object->objectId);
228+
/* If already a member of this extension, OK */
229+
if (oldext == CurrentExtensionObject)
230+
return;
231+
/* Else complain */
232+
ereport(ERROR,
233+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
234+
errmsg("%s is not a member of extension \"%s\"",
235+
getObjectDescription(object),
236+
get_extension_name(CurrentExtensionObject)),
237+
errdetail("An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one that it already owns.")));
238+
}
239+
}
240+
177241
/*
178242
* deleteDependencyRecordsFor -- delete all records with given depender
179243
* 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
@@ -867,7 +867,7 @@ makeOperatorDependencies(HeapTuple tuple, bool isUpdate)
867867
oper->oprowner);
868868

869869
/* Dependency on extension */
870-
recordDependencyOnCurrentExtension(&myself, true);
870+
recordDependencyOnCurrentExtension(&myself, isUpdate);
871871

872872
return myself;
873873
}

‎src/backend/catalog/pg_type.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -528,10 +528,9 @@ TypeCreate(Oid newTypeOid,
528528
* If rebuild is true, we remove existing dependencies and rebuild them
529529
* from scratch. This is needed for ALTER TYPE, and also when replacing
530530
* a shell type. We don't remove an existing extension dependency, though.
531-
* (That means an extension can't absorb a shell type created in another
532-
* extension, nor ALTER a type created by another extension. Also, if it
533-
* replaces a free-standing shell type or ALTERs a free-standing type,
534-
* that type will become a member of the extension.)
531+
* That means an extension can't absorb a shell type that is free-standing
532+
* or belongs to another extension, nor ALTER a type that is free-standing or
533+
* belongs to another extension.
535534
*/
536535
void
537536
GenerateTypeDependencies(Oid typeObjectId,

‎src/backend/commands/createas.c

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -243,15 +243,27 @@ ExecCreateTableAs(CreateTableAsStmt *stmt, const char *queryString,
243243
if (stmt->if_not_exists)
244244
{
245245
Oid nspid;
246+
Oid oldrelid;
246247

247-
nspid = RangeVarGetCreationNamespace(stmt->into->rel);
248+
nspid = RangeVarGetCreationNamespace(into->rel);
248249

249-
if (get_relname_relid(stmt->into->rel->relname, nspid))
250+
oldrelid = get_relname_relid(into->rel->relname, nspid);
251+
if (OidIsValid(oldrelid))
250252
{
253+
/*
254+
* The relation exists and IF NOT EXISTS has been specified.
255+
*
256+
* If we are in an extension script, insist that the pre-existing
257+
* object be a member of the extension, to avoid security risks.
258+
*/
259+
ObjectAddressSet(address, RelationRelationId, oldrelid);
260+
checkMembershipInCurrentExtension(&address);
261+
262+
/* OK to skip */
251263
ereport(NOTICE,
252264
(errcode(ERRCODE_DUPLICATE_TABLE),
253265
errmsg("relation \"%s\" already exists, skipping",
254-
stmt->into->rel->relname)));
266+
into->rel->relname)));
255267
return InvalidObjectAddress;
256268
}
257269
}

‎src/backend/commands/foreigncmds.c

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

889889
/*
890-
* Check that there is no other foreign server by this name. Do nothing if
891-
* IF NOT EXISTS was enforced.
890+
* Check that there is no other foreign server by this name. If there is
891+
* one, do nothing if IF NOT EXISTS was specified.
892892
*/
893-
if (GetForeignServerByName(stmt->servername, true) != NULL)
893+
srvId = get_foreign_server_oid(stmt->servername, true);
894+
if (OidIsValid(srvId))
894895
{
895896
if (stmt->if_not_exists)
896897
{
898+
/*
899+
* If we are in an extension script, insist that the pre-existing
900+
* object be a member of the extension, to avoid security risks.
901+
*/
902+
ObjectAddressSet(myself, ForeignServerRelationId, srvId);
903+
checkMembershipInCurrentExtension(&myself);
904+
905+
/* OK to skip */
897906
ereport(NOTICE,
898907
(errcode(ERRCODE_DUPLICATE_OBJECT),
899908
errmsg("server \"%s\" already exists, skipping",
@@ -1182,6 +1191,10 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
11821191
{
11831192
if (stmt->if_not_exists)
11841193
{
1194+
/*
1195+
* Since user mappings aren't members of extensions (see comments
1196+
* below), no need for checkMembershipInCurrentExtension here.
1197+
*/
11851198
ereport(NOTICE,
11861199
(errcode(ERRCODE_DUPLICATE_OBJECT),
11871200
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
@@ -113,14 +113,25 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString,
113113
* the permissions checks, but since CREATE TABLE IF NOT EXISTS makes its
114114
* creation-permission check first, we do likewise.
115115
*/
116-
if (stmt->if_not_exists &&
117-
SearchSysCacheExists1(NAMESPACENAME, PointerGetDatum(schemaName)))
116+
if (stmt->if_not_exists)
118117
{
119-
ereport(NOTICE,
120-
(errcode(ERRCODE_DUPLICATE_SCHEMA),
121-
errmsg("schema \"%s\" already exists, skipping",
122-
schemaName)));
123-
return InvalidOid;
118+
namespaceId = get_namespace_oid(schemaName, true);
119+
if (OidIsValid(namespaceId))
120+
{
121+
/*
122+
* If we are in an extension script, insist that the pre-existing
123+
* object be a member of the extension, to avoid security risks.
124+
*/
125+
ObjectAddressSet(address, NamespaceRelationId, namespaceId);
126+
checkMembershipInCurrentExtension(&address);
127+
128+
/* OK to skip */
129+
ereport(NOTICE,
130+
(errcode(ERRCODE_DUPLICATE_SCHEMA),
131+
errmsg("schema \"%s\" already exists, skipping",
132+
schemaName)));
133+
return InvalidOid;
134+
}
124135
}
125136

126137
/*

‎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
@@ -173,6 +173,10 @@ CreateStatistics(CreateStatsStmt *stmt)
173173
{
174174
if (stmt->if_not_exists)
175175
{
176+
/*
177+
* Since stats objects aren't members of extensions (see comments
178+
* below), no need for checkMembershipInCurrentExtension here.
179+
*/
176180
ereport(NOTICE,
177181
(errcode(ERRCODE_DUPLICATE_OBJECT),
178182
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
@@ -205,7 +205,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
205205
CommandCounterIncrement();
206206

207207
/*
208-
* Finally update the view options.
208+
* Update the view's options.
209209
*
210210
* The new options list replaces the existing options list, even if
211211
* it's empty.
@@ -218,8 +218,22 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
218218
/* EventTriggerAlterTableStart called by ProcessUtilitySlow */
219219
AlterTableInternal(viewOid, atcmds, true);
220220

221+
/*
222+
* There is very little to do here to update the view's dependencies.
223+
* Most view-level dependency relationships, such as those on the
224+
* owner, schema, and associated composite type, aren't changing.
225+
* Because we don't allow changing type or collation of an existing
226+
* view column, those dependencies of the existing columns don't
227+
* change either, while the AT_AddColumnToView machinery took care of
228+
* adding such dependencies for new view columns. The dependencies of
229+
* the view's query could have changed arbitrarily, but that was dealt
230+
* with inside StoreViewQuery. What remains is only to check that
231+
* view replacement is allowed when we're creating an extension.
232+
*/
221233
ObjectAddressSet(address, RelationRelationId, viewOid);
222234

235+
recordDependencyOnCurrentExtension(&address, true);
236+
223237
/*
224238
* Seems okay, so return the OID of the pre-existing view.
225239
*/

‎src/backend/parser/parse_utilcmd.c

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,16 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
206206
*/
207207
if (stmt->if_not_exists && OidIsValid(existing_relid))
208208
{
209+
/*
210+
* If we are in an extension script, insist that the pre-existing
211+
* object be a member of the extension, to avoid security risks.
212+
*/
213+
ObjectAddress address;
214+
215+
ObjectAddressSet(address, RelationRelationId, existing_relid);
216+
checkMembershipInCurrentExtension(&address);
217+
218+
/* OK to skip */
209219
ereport(NOTICE,
210220
(errcode(ERRCODE_DUPLICATE_TABLE),
211221
errmsg("relation \"%s\" already exists, skipping",

0 commit comments

Comments
 (0)