Skip to content

Commit da83ca7

Browse files
tglsfdctglsfdc
committed
Make pg_dump's ACL, sec label, and comment entries reliably identifiable.
_tocEntryRequired() expects that it can identify ACL, SECURITY LABEL, and COMMENT TOC entries that are for large objects by seeing whether the tag for them starts with "LARGE OBJECT ". While that works fine for actual large objects, which are indeed tagged that way, it's subject to false positives unless every such entry's tag starts with an appropriate type ID. And in fact it does not work for ACLs, because up to now we customarily tagged those entries with just the bare name of the object. This means that an ACL for an object named "LARGE OBJECT something" would be misclassified as data not schema, with undesirable results in a schema-only or data-only dump --- although pg_upgrade seems unaffected, due to the special case for binary-upgrade mode further down in _tocEntryRequired(). We can fix this by changing all the dumpACL calls to use the label strings already in use for comments and security labels, which do follow the convention of starting with an object type indicator. Well, mostly they follow it. dumpDatabase() got it wrong, using just the bare database name for those purposes, so that a database named "LARGE OBJECT something" would similarly be subject to having its comment or security label dropped or included when not wanted. Bring that into line too. (Note that up to now, database ACLs have not been processed by pg_dump, so that this issue doesn't affect them.) _tocEntryRequired() itself is not free of fault: it was overly liberal about matching object tags to "LARGE OBJECT " in binary-upgrade mode. This looks like it is probably harmless because there would be no data component to strip anyway in that mode, but at best it's trouble waiting to happen, so tighten that up too. The possible misclassification of SECURITY LABEL entries for databases is in principle a security problem, but the opportunities for actual exploits seem too narrow to be interesting. The other cases seem like just bugs, since an object owner can change its ACL or comment for himself, he needn't try to trick someone else into doing it by choosing a strange name. This has been broken since per-large-object TOC entries were introduced in 9.0, so back-patch to all supported branches. Discussion: https://postgr.es/m/21714.1516553459@sss.pgh.pa.us
1 parent f840557 commit da83ca7

File tree

2 files changed

+51
-36
lines changed

2 files changed

+51
-36
lines changed

‎src/bin/pg_dump/pg_backup_archiver.c

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2744,8 +2744,14 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
27442744
* other information should be generated in binary-upgrade mode (not
27452745
* the actual data).
27462746
*/
2747-
if (!(ropt->binary_upgrade && strcmp(te->desc,"BLOB") == 0) &&
2748-
!(ropt->binary_upgrade && strncmp(te->tag,"LARGE OBJECT ", 13) == 0))
2747+
if (!(ropt->binary_upgrade &&
2748+
(strcmp(te->desc, "BLOB") == 0 ||
2749+
(strcmp(te->desc, "ACL") == 0 &&
2750+
strncmp(te->tag, "LARGE OBJECT ", 13) == 0) ||
2751+
(strcmp(te->desc, "COMMENT") == 0 &&
2752+
strncmp(te->tag, "LARGE OBJECT ", 13) == 0) ||
2753+
(strcmp(te->desc, "SECURITY LABEL") == 0 &&
2754+
strncmp(te->tag, "LARGE OBJECT ", 13) == 0))))
27492755
res = res & REQ_SCHEMA;
27502756
}
27512757

‎src/bin/pg_dump/pg_dump.c

Lines changed: 43 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2295,6 +2295,7 @@ dumpDatabase(Archive *fout)
22952295
PQExpBuffer dbQry = createPQExpBuffer();
22962296
PQExpBuffer delQry = createPQExpBuffer();
22972297
PQExpBuffer creaQry = createPQExpBuffer();
2298+
PQExpBuffer labelq = createPQExpBuffer();
22982299
PGconn *conn = GetConnection(fout);
22992300
PGresult *res;
23002301
int i_tableoid,
@@ -2578,16 +2579,20 @@ dumpDatabase(Archive *fout)
25782579
destroyPQExpBuffer(loOutQry);
25792580
}
25802581

2582+
/* Compute correct tag for comments etc */
2583+
appendPQExpBuffer(labelq, "DATABASE %s", fmtId(datname));
2584+
25812585
/* Dump DB comment if any */
25822586
if (fout->remoteVersion >= 80200)
25832587
{
25842588
/*
2585-
* 8.2 keeps comments on shared objects in a shared table, so we
2586-
* cannot use the dumpComment used for other database objects.
2589+
* 8.2 and up keep comments on shared objects in a shared table, so we
2590+
* cannot use the dumpComment() code used for other database objects.
2591+
* Be careful that the ArchiveEntry parameters match that function.
25872592
*/
25882593
char *comment = PQgetvalue(res, 0, PQfnumber(res, "description"));
25892594

2590-
if (comment && strlen(comment))
2595+
if (comment && *comment)
25912596
{
25922597
resetPQExpBuffer(dbQry);
25932598

@@ -2599,17 +2604,17 @@ dumpDatabase(Archive *fout)
25992604
appendStringLiteralAH(dbQry, comment, fout);
26002605
appendPQExpBufferStr(dbQry, ";\n");
26012606

2602-
ArchiveEntry(fout, dbCatId, createDumpId(), datname, NULL, NULL,
2603-
dba, false, "COMMENT", SECTION_NONE,
2607+
ArchiveEntry(fout, nilCatalogId, createDumpId(),
2608+
labelq->data, NULL, NULL, dba,
2609+
false, "COMMENT", SECTION_NONE,
26042610
dbQry->data, "", NULL,
2605-
&dbDumpId, 1, NULL, NULL);
2611+
&(dbDumpId), 1,
2612+
NULL, NULL);
26062613
}
26072614
}
26082615
else
26092616
{
2610-
resetPQExpBuffer(dbQry);
2611-
appendPQExpBuffer(dbQry, "DATABASE %s", fmtId(datname));
2612-
dumpComment(fout, dbQry->data, NULL, "",
2617+
dumpComment(fout, labelq->data, NULL, dba,
26132618
dbCatId, 0, dbDumpId);
26142619
}
26152620

@@ -2625,11 +2630,13 @@ dumpDatabase(Archive *fout)
26252630
shres = ExecuteSqlQuery(fout, seclabelQry->data, PGRES_TUPLES_OK);
26262631
resetPQExpBuffer(seclabelQry);
26272632
emitShSecLabels(conn, shres, seclabelQry, "DATABASE", datname);
2628-
if (strlen(seclabelQry->data))
2629-
ArchiveEntry(fout, dbCatId, createDumpId(), datname, NULL, NULL,
2630-
dba, false, "SECURITY LABEL", SECTION_NONE,
2633+
if (seclabelQry->len > 0)
2634+
ArchiveEntry(fout, nilCatalogId, createDumpId(),
2635+
labelq->data, NULL, NULL, dba,
2636+
false, "SECURITY LABEL", SECTION_NONE,
26312637
seclabelQry->data, "", NULL,
2632-
&dbDumpId, 1, NULL, NULL);
2638+
&(dbDumpId), 1,
2639+
NULL, NULL);
26332640
destroyPQExpBuffer(seclabelQry);
26342641
PQclear(shres);
26352642
}
@@ -2639,6 +2646,7 @@ dumpDatabase(Archive *fout)
26392646
destroyPQExpBuffer(dbQry);
26402647
destroyPQExpBuffer(delQry);
26412648
destroyPQExpBuffer(creaQry);
2649+
destroyPQExpBuffer(labelq);
26422650
}
26432651

26442652

@@ -8139,7 +8147,7 @@ dumpNamespace(Archive *fout, NamespaceInfo *nspinfo)
81398147
nspinfo->dobj.catId, 0, nspinfo->dobj.dumpId);
81408148

81418149
dumpACL(fout, nspinfo->dobj.catId, nspinfo->dobj.dumpId, "SCHEMA",
8142-
qnspname, NULL, nspinfo->dobj.name, NULL,
8150+
qnspname, NULL, labelq->data, NULL,
81438151
nspinfo->rolname, nspinfo->nspacl);
81448152

81458153
free(qnspname);
@@ -8420,7 +8428,7 @@ dumpEnumType(Archive *fout, TypeInfo *tyinfo)
84208428
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
84218429

84228430
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
8423-
qtypname, NULL, tyinfo->dobj.name,
8431+
qtypname, NULL, labelq->data,
84248432
tyinfo->dobj.namespace->dobj.name,
84258433
tyinfo->rolname, tyinfo->typacl);
84268434

@@ -8552,7 +8560,7 @@ dumpRangeType(Archive *fout, TypeInfo *tyinfo)
85528560
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
85538561

85548562
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
8555-
qtypname, NULL, tyinfo->dobj.name,
8563+
qtypname, NULL, labelq->data,
85568564
tyinfo->dobj.namespace->dobj.name,
85578565
tyinfo->rolname, tyinfo->typacl);
85588566

@@ -8621,7 +8629,7 @@ dumpUndefinedType(Archive *fout, TypeInfo *tyinfo)
86218629
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
86228630

86238631
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
8624-
qtypname, NULL, tyinfo->dobj.name,
8632+
qtypname, NULL, labelq->data,
86258633
tyinfo->dobj.namespace->dobj.name,
86268634
tyinfo->rolname, tyinfo->typacl);
86278635

@@ -9009,7 +9017,7 @@ dumpBaseType(Archive *fout, TypeInfo *tyinfo)
90099017
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
90109018

90119019
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
9012-
qtypname, NULL, tyinfo->dobj.name,
9020+
qtypname, NULL, labelq->data,
90139021
tyinfo->dobj.namespace->dobj.name,
90149022
tyinfo->rolname, tyinfo->typacl);
90159023

@@ -9171,7 +9179,7 @@ dumpDomain(Archive *fout, TypeInfo *tyinfo)
91719179
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
91729180

91739181
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
9174-
qtypname, NULL, tyinfo->dobj.name,
9182+
qtypname, NULL, labelq->data,
91759183
tyinfo->dobj.namespace->dobj.name,
91769184
tyinfo->rolname, tyinfo->typacl);
91779185

@@ -9380,7 +9388,7 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
93809388
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
93819389

93829390
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
9383-
qtypname, NULL, tyinfo->dobj.name,
9391+
qtypname, NULL, labelq->data,
93849392
tyinfo->dobj.namespace->dobj.name,
93859393
tyinfo->rolname, tyinfo->typacl);
93869394

@@ -9690,7 +9698,7 @@ dumpProcLang(Archive *fout, ProcLangInfo *plang)
96909698

96919699
if (plang->lanpltrusted)
96929700
dumpACL(fout, plang->dobj.catId, plang->dobj.dumpId, "LANGUAGE",
9693-
qlanname, NULL, plang->dobj.name,
9701+
qlanname, NULL, labelq->data,
96949702
lanschema,
96959703
plang->lanowner, plang->lanacl);
96969704

@@ -10279,7 +10287,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
1027910287
finfo->dobj.catId, 0, finfo->dobj.dumpId);
1028010288

1028110289
dumpACL(fout, finfo->dobj.catId, finfo->dobj.dumpId, "FUNCTION",
10282-
funcsig, NULL, funcsig_tag,
10290+
funcsig, NULL, labelq->data,
1028310291
finfo->dobj.namespace->dobj.name,
1028410292
finfo->rolname, finfo->proacl);
1028510293

@@ -12035,14 +12043,12 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
1203512043
* syntax for zero-argument aggregates and ordered-set aggregates.
1203612044
*/
1203712045
free(aggsig);
12038-
free(aggsig_tag);
1203912046

1204012047
aggsig = format_function_signature(fout, &agginfo->aggfn, true);
12041-
aggsig_tag = format_function_signature(fout, &agginfo->aggfn, false);
1204212048

1204312049
dumpACL(fout, agginfo->aggfn.dobj.catId, agginfo->aggfn.dobj.dumpId,
1204412050
"FUNCTION",
12045-
aggsig, NULL, aggsig_tag,
12051+
aggsig, NULL, labelq->data,
1204612052
agginfo->aggfn.dobj.namespace->dobj.name,
1204712053
agginfo->aggfn.rolname, agginfo->aggfn.proacl);
1204812054

@@ -12471,7 +12477,7 @@ dumpForeignDataWrapper(Archive *fout, FdwInfo *fdwinfo)
1247112477
/* Handle the ACL */
1247212478
dumpACL(fout, fdwinfo->dobj.catId, fdwinfo->dobj.dumpId,
1247312479
"FOREIGN DATA WRAPPER",
12474-
qfdwname, NULL, fdwinfo->dobj.name,
12480+
qfdwname, NULL, labelq->data,
1247512481
NULL, fdwinfo->rolname,
1247612482
fdwinfo->fdwacl);
1247712483

@@ -12563,7 +12569,7 @@ dumpForeignServer(Archive *fout, ForeignServerInfo *srvinfo)
1256312569
/* Handle the ACL */
1256412570
dumpACL(fout, srvinfo->dobj.catId, srvinfo->dobj.dumpId,
1256512571
"FOREIGN SERVER",
12566-
qsrvname, NULL, srvinfo->dobj.name,
12572+
qsrvname, NULL, labelq->data,
1256712573
NULL, srvinfo->rolname,
1256812574
srvinfo->srvacl);
1256912575

@@ -12762,7 +12768,8 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
1276212768
* FOREIGN DATA WRAPPER, SERVER, or LARGE OBJECT.
1276312769
* 'name' is the formatted name of the object. Must be quoted etc. already.
1276412770
* 'subname' is the formatted name of the sub-object, if any. Must be quoted.
12765-
* 'tag' is the tag for the archive entry (typ. unquoted name of object).
12771+
* 'tag' is the tag for the archive entry (should be the same tag as would be
12772+
* used for comments etc; for example "TABLE foo").
1276612773
* 'nspname' is the namespace the object is in (NULL if none).
1276712774
* 'owner' is the owner, NULL if there is no owner (for languages).
1276812775
* 'acls' is the string read out of the fooacl system catalog field;
@@ -12838,7 +12845,7 @@ dumpSecLabel(Archive *fout, const char *target,
1283812845
if (no_security_labels)
1283912846
return;
1284012847

12841-
/* Comments are schema not data ... except blob comments are data */
12848+
/* Security labels are schema not data ... except blob labels are data */
1284212849
if (strncmp(target, "LARGE OBJECT ", 13) != 0)
1284312850
{
1284412851
if (dataOnly)
@@ -13116,6 +13123,8 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
1311613123
if (tbinfo->dobj.dump && !dataOnly)
1311713124
{
1311813125
char *namecopy;
13126+
const char *objtype;
13127+
char *acltag;
1311913128

1312013129
if (tbinfo->relkind == RELKIND_SEQUENCE)
1312113130
dumpSequence(fout, tbinfo);
@@ -13124,12 +13133,13 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
1312413133

1312513134
/* Handle the ACL here */
1312613135
namecopy = pg_strdup(fmtId(tbinfo->dobj.name));
13136+
objtype = (tbinfo->relkind == RELKIND_SEQUENCE) ? "SEQUENCE" : "TABLE";
13137+
acltag = psprintf("%s %s", objtype, namecopy);
1312713138
dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
13128-
(tbinfo->relkind == RELKIND_SEQUENCE) ? "SEQUENCE" :
13129-
"TABLE",
13130-
namecopy, NULL, tbinfo->dobj.name,
13139+
objtype, namecopy, NULL, acltag,
1313113140
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
1313213141
tbinfo->relacl);
13142+
free(acltag);
1313313143

1313413144
/*
1313513145
* Handle column ACLs, if any. Note: we pull these with a separate
@@ -13154,10 +13164,9 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
1315413164
char *attname = PQgetvalue(res, i, 0);
1315513165
char *attacl = PQgetvalue(res, i, 1);
1315613166
char *attnamecopy;
13157-
char *acltag;
1315813167

1315913168
attnamecopy = pg_strdup(fmtId(attname));
13160-
acltag = psprintf("%s.%s", tbinfo->dobj.name, attname);
13169+
acltag = psprintf("COLUMN %s.%s", namecopy, attnamecopy);
1316113170
/* Column's GRANT type is always TABLE */
1316213171
dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId, "TABLE",
1316313172
namecopy, attnamecopy, acltag,

0 commit comments

Comments
 (0)