Re: createdb compares strategy as case-sensitive
On 4/21/24 17:10, Tom Lane wrote: > Tomas Vondra writes: >> On 4/21/24 00:19, Tom Lane wrote: >>> I'm not suggesting that this is an interesting security vulnerability, >>> because if you can control the arguments to createdb it's probably >>> game over long since. But wrapping the arguments is good for >>> delivering on-point error messages. So I'd add a fmtId() call to >>> LOCALE_PROVIDER too. > >> OK, the attached 0001 patch does these three things - adds the fmtId() >> for locale_provider, make the comparison case-insensitive for strategy >> and also removes the comma from the hint. > > LGTM. > Pushed, after tweaking the commit message a bit. >> The createdb vs. CREATE DATABASE difference made me look if we have any >> regression tests for CREATE DATABASE, and we don't. I guess it would be >> good to have some, so I added a couple, for some of the parameters, see >> 0002. But there's a problem with the locale stuff - this seems to work >> in plain "make check", but pg_upgrade creates the clusters with >> different providers etc. which changes the expected output. I'm not sure >> there's a good way to deal with this ... > > Probably not worth the trouble, really. > Agreed. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: createdb compares strategy as case-sensitive
Tomas Vondra writes: > On 4/21/24 00:19, Tom Lane wrote: >> I'm not suggesting that this is an interesting security vulnerability, >> because if you can control the arguments to createdb it's probably >> game over long since. But wrapping the arguments is good for >> delivering on-point error messages. So I'd add a fmtId() call to >> LOCALE_PROVIDER too. > OK, the attached 0001 patch does these three things - adds the fmtId() > for locale_provider, make the comparison case-insensitive for strategy > and also removes the comma from the hint. LGTM. > The createdb vs. CREATE DATABASE difference made me look if we have any > regression tests for CREATE DATABASE, and we don't. I guess it would be > good to have some, so I added a couple, for some of the parameters, see > 0002. But there's a problem with the locale stuff - this seems to work > in plain "make check", but pg_upgrade creates the clusters with > different providers etc. which changes the expected output. I'm not sure > there's a good way to deal with this ... Probably not worth the trouble, really. regards, tom lane
Re: createdb compares strategy as case-sensitive
On 4/21/24 00:19, Tom Lane wrote:
> Tomas Vondra writes:
>> On 4/20/24 22:40, Tom Lane wrote:
>>> Seems reasonable. The alternative could be to remove createdb.c's use
>>> of fmtId() here, but I don't think that's actually better.
>
>> Why? It seems to me this is quite close to e.g. LOCALE_PROVIDER, and we
>> don't do fmtId() for that. So why should we do that for STRATEGY?
>
> Hah, nothing like being randomly inconsistent with adjacent code.
> Every other argument handled by createdb gets wrapped by either
> fmtId or appendStringLiteralConn.
>
> I think the argument for this is it ensures that the switch value as
> accepted by createdb is the argument that CREATE DATABASE will see.
> Compare
>
> $ createdb --strategy="foo bar" mydb
> createdb: error: database creation failed: ERROR: invalid create database
> strategy "foo bar"
> HINT: Valid strategies are "wal_log", and "file_copy".
>
> $ createdb --locale-provider="foo bar" mydb
> createdb: error: database creation failed: ERROR: syntax error at or near ";"
> LINE 1: CREATE DATABASE mydb LOCALE_PROVIDER foo bar;
> ^
>
> I'm not suggesting that this is an interesting security vulnerability,
> because if you can control the arguments to createdb it's probably
> game over long since. But wrapping the arguments is good for
> delivering on-point error messages. So I'd add a fmtId() call to
> LOCALE_PROVIDER too.
>
> BTW, someone's taken the Oxford comma too far in that HINT.
> Nobody writes a comma when there are only two alternatives.
>
OK, the attached 0001 patch does these three things - adds the fmtId()
for locale_provider, make the comparison case-insensitive for strategy
and also removes the comma from the hint.
The createdb vs. CREATE DATABASE difference made me look if we have any
regression tests for CREATE DATABASE, and we don't. I guess it would be
good to have some, so I added a couple, for some of the parameters, see
0002. But there's a problem with the locale stuff - this seems to work
in plain "make check", but pg_upgrade creates the clusters with
different providers etc. which changes the expected output. I'm not sure
there's a good way to deal with this ...
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom e7a18c780591673592b2a5eb1e129fcceb17f0fe Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sun, 21 Apr 2024 14:19:32 +0200
Subject: [PATCH 1/2] createdb: compare strategy case-insensitive
When specifying the createdb strategy, the documentation suggests valid
options are FILE_COPY and WAL_LOG, but the code does case-sensitive
comparison and accepts only "file_copy" and "wal_log" as valid.
Fixed by doing a case-insensitive comparison using pg_strcasecmp(), same
as for other string parameters.
While at it, apply fmtId() to a nearby "locale_provider". This already
did the comparison in case-insensitive way, but the value would not be
double-quoted, confusing the parser and the error message.
Backpatch to 15, where the strategy was introduced.
Backpatch-through: 15
---
src/backend/commands/dbcommands.c | 6 +++---
src/bin/scripts/createdb.c| 2 +-
src/bin/scripts/t/020_createdb.pl | 10 ++
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 8229dfa1f22..cd06d1270c5 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -1018,15 +1018,15 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
char *strategy;
strategy = defGetString(dstrategy);
- if (strcmp(strategy, "wal_log") == 0)
+ if (pg_strcasecmp(strategy, "wal_log") == 0)
dbstrategy = CREATEDB_WAL_LOG;
- else if (strcmp(strategy, "file_copy") == 0)
+ else if (pg_strcasecmp(strategy, "file_copy") == 0)
dbstrategy = CREATEDB_FILE_COPY;
else
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid create database strategy \"%s\"", strategy),
- errhint("Valid strategies are \"wal_log\", and \"file_copy\".")));
+ errhint("Valid strategies are \"wal_log\" and \"file_copy\".")));
}
/* If encoding or locales are defaulted, use source's setting */
diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c
index 007061e756f..30426860efa 100644
--- a/src/bin/scripts/createdb.c
+++ b/src/bin/scripts/createdb.c
@@ -237,7 +237,7 @@ main(int argc, char *argv[])
appendStringLiteralConn(, lc_ctype, conn);
}
if (locale_provider)
- appendPQExpBuffer(, " LOCALE_PROVIDER %s", locale_provider);
+ appendPQExpBuffer(, " LOCALE_PROVIDER %s", fmtId(locale_provider));
if (icu_locale)
{
appendPQExpBufferStr(, " ICU_LOCALE ");
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index 512b55c48a9..4a0e2c883a1 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -255,11 +255,21
Re: createdb compares strategy as case-sensitive
Tomas Vondra writes: > On 4/20/24 22:40, Tom Lane wrote: >> Seems reasonable. The alternative could be to remove createdb.c's use >> of fmtId() here, but I don't think that's actually better. > Why? It seems to me this is quite close to e.g. LOCALE_PROVIDER, and we > don't do fmtId() for that. So why should we do that for STRATEGY? Hah, nothing like being randomly inconsistent with adjacent code. Every other argument handled by createdb gets wrapped by either fmtId or appendStringLiteralConn. I think the argument for this is it ensures that the switch value as accepted by createdb is the argument that CREATE DATABASE will see. Compare $ createdb --strategy="foo bar" mydb createdb: error: database creation failed: ERROR: invalid create database strategy "foo bar" HINT: Valid strategies are "wal_log", and "file_copy". $ createdb --locale-provider="foo bar" mydb createdb: error: database creation failed: ERROR: syntax error at or near ";" LINE 1: CREATE DATABASE mydb LOCALE_PROVIDER foo bar; ^ I'm not suggesting that this is an interesting security vulnerability, because if you can control the arguments to createdb it's probably game over long since. But wrapping the arguments is good for delivering on-point error messages. So I'd add a fmtId() call to LOCALE_PROVIDER too. BTW, someone's taken the Oxford comma too far in that HINT. Nobody writes a comma when there are only two alternatives. regards, tom lane
Re: createdb compares strategy as case-sensitive
On 4/20/24 22:40, Tom Lane wrote: > Tomas Vondra writes: >> While doing some testing with createdb, I noticed it only accepts >> file_copy/wal_log as valid strategies, not FILE_COPY/WAL_LOG (which is >> what the docs say). The same thing applies to CREATE DATABASE. > > Hmm, actually it does work in CREATE DATABASE: > > regression=# create database foo STRATEGY = FILE_COPY; > CREATE DATABASE > > but it fails in createdb because that does > > if (strategy) > appendPQExpBuffer(, " STRATEGY %s", fmtId(strategy)); > > and fmtId will double-quote the strategy if it's upper-case, and then > the backend grammar doesn't case-fold it, and kaboom. > Oh, right. I should have tested CREATE DATABASE instead of just assuming it has the same issue ... >> The problem is that createdb() does the check using strcmp() which is >> case-sensitive. IMHO this should do pg_strcasecmp() which is what we do >> for other string parameters nearby. > > Seems reasonable. The alternative could be to remove createdb.c's use > of fmtId() here, but I don't think that's actually better. > Why? It seems to me this is quite close to e.g. LOCALE_PROVIDER, and we don't do fmtId() for that. So why should we do that for STRATEGY? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: createdb compares strategy as case-sensitive
Tomas Vondra writes: > While doing some testing with createdb, I noticed it only accepts > file_copy/wal_log as valid strategies, not FILE_COPY/WAL_LOG (which is > what the docs say). The same thing applies to CREATE DATABASE. Hmm, actually it does work in CREATE DATABASE: regression=# create database foo STRATEGY = FILE_COPY; CREATE DATABASE but it fails in createdb because that does if (strategy) appendPQExpBuffer(, " STRATEGY %s", fmtId(strategy)); and fmtId will double-quote the strategy if it's upper-case, and then the backend grammar doesn't case-fold it, and kaboom. > The problem is that createdb() does the check using strcmp() which is > case-sensitive. IMHO this should do pg_strcasecmp() which is what we do > for other string parameters nearby. Seems reasonable. The alternative could be to remove createdb.c's use of fmtId() here, but I don't think that's actually better. regards, tom lane
