MIME-Version: 1.0
In-Reply-To: 
 <CALdSSPjBpUfY=S2i_3ACqF7YUJ=po1TDwYnDPDx38=j8LKXj7g@mail.gmail.com>
References: <19478-37289e8b0d1a1299@postgresql.org>
 <SY7PR01MB1092112D26F767633CF783E88B6052@SY7PR01MB10921.ausprd01.prod.outlook.com>
 <CALdSSPjBpUfY=S2i_3ACqF7YUJ=po1TDwYnDPDx38=j8LKXj7g@mail.gmail.com>
From: "David G. Johnston" <david.g.johnston@gmail.com>
Date: Fri, 15 May 2026 21:28:56 -0700
Message-ID: 
 <CAKFQuwYHJEUrGCyMoCnZFV9CCtCBMp0dTTRxEuCTW2RZMLq4Tw@mail.gmail.com>
Subject: Re: BUG #19478: `dblink_close` can be used for injection.
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: Japin Li <japinli@hotmail.com>,
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>,
 zengman <zengman@halodbtech.com>
Content-Type: multipart/alternative; boundary="0000000000008985240651e7c4da"
Archived-At: 
 <https://www.postgresql.org/message-id/CAKFQuwYHJEUrGCyMoCnZFV9CCtCBMp0dTTRxEuCTW2RZMLq4Tw%40mail.gmail.com>
Precedence: bulk

--0000000000008985240651e7c4da
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Friday, May 15, 2026, Kirill Reshke <reshkekirill@gmail.com> wrote:

>
>
> On Sat, 16 May 2026, 06:24 Japin Li, <japinli@hotmail.com> wrote:
>
>> On Fri, 15 May 2026 at 01:29, PG Bug reporting form <
>> noreply@postgresql.org> wrote:
>> > The following bug has been logged on the website:
>> >
>> > Bug reference:      19478
>> > Logged by:          Man Zeng
>> > Email address:      zengman@halodbtech.com
>> > PostgreSQL version: 18.4
>> > Operating system:   24.04.1-Ubuntu
>> > Description:
>> >
>> >
>> >
>> > -       appendStringInfo(&buf, "CLOSE %s", curname);
>> > +       appendStringInfo(&buf, "CLOSE %s", quote_ident_cstr(curname));
>> >
>>
>>
>> According to the documentation [1], it should be a cursor name.  Wrappin=
g
>> it
>> in quotes can prevent attacks like SQL injection.  I think your
>> modification
>> is correct, and we should add test cases for it.
>>
>> [1] https://www.postgresql.org/docs/current/contrib-dblink-close.html
>>
>
> Well, is there any actual injection? I mean, if user can execute
>> dblink_close, then user can do an SQL with dblink_open and simply do a S=
QL?
>> Unless wierd case when we only granted with close function, I guess
>>
>
Switching to quote_ident means we no longer lowercase an unquoted input.
Is this improvement in api design worth the potential breakage?  If so,
make sure we at least change the dblink_open (and fetch=E2=80=A6) code simi=
larly.

I=E2=80=99m disinclined to change this unless it=E2=80=99s shown the only p=
ossible use of
the identifier is within the dblink function arguments where can change all
uses to quote_identifier.  Even then, inconsistent capitalization still
might exist.

David J.

--0000000000008985240651e7c4da
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Friday, May 15, 2026, Kirill Reshke &lt;<a href=3D"mailto:reshkekirill@g=
mail.com">reshkekirill@gmail.com</a>&gt; wrote:<br><blockquote class=3D"gma=
il_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-lef=
t:1ex"><div dir=3D"auto"><div><br><br><div class=3D"gmail_quote"><div dir=
=3D"ltr" class=3D"gmail_attr">On Sat, 16 May 2026, 06:24 Japin Li, &lt;<a h=
ref=3D"mailto:japinli@hotmail.com" target=3D"_blank">japinli@hotmail.com</a=
>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0 0=
 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, 15 May 2026 at=
 01:29, PG Bug reporting form &lt;<a href=3D"mailto:noreply@postgresql.org"=
 rel=3D"noreferrer" target=3D"_blank">noreply@postgresql.org</a>&gt; wrote:=
<br>
&gt; The following bug has been logged on the website:<br>
&gt;<br>
&gt; Bug reference:=C2=A0 =C2=A0 =C2=A0 19478<br>
&gt; Logged by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Man Zeng<br>
&gt; Email address:=C2=A0 =C2=A0 =C2=A0 <a href=3D"mailto:zengman@halodbtec=
h.com" rel=3D"noreferrer" target=3D"_blank">zengman@halodbtech.com</a><br>
&gt; PostgreSQL version: 18.4<br>
&gt; Operating system:=C2=A0 =C2=A024.04.1-Ubuntu<br>
&gt; Description:=C2=A0 =C2=A0 =C2=A0 =C2=A0 <br>
&gt;<br>
&gt;<br>
&gt;=C2=A0 <br>
&gt; -=C2=A0 =C2=A0 =C2=A0 =C2=A0appendStringInfo(&amp;buf, &quot;CLOSE %s&=
quot;, curname);<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0appendStringInfo(&amp;buf, &quot;CLOSE %s&=
quot;, quote_ident_cstr(curname));<br>
&gt;=C2=A0 <br><br>
<br>
According to the documentation [1], it should be a cursor name.=C2=A0 Wrapp=
ing it<br>
in quotes can prevent attacks like SQL injection.=C2=A0 I think your modifi=
cation<br>
is correct, and we should add test cases for it.<br>
<br>
[1] <a href=3D"https://www.postgresql.org/docs/current/contrib-dblink-close=
.html" rel=3D"noreferrer noreferrer" target=3D"_blank">https://www.postgres=
ql.org/<wbr>docs/current/contrib-dblink-<wbr>close.html</a><br>=C2=A0</bloc=
kquote><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-=
left:1px #ccc solid;padding-left:1ex">Well, is there any actual injection? =
I mean, if user can execute dblink_close, then user can do an SQL with=C2=
=A0dblink_open and simply do a SQL? Unless wierd case when we only granted =
with close function, I guess<br></blockquote></div></div></div></blockquote=
><div><br></div><div>Switching to quote_ident means we no longer lowercase =
an unquoted input.=C2=A0 Is this improvement in api design worth the potent=
ial breakage?=C2=A0 If so, make sure we at least change the dblink_open (an=
d fetch=E2=80=A6) code similarly.</div><div><br></div><div>I=E2=80=99m disi=
nclined to change this unless it=E2=80=99s shown the only possible use of t=
he identifier is within the dblink function arguments where can change all =
uses to quote_identifier.=C2=A0 Even then, inconsistent capitalization stil=
l might exist.</div><div><br></div><div>David J.</div><div><br></div>

--0000000000008985240651e7c4da--