Sutou Kouhei писал(а) 2025-02-04 13:29:
Hi
Hi,
In <d838025aceeb19c9ff1db702fa55c...@postgrespro.ru>
"Re: Make COPY format extendable: Extract COPY TO format
implementations" on Mon, 03 Feb 2025 13:38:04 +0700,
Vladlen Popolitov <v.popoli...@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.
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.
it assumes some SetOptions interface, that defines
an options structure according to the new method requirements.
Sorry. I couldn't find the SetOptions interface in source
code. I found only AT_SetOptions. Did you mean it by "some
SetOptions interface"?
Yes.
I'm familiar with only access method. It has
IndexAmRoutine::amoptions. Is it a SetOptions interface
example?
Yes. I think, it would be compatible with other modules
of source code and could use the same code base to process
options of COPY TO/FROM
FYI: The current patch set doesn't have custom options
support yet. Because we want to start from a minimal feature
set. But we'll add support for custom options eventually.
Sorry for disturbing. I did not have intention to stop your
patch, I would like to point to that details as early as
possible.
--
Best regards,
Vladlen Popolitov.
