proposal: psql \setfileref

On Wed, Aug 31, 2016 at 11:32 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

I propose a new type of psql variables - file references. The content of
file reference is specified by referenced file. It allows simple inserting
large data without necessity of manual escaping or using LO api.

When I wrote the patch, I used parametrized queries for these data instead
escaped strings - the code is not much bigger, and the error messages are
much more friendly if query is not bloated by bigger content. The text mode
is used only - when escaping is not required, then content is implicitly
transformed to bytea. By default the content of file is bytea. When use
requires escaping, then he enforces text escaping - because it has sense
only for text type.

postgres=# \setfileref a ~/test2.xml
postgres=# \setfileref b ~/avatar.gif
postgres=# insert into test values(convert_from(:a, 'latin2')::xml, :b);
-- xml is passed as bytea
postgres=# insert into test values(:'a', :b); -- xml is passed via unknown
text value

The content of file reference variables is not persistent in memory.

Comments, notes?

Regards

Pavel

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Clearly jumping ahead on this one, but if the fileref is essentially a pipe
to "cat /path/to/file.name", is there anything stopping us from setting
pipes?
My interest is primarily in ways that COPY could use this.

2016-08-31 18:24 GMT+02:00 Corey Huinker <corey.huinker@gmail.com>:

On Wed, Aug 31, 2016 at 11:32 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

I propose a new type of psql variables - file references. The content of
file reference is specified by referenced file. It allows simple inserting
large data without necessity of manual escaping or using LO api.

When I wrote the patch, I used parametrized queries for these data
instead escaped strings - the code is not much bigger, and the error
messages are much more friendly if query is not bloated by bigger content.
The text mode is used only - when escaping is not required, then content is
implicitly transformed to bytea. By default the content of file is bytea.
When use requires escaping, then he enforces text escaping - because it has
sense only for text type.

postgres=# \setfileref a ~/test2.xml
postgres=# \setfileref b ~/avatar.gif
postgres=# insert into test values(convert_from(:a, 'latin2')::xml, :b);
-- xml is passed as bytea
postgres=# insert into test values(:'a', :b); -- xml is passed via
unknown text value

The content of file reference variables is not persistent in memory.

Comments, notes?

Regards

Pavel

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Clearly jumping ahead on this one, but if the fileref is essentially a
pipe to "cat /path/to/file.name", is there anything stopping us from
setting pipes?

My interest is primarily in ways that COPY could use this.

I don't see a reason why it should not be possible - the current code can't
do it, but with 20 lines more, it should be possible

There is one disadvantage against copy - the content should be fully loaded
to memory, but any other functionality should be same.

Regards

Pavel

Hi Pavel,

I just tried to apply your patch psql-setfileref-initial.patch (using git apply) to the newest revision of postgres at the time (da6c4f6ca88) and it failed to patch startup.c. Thinking that the patch was for some previous revision, I then checked out d062245b5, which was from 2016-08-31, and tried applying the patch from there. I had the same problem:

$ git apply psql-setfileref-initial.patch
error: patch failed: src/bin/psql/startup.c:106
error: src/bin/psql/startup.c: patch does not apply

However, when I applied the changes to startup.c manually and removed them from the patch, the rest of the patch applied without a problem. I don't know if I may have done something wrong in trying to apply the patch, but you may want to double check if you need to regenerate your patch from the latest revision so it will apply smoothly for reviewers.

In the meantime, I haven't had a chance to try out the fileref feature yet but I'll give it a go when I get a chance!

Thanks!
Ryan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-09-26 14:57 GMT+02:00 Ryan Murphy <ryanfmurphy@gmail.com>:

Hi Pavel,

I just tried to apply your patch psql-setfileref-initial.patch (using git
apply) to the newest revision of postgres at the time (da6c4f6ca88) and it
failed to patch startup.c. Thinking that the patch was for some previous
revision, I then checked out d062245b5, which was from 2016-08-31, and
tried applying the patch from there. I had the same problem:

$ git apply psql-setfileref-initial.patch
error: patch failed: src/bin/psql/startup.c:106
error: src/bin/psql/startup.c: patch does not apply

However, when I applied the changes to startup.c manually and removed them
from the patch, the rest of the patch applied without a problem. I don't
know if I may have done something wrong in trying to apply the patch, but
you may want to double check if you need to regenerate your patch from the
latest revision so it will apply smoothly for reviewers.

Thank you for info. I'll do it immediately.

Regards

Pavel

Show quoted text

In the meantime, I haven't had a chance to try out the fileref feature yet
but I'll give it a go when I get a chance!

Thanks!
Ryan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi

2016-09-26 14:57 GMT+02:00 Ryan Murphy <ryanfmurphy@gmail.com>:

Hi Pavel,

I just tried to apply your patch psql-setfileref-initial.patch (using git
apply) to the newest revision of postgres at the time (da6c4f6ca88) and it
failed to patch startup.c. Thinking that the patch was for some previous
revision, I then checked out d062245b5, which was from 2016-08-31, and
tried applying the patch from there. I had the same problem:

$ git apply psql-setfileref-initial.patch
error: patch failed: src/bin/psql/startup.c:106
error: src/bin/psql/startup.c: patch does not apply

However, when I applied the changes to startup.c manually and removed them
from the patch, the rest of the patch applied without a problem. I don't
know if I may have done something wrong in trying to apply the patch, but
you may want to double check if you need to regenerate your patch from the
latest revision so it will apply smoothly for reviewers.

please, can you check attached patch? It worked in my laptop.

Regards

Pavel

Show quoted text

In the meantime, I haven't had a chance to try out the fileref feature yet
but I'll give it a go when I get a chance!

Thanks!
Ryan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

please, can you check attached patch? It worked in my laptop.

Regards

Pavel

Yes, that one applied for me without any problems.

Thanks,
Ryan

2016-09-26 21:47 GMT+02:00 Ryan Murphy <ryanfmurphy@gmail.com>:

please, can you check attached patch? It worked in my laptop.

Regards

Pavel

Yes, that one applied for me without any problems.

Great,

Thank you

Pavel

Show quoted text

Thanks,
Ryan

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, failed
Spec compliant: tested, failed
Documentation: not tested

Contents & Purpose
==================

This patch adds a new type of psql variables: file references, that can be
set using command \setfileref. The value of the named variable is the path
to the referenced file. It allows simple inserting of large data without
necessity of manual escaping or using LO api. Use:

postgres=# create table test (col1 bytea);
postgres=# \setfileref a ~/avatar.gif
postgres=# insert into test values(:a);

Content of file is returned as bytea, the text output can be used when
escaping is not required, in this case use convert_from() to obtain a
text output.

postgres=# create table test (col1 text);
postgres=# \setfileref a ~/README.txt
postgres=# insert into test values(convert_from(:a, 'utf8'));

The content of file reference variables is not persistent in memory.

List of file referenced variable can be listed using \set

postgres=# \set
...
b = ^'~/README.txt'

Empty file outputs an empty bytea (\x).

Initial Run
===========

The patch applies cleanly to HEAD and doesn't break anything, at least the
regression tests all pass successfully.

Feedback on testing
===============

I see several problems.

1) Error reading referenced file returns the system error and a syntax error
on variable:

postgres=# \setfileref a /etc/sudoers
postgres=# insert into test values (:a);
/etc/sudoers: Permission denied
ERROR: syntax error at or near ":"
LINE 1: insert into t1 values (:a);

2) Trying to load a file with size upper than the 1GB limit reports the following
error (size 2254110720 bytes):

postgres=# \setfileref b /home/VMs/ISOs/sol-10-u11-ga-x86-dvd.iso
postgres=# insert into test values (:b);
INSERT 0 1
postgres=# ANALYZE test;
postgres=# SELECT relname, relkind, relpages, pg_size_pretty(relpages::bigint*8192) FROM pg_class WHERE relname='test';
relname | relkind | relpages | pg_size_pretty
---------+---------+----------+----------------
test | r | 1 | 8192 bytes
(1 row)

postgres=# select * from test;
col1
------
\x
(1 row)

This should not insert an empty bytea but might raise an error instead.

Trying to load smaller file but with bytea escaping total size upper than
the 1GB limit (size 666894336 bytes) reports:

postgres=# \setfileref a /var/ISOs/CentOS-7-x86_64-Minimal-1503-01.iso
postgres=# insert into t1 values (1, :a, 'tr');
ERROR: out of memory
DETAIL: Cannot enlarge string buffer containing 0 bytes by 1333788688 more bytes.
Time: 1428.657 ms (00:01.429)

This raise an error as bytea escaping increase content size which is the behavior expected.

3) The path doesn't not allow the use of pipe to system command, which is a secure
behavior, but it is quite easy to perform a DOS by using special files like:

postgres=# \setfileref b /dev/random
postgres=# insert into t1 (:b);.

this will never end until we kill the psql session. I think we must also prevent
non regular files to be referenced using S_ISREG.

I think all these three errors can be caught and prevented at variable declaration using some tests on files like:

is readable?
is a regular file?
is small size enough?

to report an error directly at variable file reference setting.

4) An other problem is that like this this patch will allow anyone to upload into a
column the content of any system file that can be read by postgres system user
and then allow non system user to read its content. For example, connected as
a basic PostgreSQL only user:

testdb=> select current_user;
current_user
--------------
user1
(1 row)
testdb=> \setfileref b /etc/passwd
testdb=> insert into test values (:b);
INSERT 0 1

then to read the file:

testdb=> select convert_from(col1, 'utf8') from t1;

Maybe the referenced files must be limited to some part of the filesystem
or the feature limited to superuser only.

5) There is no documentation at all on this feature. Here a proposal
for inclusion in doc/src/sgml/ref/psql-ref.sgml

\setfileref name value
Sets the internal variable name as a reference to the file path
set as value. To unset a variable, use the \unset command.

File references are shown as file path prefixed with the ^ character
when using the \set command alone.

Valid variable names can contain characters, digits, and underscores.
See the section Variables below for details. Variable names are
case-sensitive.

More detail here about file size, file privilege, etc following
what will be decided.

6) I would also like to see \setfileref display all file references variables
defined instead of message "missing required argument". Just like \set and
\pset alone are working. \set can still show the file references variables, that's
not a problem for me.

The new status of this patch is: Waiting on Author

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net> wrote:

4) An other problem is that like this this patch will allow anyone to upload into a
column the content of any system file that can be read by postgres system user
and then allow non system user to read its content.

I thought this was a client-side feature, so that it would let a
client upload any file that the client can read, but not things that
can only be read by the postgres system user.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le 03/10/2016 à 23:03, Robert Haas a écrit :

On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net> wrote:

4) An other problem is that like this this patch will allow anyone to upload into a
column the content of any system file that can be read by postgres system user
and then allow non system user to read its content.

I thought this was a client-side feature, so that it would let a
client upload any file that the client can read, but not things that
can only be read by the postgres system user.

Yes that's right, sorry for the noise, forget this fourth report.

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le 03/10/2016 à 23:23, Gilles Darold a écrit :

Le 03/10/2016 à 23:03, Robert Haas a écrit :

On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net> wrote:

4) An other problem is that like this this patch will allow anyone to upload into a
column the content of any system file that can be read by postgres system user
and then allow non system user to read its content.

I thought this was a client-side feature, so that it would let a
client upload any file that the client can read, but not things that
can only be read by the postgres system user.

Yes that's right, sorry for the noise, forget this fourth report.

After some more though there is still a security issue here. For a
PostgreSQL user who also have login acces to the server, it is possible
to read any file that the postgres system user can read, especially a
.pgpass or a recovery.conf containing password.

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-10-04 9:18 GMT+02:00 Gilles Darold <gilles.darold@dalibo.com>:

Le 03/10/2016 à 23:23, Gilles Darold a écrit :

Le 03/10/2016 à 23:03, Robert Haas a écrit :

On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net>

wrote:

4) An other problem is that like this this patch will allow anyone to

upload into a

column the content of any system file that can be read by postgres

system user

and then allow non system user to read its content.

I thought this was a client-side feature, so that it would let a
client upload any file that the client can read, but not things that
can only be read by the postgres system user.

Yes that's right, sorry for the noise, forget this fourth report.

After some more though there is still a security issue here. For a
PostgreSQL user who also have login acces to the server, it is possible
to read any file that the postgres system user can read, especially a
.pgpass or a recovery.conf containing password.

This patch doesn't introduce any new server side functionality, so if there
is some vulnerability, then it is exists now too.

Regards

Pavel

Show quoted text

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Le 04/10/2016 à 17:29, Pavel Stehule a écrit :

2016-10-04 9:18 GMT+02:00 Gilles Darold <gilles.darold@dalibo.com
<mailto:gilles.darold@dalibo.com>>:

Le 03/10/2016 à 23:23, Gilles Darold a écrit :

Le 03/10/2016 à 23:03, Robert Haas a écrit :

On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold

<gilles@darold.net <mailto:gilles@darold.net>> wrote:

4) An other problem is that like this this patch will allow

anyone to upload into a

column the content of any system file that can be read by

postgres system user

and then allow non system user to read its content.

I thought this was a client-side feature, so that it would let a
client upload any file that the client can read, but not things

that

can only be read by the postgres system user.

Yes that's right, sorry for the noise, forget this fourth report.

After some more though there is still a security issue here. For a
PostgreSQL user who also have login acces to the server, it is
possible
to read any file that the postgres system user can read, especially a
.pgpass or a recovery.conf containing password.

This patch doesn't introduce any new server side functionality, so if
there is some vulnerability, then it is exists now too.

It doesn't exists, that was my system user which have extended
privilege. You can definitively forget the fouth point.

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

hi

2016-10-04 9:18 GMT+02:00 Gilles Darold <gilles.darold@dalibo.com>:

Le 03/10/2016 à 23:23, Gilles Darold a écrit :

Le 03/10/2016 à 23:03, Robert Haas a écrit :

On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net>

wrote:

4) An other problem is that like this this patch will allow anyone to

upload into a

column the content of any system file that can be read by postgres

system user

and then allow non system user to read its content.

I thought this was a client-side feature, so that it would let a
client upload any file that the client can read, but not things that
can only be read by the postgres system user.

Yes that's right, sorry for the noise, forget this fourth report.

After some more though there is still a security issue here. For a
PostgreSQL user who also have login acces to the server, it is possible
to read any file that the postgres system user can read, especially a
.pgpass or a recovery.conf containing password.

here is new update - some mentioned issues are fixed + regress tests and
docs

regards

Pavel

Show quoted text

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

here is new update - some mentioned issues are fixed + regress tests and

docs

This might tie into some work I'm doing. Is there any way these filerefs
could be used as the inline portion of a COPY command?
i.e. like this:

COPY my_table FROM STDIN
:file_ref
\.

2016-10-09 17:27 GMT+02:00 Corey Huinker <corey.huinker@gmail.com>:

here is new update - some mentioned issues are fixed + regress tests and

docs

This might tie into some work I'm doing. Is there any way these filerefs
could be used as the inline portion of a COPY command?
i.e. like this:

COPY my_table FROM STDIN
:file_ref
\.

I understand, but I am not sure how difficult implementation it is. This
part (COPY input) doesn't support parametrization - and parametrization can
have a negative performance impact.

Regards

Pavel

On Sun, Oct 9, 2016 at 12:12 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2016-10-09 17:27 GMT+02:00 Corey Huinker <corey.huinker@gmail.com>:

here is new update - some mentioned issues are fixed + regress tests and

docs

This might tie into some work I'm doing. Is there any way these filerefs
could be used as the inline portion of a COPY command?
i.e. like this:

COPY my_table FROM STDIN
:file_ref
\.

I understand, but I am not sure how difficult implementation it is. This
part (COPY input) doesn't support parametrization - and parametrization can
have a negative performance impact.

Regards

I may not understand your response. I was thinking we'd want :file_ref to
simply 'cat' the file (or program) in place. The fact that the output was
consumed by COPY was coincidental. Does that chance your response?

2016-10-09 19:14 GMT+02:00 Corey Huinker <corey.huinker@gmail.com>:

On Sun, Oct 9, 2016 at 12:12 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2016-10-09 17:27 GMT+02:00 Corey Huinker <corey.huinker@gmail.com>:

here is new update - some mentioned issues are fixed + regress tests and

docs

This might tie into some work I'm doing. Is there any way these filerefs
could be used as the inline portion of a COPY command?
i.e. like this:

COPY my_table FROM STDIN
:file_ref
\.

I understand, but I am not sure how difficult implementation it is. This
part (COPY input) doesn't support parametrization - and parametrization can
have a negative performance impact.

Regards

I may not understand your response. I was thinking we'd want :file_ref to
simply 'cat' the file (or program) in place. The fact that the output was
consumed by COPY was coincidental. Does that chance your response?

look to code - some parts allows psql session variables, some not - I can
use variables in statements - not in data block.

More, there is ambiguity - :xxx should not be part of SQL statement (and
then psql variables are possible), but :xxxx should be part of data.

Regards

Pavel

Show quoted text

look to code - some parts allows psql session variables, some not - I can
use variables in statements - not in data block.

More, there is ambiguity - :xxx should not be part of SQL statement (and
then psql variables are possible), but :xxxx should be part of data.

Regards

Pavel

Makes sense, thanks for clearing it up.