public inbox for [email protected]  

help / color / mirror / Atom feed

From: Vladlen Popolitov <[email protected]>
To: Sutou Kouhei <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
Date: Tue, 04 Feb 2025 17:46:10 +0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CAD21AoA3KMddnjxY1hxth3f4f1wo8a8i2icgK6GEKqXNR_e6jA@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Sutou Kouhei писал(а) 2025-02-04 13:29:
Hi
> Hi,
> 
> In <[email protected]>
>   "Re: Make COPY format extendable: Extract COPY TO format 
> implementations" on Mon, 03 Feb 2025 13:38:04 +0700,
>   Vladlen Popolitov <[email protected]> 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.

view thread (14+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox