8000 In extensions, don't replace objects not belonging to the extension. · postgres/postgres@f52d2fb · GitHub
[go: up one dir, main page]
Skip to content

Commit f52d2fb

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 97d6614 commit f52d2fb

File tree

21 files changed

+537
-50
lines changed

21 files changed

+537
-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-
6D4E 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: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -77,15 +77,24 @@ CollationCreate(const char *collname, Oid collnamespace,
7777
* friendlier error message. The unique index provides a backstop against
7878
* race conditions.
7979
*/
80-
if (SearchSysCacheExists3(COLLNAMEENCNSP,
81-
PointerGetDatum(collname),
82-
Int32GetDatum(collencoding),
83-
ObjectIdGetDatum(collnamespace)))
80+
oid = GetSysCacheOid3(COLLNAMEENCNSP,
81+
PointerGetDatum(collname),
82+
Int32GetDatum(collencoding),
83+
ObjectIdGetDatum(collnamespace));
84+
if (OidIsValid(oid))
8485
{
8586
if (quiet)
8687
return InvalidOid;
8788
else if (if_not_exists)
8889
{
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 */
8998
ereport(NOTICE,
9099
(errcode(ERRCODE_DUPLICATE_OBJECT),
91100
collencoding == -1
@@ -115,16 +124,17 @@ CollationCreate(const char *collname, Oid collnamespace,
115124
* so we take a ShareRowExclusiveLock earlier, to protect against
116125
* concurrent changes fooling this check.
117126
*/
118-
if ((collencoding == -1 &&
119-
SearchSysCacheExists3(COLLNAMEENCNSP,
120-
PointerGetDatum(collname),
121-
Int32GetDatum(GetDatabaseEncoding()),
122-
ObjectIdGetDatum(collnamespace))) ||
123-
(collencoding != -1 &&
124-
SearchSysCacheExists3(COLLNAMEENCNSP,
125-
PointerGetDatum(collname),
126-
Int32GetDatum(-1),
127-
ObjectIdGetDatum(collnamespace))))
127+
if (collencoding == -1)
128+
oid = GetSysCacheOid3(COLLNAMEENCNSP,
129+
PointerGetDatum(collname),
130+
Int32GetDatum(GetDatabaseEncoding()),
131+
ObjectIdGetDatum(collnamespace));
132+
else
133+
oid = GetSysCacheOid3(COLLNAMEENCNSP,
134+
PointerGetDatum(collname),
135+
Int32GetDatum(-1),
136+
ObjectIdGetDatum(collnamespace));
137+
if (OidIsValid(oid))
128138
{
129139
if (quiet)
130140
{
@@ -133,6 +143,14 @@ CollationCreate(const char *collname, Oid collnamespace,
133143
}
134144
else if (if_not_exists)
135145
{
146+
/*
147+
* If we are in an extension script, insist that the pre-existing
148+
* object be a member of the extension, to avoid security risks.
149+
*/
150+
ObjectAddressSet(myself, CollationRelationId, oid);
151+
checkMembershipInCurrentExtension(&myself);
152+
153+
/* OK to skip */
136154
heap_close(rel, NoLock);
137155
ereport(NOTICE,
138156
(errcode(ERRCODE_DUPLICATE_OBJECT),

‎src/backend/catalog/pg_depend.c

Lines changed: 69 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -125,15 +125,23 @@ recordMultipleDependencies(const ObjectAddress *depender,
125125

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

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

169190
/* OK, record it as a member of CurrentExtensionObject */
@@ -175,6 +196,49 @@ recordDependencyOnCurrentExtension(const ObjectAddress *object,
175196
}
176197
}
177198

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

859859
/* Dependency on extension */
860-
recordDependencyOnCurrentExtension(&myself, true);
860+
recordDependencyOnCurrentExtension(&myself, isUpdate);
861861

862862
return myself;
863863
}

‎src/backend/catalog/pg_type.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -511,10 +511,9 @@ TypeCreate(Oid newTypeOid,
511511
* If rebuild is true, we remove existing dependencies and rebuild them
512512
* from scratch. This is needed for ALTER TYPE, and also when replacing
513513
* a shell type. We don't remove an existing extension dependency, though.
514-
* (That means an extension can't absorb a shell type created in another
515-
* extension, nor ALTER a type created by another extension. Also, if it
516-
* replaces a free-standing shell type or ALTERs a free-standing type,
517-
* that type will become a member of the extension.)
514+
* That means an extension can't absorb a shell type that is free-standing
515+
* or belongs to another extension, nor ALTER a type that is free-standing or
516+
* belongs to another extension.
518517
*/
519518
void
520519
GenerateTypeDependencies(Oid typeObjectId,

‎src/backend/commands/createas.c

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -240,15 +240,27 @@ ExecCreateTableAs(CreateTableAsStmt *stmt, const char *queryString,
240240
if (stmt->if_not_exists)
241241
{
242242
Oid nspid;
243+
Oid oldrelid;
243244

244-
nspid = RangeVarGetCreationNamespace(stmt->into->rel);
245+
nspid = RangeVarGetCreationNamespace(into->rel);
245246

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

‎src/backend/commands/foreigncmds.c

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

880880
/*
881-
* Check that there is no other foreign server by this name. Do nothing if
882-
* IF NOT EXISTS was enforced.
881+
* Check that there is no other foreign server by this name. If there is
882+
* one, do nothing if IF NOT EXISTS was specified.
883883
*/
884-
if (GetForeignServerByName(stmt->servername, true) != NULL)
884+
srvId = get_foreign_server_oid(stmt->servername, true);
885+
if (OidIsValid(srvId))
885886
{
886887
if (stmt->if_not_exists)
887888
{
889+
/*
890+
* If we are in an extension script, insist that the pre-existing
891+
* object be a member of the extension, to avoid security risks.
892+
*/
893+
ObjectAddressSet(myself, ForeignServerRelationId, srvId);
894+
checkMembershipInCurrentExtension(&myself);
895+
896+
/* OK to skip */
888897
ereport(NOTICE,
889898
(errcode(ERRCODE_DUPLICATE_OBJECT),
890899
errmsg("server \"%s\" already exists, skipping",
@@ -1170,6 +1179,10 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
11701179
{
11711180
if (stmt->if_not_exists)
11721181
{
1182+
/*
1183+
* Since user mappings aren't members of extensions (see comments
1184+
* below), no need for checkMembershipInCurrentExtension here.
1185+
*/
11731186
ereport(NOTICE,
11741187
(errcode(ERRCODE_DUPLICATE_OBJECT),
11751188
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
@@ -147,6 +147,14 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
147147
RangeVarGetAndCheckCreationNamespace(seq->sequence, NoLock, &seqoid);
148148
if (OidIsValid(seqoid))
149149
{
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(address, RelationRelationId, seqoid);
155+
checkMembershipInCurrentExtension(&address);
156+
157+
/* OK to skip */
150158
ereport(NOTICE,
151159
(errcode(ERRCODE_DUPLICATE_TABLE),
152160
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
@@ -166,6 +166,10 @@ CreateStatistics(CreateStatsStmt *stmt)
166166
{
167167
if (stmt->if_not_exists)
168168
{
169+
/*
170+
* Since stats objects aren't members of extensions (see comments
171+
* below), no need for checkMembershipInCurrentExtension here.
172+
*/
169173
ereport(NOTICE,
170174
(errcode(ERRCODE_DUPLICATE_OBJECT),
171175
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
@@ -203,6 +203,16 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
203203
*/
204204
if (stmt->if_not_exists && OidIsValid(existing_relid))
205205
{
206+
/*
207+
* If we are in an extension script, insist that the pre-existing
208+
* object be a member of the extension, to avoid security risks.
209+
*/
210+
ObjectAddress address;
211+
212+
ObjectAddressSet(address, RelationRelationId, existing_relid);
213+
checkMembershipInCurrentExtension(&address);
214+
215+
/* OK to skip */
206216
ereport(NOTICE,
207217
(errcode(ERRCODE_DUPLICATE_TABLE),
208218
errmsg("relation \"%s\" already exists, skipping",

0 commit comments

Comments
 (0)
0