Re: Make COPY format extendable: Extract COPY TO format implementations

On Tue, Feb 4, 2025 at 2:46 AM Vladlen Popolitov
<v.popolitov@postgrespro.ru> wrote:
>
> Sutou Kouhei писал(а) 2025-02-04 13:29:
> Hi
> > Hi,
> >
> > In <d838025aceeb19c9ff1db702fa55cabf@postgrespro.ru>
> >   "Re: Make COPY format extendable: Extract COPY TO format
> > implementations" on Mon, 03 Feb 2025 13:38:04 +0700,
> >   Vladlen Popolitov <v.popolitov@postgrespro.ru> wrote:
> >
> >> I would like to inform about the security breach in your design of
> >> COPY TO/FROM.
> >
> > Thanks! I didn't notice it.
> >
> >> You use FORMAT option to add new formats, filling it with routine name
> >> in shared library. As result any caller can call any routine in
> >> PostgreSQL kernel.
> >
> > We require "FORMAT_NAME(internal)" signature:
> >
> > ----
> >       funcargtypes[0] = INTERNALOID;
> >       handlerOid = LookupFuncName(list_make1(makeString(format)), 1,
> >                                                               funcargtypes, true);
> > ----
> >
> > So any caller can call only routines that use the signature.
> >
> > Should we add more checks for security? If so, what checks
> > are needed?
> >
> > For example, does requiring a prefix such as "copy_" (use
> > "copy_json" for "json" format) improve security?
> >
> > For example, we need to register a handler explicitly
> > (CREATE ACCESS METHOD) when we want to use a new access
> > method. Should we require an explicit registration for
> > custom COPY format too?
> >
> >
>
> I think, in case of USING PostgreSQL kernel will call corresponding
> handler,
> and it looks secure - the same as for table and index methods handlers.

IIUC even with custom copy format patches, we call the corresponding
handler function to get the routines, which is essentially similar to
what we do for table AM, index AM, and tablesample.I don't think we
allow users to call any routine in PostgreSQL core via custom FORMAT
option.

BTW we need to check if the return value type of the handler function
is copy_handler.

>
> >>  Standard PostgreSQL realisation for new methods to use USING
> >>  keyword. Every
> >> new method could have own options (FORMAT is option of internal 'copy
> >> from/to'
> >> methods),
> >
> > Ah, I didn't think about USING.
> >
> > You suggest "COPY ... USING json" not "COPY ... FORMAT json"
> > like "CREATE INDEX ... USING custom_index", right? It will
> > work. If we use this interface, we should reject "COPY
> > ... FORMAT ... USING" (both of FORMAT/USING are specified).
> >
> >
> I cannot recommend about rejecting, I do not know details
> of realisation of this part of code. Just idea - FORMAT value
> could be additional option to copy handler or NULL
> if it is omitted.
>   If you add extensibility, than every handler will be the
> extension, that can handle one or more formats.

Hmm, if we use the USING clause to specify the format type, we end up
having two ways to specify the format type (e.g., 'COPY ... USING
text' and 'COPY .. WITH (format = text)'), which seems to confuse
users.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com