public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 01/10] Allow alternate compression methods for wal_compression
4+ messages / 3 participants
[nested] [flat]
* [PATCH 01/10] Allow alternate compression methods for wal_compression
@ 2021-02-27 04:03 Andrey Borodin <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Andrey Borodin @ 2021-02-27 04:03 UTC (permalink / raw)
TODO: bump XLOG_PAGE_MAGIC
---
doc/src/sgml/config.sgml | 17 +++++
src/backend/Makefile | 2 +-
src/backend/access/transam/xlog.c | 10 +++
src/backend/access/transam/xloginsert.c | 52 +++++++++++++--
src/backend/access/transam/xlogreader.c | 63 ++++++++++++++++++-
src/backend/utils/misc/guc.c | 11 ++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/access/xlog.h | 1 +
src/include/access/xlog_internal.h | 8 +++
src/include/access/xlogreader.h | 1 +
src/include/access/xlogrecord.h | 9 +--
11 files changed, 163 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index a218d78bef..7fb2a84626 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3072,6 +3072,23 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-wal-compression-method" xreflabel="wal_compression_method">
+ <term><varname>wal_compressionion_method</varname> (<type>enum</type>)
+ <indexterm>
+ <primary><varname>wal_compression_method</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This parameter selects the compression method used to compress WAL when
+ <varname>wal_compression</varname> is enabled.
+ The supported methods are pglz and zlib.
+ The default value is <literal>pglz</literal>.
+ Only superusers can change this setting.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-wal-init-zero" xreflabel="wal_init_zero">
<term><varname>wal_init_zero</varname> (<type>boolean</type>)
<indexterm>
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 0da848b1fd..3af216ddfc 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -48,7 +48,7 @@ OBJS = \
LIBS := $(filter-out -lpgport -lpgcommon, $(LIBS)) $(LDAP_LIBS_BE) $(ICU_LIBS)
# The backend doesn't need everything that's in LIBS, however
-LIBS := $(filter-out -lz -lreadline -ledit -ltermcap -lncurses -lcurses, $(LIBS))
+LIBS := $(filter-out -lreadline -ledit -ltermcap -lncurses -lcurses, $(LIBS))
ifeq ($(with_systemd),yes)
LIBS += -lsystemd
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f4d1ce5dea..15da91a8dd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -99,6 +99,7 @@ bool EnableHotStandby = false;
bool fullPageWrites = true;
bool wal_log_hints = false;
bool wal_compression = false;
+int wal_compression_method = WAL_COMPRESSION_PGLZ;
char *wal_consistency_checking_string = NULL;
bool *wal_consistency_checking = NULL;
bool wal_init_zero = true;
@@ -180,6 +181,15 @@ const struct config_enum_entry recovery_target_action_options[] = {
{NULL, 0, false}
};
+/* Note that due to conditional compilation, offsets within the array are not static */
+const struct config_enum_entry wal_compression_options[] = {
+ {"pglz", WAL_COMPRESSION_PGLZ, false},
+#ifdef HAVE_LIBZ
+ {"zlib", WAL_COMPRESSION_ZLIB, false},
+#endif
+ {NULL, 0, false}
+};
+
/*
* Statistics for current checkpoint are collected in this global struct.
* Because only the checkpointer or a stand-alone backend can perform
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 7052dc245e..34e1227381 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -33,6 +33,10 @@
#include "storage/proc.h"
#include "utils/memutils.h"
+#ifdef HAVE_LIBZ
+#include <zlib.h>
+#endif
+
/* Buffer size required to store a compressed version of backup block image */
#define PGLZ_MAX_BLCKSZ PGLZ_MAX_OUTPUT(BLCKSZ)
@@ -113,7 +117,8 @@ static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info,
XLogRecPtr RedoRecPtr, bool doPageWrites,
XLogRecPtr *fpw_lsn, int *num_fpi);
static bool XLogCompressBackupBlock(char *page, uint16 hole_offset,
- uint16 hole_length, char *dest, uint16 *dlen);
+ uint16 hole_length, char *dest,
+ uint16 *dlen, WalCompression compression);
/*
* Begin constructing a WAL record. This must be called before the
@@ -630,11 +635,12 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
*/
if (wal_compression)
{
+ bimg.compression_method = wal_compression_method;
is_compressed =
XLogCompressBackupBlock(page, bimg.hole_offset,
cbimg.hole_length,
regbuf->compressed_page,
- &compressed_len);
+ &compressed_len, bimg.compression_method);
}
/*
@@ -827,7 +833,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
*/
static bool
XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length,
- char *dest, uint16 *dlen)
+ char *dest, uint16 *dlen, WalCompression compression)
{
int32 orig_len = BLCKSZ - hole_length;
int32 len;
@@ -853,12 +859,48 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length,
else
source = page;
+ switch (compression)
+ {
+ case WAL_COMPRESSION_PGLZ:
+ len = pglz_compress(source, orig_len, dest, PGLZ_strategy_default);
+ break;
+
+#ifdef HAVE_LIBZ
+ case WAL_COMPRESSION_ZLIB:
+ {
+ unsigned long len_l = PGLZ_MAX_BLCKSZ;
+ int ret;
+ ret = compress2((Bytef*)dest, &len_l, (Bytef*)source, orig_len, 1);
+ if (ret != Z_OK)
+ {
+ // XXX: using an interface other than compress() would allow giving a better error message
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("failed compressing zlib (%d)", ret)));
+ len_l = -1;
+ }
+ len = len_l;
+ break;
+ }
+#endif
+
+ default:
+ /*
+ * It should be impossible to get here for unsupported algorithms,
+ * which cannot be assigned if they're not enabled at compile time.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("unknown compression method requested: %d(%s)",
+ compression, wal_compression_name(compression))));
+
+ }
+
/*
- * We recheck the actual size even if pglz_compress() reports success and
+ * We recheck the actual size even if compression reports success and
* see if the number of bytes saved by compression is larger than the
* length of extra data needed for the compressed version of block image.
*/
- len = pglz_compress(source, orig_len, dest, PGLZ_strategy_default);
if (len >= 0 &&
len + extra_bytes < orig_len)
{
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 42738eb940..afca22a26c 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -33,6 +33,10 @@
#include "utils/memutils.h"
#endif
+#ifdef HAVE_LIBZ
+#include <zlib.h>
+#endif
+
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
@@ -1286,6 +1290,7 @@ DecodeXLogRecord(XLogReaderState *state, XLogRecord *record, char **errormsg)
{
COPY_HEADER_FIELD(&blk->bimg_len, sizeof(uint16));
COPY_HEADER_FIELD(&blk->hole_offset, sizeof(uint16));
+ COPY_HEADER_FIELD(&blk->compression_method, sizeof(uint8));
COPY_HEADER_FIELD(&blk->bimg_info, sizeof(uint8));
blk->apply_image = ((blk->bimg_info & BKPIMAGE_APPLY) != 0);
@@ -1535,6 +1540,29 @@ XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len)
}
}
+/*
+ * Return a statically allocated string associated with the given compression
+ * method. This is similar to the guc, but isn't subject to conditional
+ * compilation.
+ */
+const char *
+wal_compression_name(WalCompression compression)
+{
+ /*
+ * This could index into the guc array, except that it's compiled
+ * conditionally and unsupported methods are elided.
+ */
+ switch (compression)
+ {
+ case WAL_COMPRESSION_PGLZ:
+ return "pglz";
+ case WAL_COMPRESSION_ZLIB:
+ return "zlib";
+ default:
+ return "???";
+ }
+}
+
/*
* Restore a full-page image from a backup block attached to an XLOG record.
*
@@ -1558,8 +1586,39 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
if (bkpb->bimg_info & BKPIMAGE_IS_COMPRESSED)
{
/* If a backup block image is compressed, decompress it */
- if (pglz_decompress(ptr, bkpb->bimg_len, tmp.data,
- BLCKSZ - bkpb->hole_length, true) < 0)
+ int32 decomp_result = -1;
+ switch (bkpb->compression_method)
+ {
+ case WAL_COMPRESSION_PGLZ:
+ decomp_result = pglz_decompress(ptr, bkpb->bimg_len, tmp.data,
+ BLCKSZ - bkpb->hole_length, true);
+ break;
+
+#ifdef HAVE_LIBZ
+ case WAL_COMPRESSION_ZLIB:
+ {
+ unsigned long decomp_result_l;
+ decomp_result_l = BLCKSZ - bkpb->hole_length;
+ if (uncompress((Bytef*)tmp.data, &decomp_result_l,
+ (Bytef*)ptr, bkpb->bimg_len) == Z_OK)
+ decomp_result = decomp_result_l;
+ else
+ decomp_result = -1;
+ break;
+ }
+#endif
+
+ default:
+ report_invalid_record(record, "image at %X/%X is compressed with unsupported codec, block %d (%d/%s)",
+ (uint32) (record->ReadRecPtr >> 32),
+ (uint32) record->ReadRecPtr,
+ block_id,
+ bkpb->compression_method,
+ wal_compression_name(bkpb->compression_method));
+ return false;
+ }
+
+ if (decomp_result < 0)
{
report_invalid_record(record, "invalid compressed image at %X/%X, block %d",
LSN_FORMAT_ARGS(record->ReadRecPtr),
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 855076b1fd..8084027465 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -508,6 +508,7 @@ extern const struct config_enum_entry archive_mode_options[];
extern const struct config_enum_entry recovery_target_action_options[];
extern const struct config_enum_entry sync_method_options[];
extern const struct config_enum_entry dynamic_shared_memory_options[];
+extern const struct config_enum_entry wal_compression_options[];
/*
* GUC option variables that are exported from this module
@@ -4721,6 +4722,16 @@ static struct config_enum ConfigureNamesEnum[] =
NULL, NULL, NULL
},
+ {
+ {"wal_compression_method", PGC_SIGHUP, WAL_SETTINGS,
+ gettext_noop("Set the method used to compress full page images in the WAL."),
+ NULL
+ },
+ &wal_compression_method,
+ WAL_COMPRESSION_PGLZ, wal_compression_options,
+ NULL, NULL, NULL
+ },
+
{
{"dynamic_shared_memory_type", PGC_POSTMASTER, RESOURCES_MEM,
gettext_noop("Selects the dynamic shared memory implementation used."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index f46c2dd7a8..ef69a94492 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -213,6 +213,7 @@
# open_sync
#full_page_writes = on # recover from partial page writes
#wal_compression = off # enable compression of full-page writes
+#wal_compression_method = pglz # pglz, zlib
#wal_log_hints = off # also do full page writes of non-critical updates
# (change requires restart)
#wal_init_zero = on # zero-fill new WAL files
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 6d384d3ce6..fa2e5c611f 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -117,6 +117,7 @@ extern bool EnableHotStandby;
extern bool fullPageWrites;
extern bool wal_log_hints;
extern bool wal_compression;
+extern int wal_compression_method;
extern bool wal_init_zero;
extern bool wal_recycle;
extern bool *wal_consistency_checking;
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index b23e286406..d653839b97 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -324,4 +324,12 @@ extern bool InArchiveRecovery;
extern bool StandbyMode;
extern char *recoveryRestoreCommand;
+typedef enum WalCompression
+{
+ WAL_COMPRESSION_PGLZ,
+ WAL_COMPRESSION_ZLIB,
+} WalCompression;
+
+extern const char *wal_compression_name(WalCompression compression);
+
#endif /* XLOG_INTERNAL_H */
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 21d200d3df..3d19c315d7 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -133,6 +133,7 @@ typedef struct
bool apply_image; /* has image that should be restored */
char *bkp_image;
uint16 hole_offset;
+ uint8 compression_method;
uint16 hole_length;
uint16 bimg_len;
uint8 bimg_info;
diff --git a/src/include/access/xlogrecord.h b/src/include/access/xlogrecord.h
index 80c92a2498..0d4c212f15 100644
--- a/src/include/access/xlogrecord.h
+++ b/src/include/access/xlogrecord.h
@@ -114,7 +114,7 @@ typedef struct XLogRecordBlockHeader
* present is (BLCKSZ - <length of "hole" bytes>).
*
* Additionally, when wal_compression is enabled, we will try to compress full
- * page images using the PGLZ compression algorithm, after removing the "hole".
+ * page images, after removing the "hole".
* This can reduce the WAL volume, but at some extra cost of CPU spent
* on the compression during WAL logging. In this case, since the "hole"
* length cannot be calculated by subtracting the number of page image bytes
@@ -129,9 +129,10 @@ typedef struct XLogRecordBlockHeader
*/
typedef struct XLogRecordBlockImageHeader
{
- uint16 length; /* number of page image bytes */
- uint16 hole_offset; /* number of bytes before "hole" */
- uint8 bimg_info; /* flag bits, see below */
+ uint16 length; /* number of page image bytes */
+ uint16 hole_offset; /* number of bytes before "hole" */
+ uint8 compression_method; /* compression method used for image */
+ uint8 bimg_info; /* flag bits, see below */
/*
* If BKPIMAGE_HAS_HOLE and BKPIMAGE_IS_COMPRESSED, an
--
2.17.0
--XsQoSWH+UP9D9v3l
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0002-Run-011_crash_recovery.pl-with-wal_level-minimal.patch"
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Blocking execution of SECURITY INVOKER
@ 2023-01-13 03:29 Andres Freund <[email protected]>
2023-01-13 03:38 ` Re: Blocking execution of SECURITY INVOKER Andres Freund <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Andres Freund @ 2023-01-13 03:29 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers
Hi,
On 2023-01-12 18:40:30 -0800, Jeff Davis wrote:
> On Wed, 2023-01-11 at 19:33 -0800, Andres Freund wrote:
>
> > and the
> > privilege check will be done with the rights of the admin in many of
> > these
> > contexts.
>
> Can you explain?
If the less-privileged user does *not* have execution rights to a security
definer function, but somehow can trick the more-privileged user into calling
the function for them, e.g. by using it as the default expression of a column,
the less-privileged user can escalate to the permissions of the security
definer function.
superuser:
# CREATE FUNCTION exec_su(p_sql text) RETURNS text LANGUAGE plpgsql SECURITY DEFINER AS $$BEGIN RAISE NOTICE 'executing %', p_sql; EXECUTE p_sql;RETURN 'p_sql';END;$$;
# REVOKE ALL ON FUNCTION exec_su FROM PUBLIC ;
unprivileged user:
$ SELECT exec_su('ALTER USER less_privs SUPERUSER');
ERROR: 42501: permission denied for function exec_su
$ CREATE TABLE trick_superuser(value text default exec_su('ALTER USER less_privs SUPERUSER'));
superuser:
# INSERT INTO trick_superuser DEFAULT VALUES;
NOTICE: 00000: executing ALTER USER less_privs SUPERUSER
This case would *not* be prevented by your proposed GUC, unless I miss
something major. The superuser trusts itself and thus the exec_su() function.
> > And encouraging more security definer functions to be used will cause
> > a lot of
> > other security issues.
>
> My proposal just gives some user foo a GUC to say "I am not accepting
> the risk of eval()ing whatever arbitrary code finds its way in front of
> me with all of my privileges".
>
> If user foo sheds this security burden by setting the GUC, user bar may
> then choose to write a trigger function as SECURITY DEFINER so that foo
> can access bar's table. But that's the deal the two users struck -- foo
> declined the burden, bar accepted it. Why do we want to prevent that
> arrangement?
Because it afaict doesn't provide any meaningfully increased security
guarantees (see above), and opens up new ways of attacking, because while
granting execute on a security definer function is low risk, granting execute
on security invoker functions is very high risk, but required for triggers etc
to work.
> Right now, foo *always* has the burden and no opportunity to decline
> it, and even a paranoid user can't figure out what code they will be
> executing with a given command. That doesn't seem reasonable to me.
I agree it's not reasonable - I just don't see the proposal moving the bar.
The proposal to not trust any expressions controlled by untrusted users at
least allows to prevent execution of code, even if it doesn't provide a way to
execute the code in a safe manner. Given that we don't have the former, it
seems foolish to shoot for the latter.
> > However - I think the concept of more strict ownership checks is a
> > good one. I
> > just don't think it's right to tie it to SECURITY INVOKER.
>
> Consider a canonical trigger example like:
> https://wiki.postgresql.org/wiki/Audit_trigger or
> https://github.com/2ndQuadrant/audit-trigger/blob/master/audit.sql
>
> How can we make that secure for users that insert into the table with
> the trigger if you don't differentiate between SECURITY INVOKER and
> SECURITY DEFINER? If you allow neither, then it obviously won't work.
> And if you allow both, then the owner of the table can change the
> function to SECURITY INVOKER and the definition to be malicious a
> millisecond before you insert a tuple.
As shown above, triggers are simply not a relevant boundary when a more
privileged user accesses a table controlled by a less privileged user.
And yes, of course an audit function needs to be security definer. But that's
independent of whether it's safe for a more privileged user modify table
contents.
> I guess we currently say that anyone foolish enough to insert into a
> table that they don't own deserves what they get.
I agree that we have a problem that we should address. I just don't think your
solution works.
> That's a weird thing to say when we have such a fine-grained GRANT system
> and RLS.
That's a non-sequitur imo. Particularly when RLS you'd not allow
less-privileged users to create any objects, with the possible exception of
temp tables. The point of the grant system is for a privileged user to safely
allow a less privileged user to perform a safe subset of actions. That's just
a separate angle than allowing safe access for a more privileged user to to
objects controlled by a less privileged user.
> > I think it'd be quite valuable to have a guc that prevents the
> > execution of
> > any code that's not directly controlled by the privileged user. Not
> > just
> > checking function ownership, but also checking ownership of the
> > trigger
> > definition (i.e. table), check constraints, domain constraints,
> > indexes with
> > expression columns / partial indexes, etc
>
> That sounds like a mix of my proposal and Noah's. The way you've
> phrased it seems overly strict though -- do you mean not even execute
> untrusted expressions? And it seems to cut out maintenance commands,
> which means it would be hard for administrators to use.
Yes, I mean every expression. As show above, as soon as there is *any*
expression controlled by a less privileged user is executed, the game is lost.
I don't think that prevents all maintenance btw - for things like reindex we
switch to the object owner for evaluation via SetUserIdAndSecContext(). After
checking whether the current user is allowed to that kind of thing.
But for things like default expressions, generated columns etc, I just don't
see an alternative to erroring out when we'd otherwise evaluate an expression
that's controlled by a less privileged user. The admin can alter the table
definition / drop it, if requried.
> > Even a security definer function can mess up
> > your day when called in the wrong situation, e.g. due to getting
> > access to the
> > content of arguments (e.g. a trigger's row contents)
>
> I don't see that as a problem. If you're inserting data in a table,
> you'd expect the owner of the table to see that data and be able to
> modify it as they see fit.
>
> > or preventing an admin's
> > write from taking effect (by returning the relevant values from a
> > trigger).
>
> I don't see the problem here either. Even if we force the row to be
> inserted, the table owner could just delete it.
> > And not ever allowing execution of untrusted code in that situation IME
> > doesn't prevent desirable things.
>
> I don't understand this statement.
It's not a huge problem if a server admin gets an error while evaluating a
less-privileged-expression, because that's not commonly something that an
admin needs to do. And the admin likely can switch into the user context of
the less privileged user to perform operations in a safer context.
> >
> > I think a much more promising path towards that is to add a feature to
> > logical replication that changes the execution context to the table owner
> > while applying those changes.
>
> How is that different from SECURITY DEFINER?
It protect against vastly more things, see the default expression example.
> > And as I said, for 1 and 3 I think it's way preferrable to error out.
>
> My proposal does error out for SECURITY INVOKER, so I suppose you're
> saying we should error out for SECURITY DEFINER as well? In the case of
> 1, I think that would prevent regular maintenance by an admin.
What regular maintenance would be prevented? And would it be safe to execute
said code as superuser?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Blocking execution of SECURITY INVOKER
2023-01-13 03:29 Re: Blocking execution of SECURITY INVOKER Andres Freund <[email protected]>
@ 2023-01-13 03:38 ` Andres Freund <[email protected]>
2023-01-13 08:19 ` Re: Blocking execution of SECURITY INVOKER Jeff Davis <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Andres Freund @ 2023-01-13 03:38 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers
On 2023-01-12 19:29:43 -0800, Andres Freund wrote:
> Hi,
>
> On 2023-01-12 18:40:30 -0800, Jeff Davis wrote:
> > On Wed, 2023-01-11 at 19:33 -0800, Andres Freund wrote:
> >
> > > and the
> > > privilege check will be done with the rights of the admin in many of
> > > these
> > > contexts.
> >
> > Can you explain?
>
> If the less-privileged user does *not* have execution rights to a security
> definer function, but somehow can trick the more-privileged user into calling
> the function for them, e.g. by using it as the default expression of a column,
> the less-privileged user can escalate to the permissions of the security
> definer function.
>
> superuser:
> # CREATE FUNCTION exec_su(p_sql text) RETURNS text LANGUAGE plpgsql SECURITY DEFINER AS $$BEGIN RAISE NOTICE 'executing %', p_sql; EXECUTE p_sql;RETURN 'p_sql';END;$$;
> # REVOKE ALL ON FUNCTION exec_su FROM PUBLIC ;
>
> unprivileged user:
> $ SELECT exec_su('ALTER USER less_privs SUPERUSER');
> ERROR: 42501: permission denied for function exec_su
> $ CREATE TABLE trick_superuser(value text default exec_su('ALTER USER less_privs SUPERUSER'));
>
> superuser:
> # INSERT INTO trick_superuser DEFAULT VALUES;
> NOTICE: 00000: executing ALTER USER less_privs SUPERUSER
>
>
> This case would *not* be prevented by your proposed GUC, unless I miss
> something major. The superuser trusts itself and thus the exec_su() function.
Another reason security definer isn't a way to allow safe execution of lesser
privileged code:
superuser (andres):
# CREATE FUNCTION bleat_whoami() RETURNS text LANGUAGE plpgsql SECURITY INVOKER AS $$BEGIN RAISE NOTICE 'whoami: %', current_user;RETURN current_user;END;$$;
# REVOKE ALL ON FUNCTION bleat_whoami FROM PUBLIC;
unprivileged user:
$ CREATE FUNCTION secdef_with_default(foo text = bleat_whoami()) RETURNS text LANGUAGE plpgsql SECURITY DEFINER AS $$BEGIN RETURN 'secdef_with_default';END;$$;
$ SELECT secdef_with_default();
ERROR: 42501: permission denied for function bleat_whoami
superuser (andres):
# SELECT secdef_with_default();
NOTICE: 00000: whoami: andres
LOCATION: exec_stmt_raise, pl_exec.c:3893
┌─────────────────────┐
│ secdef_with_default │
├─────────────────────┤
│ secdef_with_default │
└─────────────────────┘
(1 row)
I.e. the default arguments where evaluated with the invoker's permissions, not
the definer's, despite being controlled by the less privileged user. Worsened
in this case by the fact that this allowed the less privileged user to call a
function they couldn't even call.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Blocking execution of SECURITY INVOKER
2023-01-13 03:29 Re: Blocking execution of SECURITY INVOKER Andres Freund <[email protected]>
2023-01-13 03:38 ` Re: Blocking execution of SECURITY INVOKER Andres Freund <[email protected]>
@ 2023-01-13 08:19 ` Jeff Davis <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Davis @ 2023-01-13 08:19 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers
On Thu, 2023-01-12 at 19:38 -0800, Andres Freund wrote:
> I.e. the default arguments where evaluated with the invoker's
> permissions, not
> the definer's, despite being controlled by the less privileged user.
This is a very interesting case. It also involves tricking the
superuser into executing their own function with the attacker's inputs.
That part is the same as the other case. What's intriguing here is that
it shows the function can be SECURITY INVOKER, and that really means it
could be any builtin function as long as the types work out.
For example:
=> create function trick(l pg_lsn = pg_switch_wal()) returns int
language plpgsql security definer as $$ begin return 42; end; $$;
If the superuser executes that, even though it's a SECURITY DEFINER
function owned by an unprivileged user, it will still call
pg_switch_wal().
--
Jeff Davis
PostgreSQL Contributor Team - AWS
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-01-13 08:19 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-02-27 04:03 [PATCH 01/10] Allow alternate compression methods for wal_compression Andrey Borodin <[email protected]>
2023-01-13 03:29 Re: Blocking execution of SECURITY INVOKER Andres Freund <[email protected]>
2023-01-13 03:38 ` Re: Blocking execution of SECURITY INVOKER Andres Freund <[email protected]>
2023-01-13 08:19 ` Re: Blocking execution of SECURITY INVOKER Jeff Davis <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox