From ae20b23a9e7029f31ee902da08a464d968319f56 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 9 Nov 2017 12:56:07 -0500 Subject: [PATCH] Refactor permissions checks for large objects. Up to now, ACL checks for large objects happened at the level of the SQL-callable functions, which led to CVE-2017-7548 because of a missing check. Push them down to be enforced in inv_api.c as much as possible, in hopes of preventing future bugs. This does have the effect of moving read and write permission errors to happen at lo_open time not loread or lowrite time, but that seems acceptable. Michael Paquier and Tom Lane Discussion: https://postgr.es/m/CAB7nPqRHmNOYbETnc_2EjsuzSM00Z+BWKv9sy6tnvSd5gWT_JA@mail.gmail.com --- src/backend/catalog/objectaddress.c | 2 +- src/backend/libpq/be-fsstubs.c | 88 ++++------------- src/backend/storage/large_object/inv_api.c | 110 ++++++++++++++++----- src/backend/utils/misc/guc.c | 12 +-- src/include/libpq/be-fsstubs.h | 5 - src/include/storage/large_object.h | 13 ++- 6 files changed, 118 insertions(+), 112 deletions(-) diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c index c2ad7c675e..8d55c76fc4 100644 --- a/src/backend/catalog/objectaddress.c +++ b/src/backend/catalog/objectaddress.c @@ -69,13 +69,13 @@ #include "commands/trigger.h" #include "foreign/foreign.h" #include "funcapi.h" -#include "libpq/be-fsstubs.h" #include "miscadmin.h" #include "nodes/makefuncs.h" #include "parser/parse_func.h" #include "parser/parse_oper.h" #include "parser/parse_type.h" #include "rewrite/rewriteSupport.h" +#include "storage/large_object.h" #include "storage/lmgr.h" #include "storage/sinval.h" #include "utils/builtins.h" diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c index 50c70dd66d..5a2479e6d3 100644 --- a/src/backend/libpq/be-fsstubs.c +++ b/src/backend/libpq/be-fsstubs.c @@ -51,11 +51,6 @@ #include "utils/builtins.h" #include "utils/memutils.h" -/* - * compatibility flag for permission checks - */ -bool lo_compat_privileges; - /* define this to enable debug logging */ /* #define FSDB 1 */ /* chunk size for lo_import/lo_export transfers */ @@ -108,14 +103,6 @@ be_lo_open(PG_FUNCTION_ARGS) lobjDesc = inv_open(lobjId, mode, fscxt); - if (lobjDesc == NULL) - { /* lookup failed */ -#if FSDB - elog(DEBUG4, "could not open large object %u", lobjId); -#endif - PG_RETURN_INT32(-1); - } - fd = newLOfd(lobjDesc); PG_RETURN_INT32(fd); @@ -163,22 +150,16 @@ lo_read(int fd, char *buf, int len) errmsg("invalid large-object descriptor: %d", fd))); lobj = cookies[fd]; - /* We don't bother to check IFS_RDLOCK, since it's always set */ - - /* Permission checks --- first time through only */ - if ((lobj->flags & IFS_RD_PERM_OK) == 0) - { - if (!lo_compat_privileges && - pg_largeobject_aclcheck_snapshot(lobj->id, - GetUserId(), - ACL_SELECT, - lobj->snapshot) != ACLCHECK_OK) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("permission denied for large object %u", - lobj->id))); - lobj->flags |= IFS_RD_PERM_OK; - } + /* + * Check state. inv_read() would throw an error anyway, but we want the + * error to be about the FD's state not the underlying privilege; it might + * be that the privilege exists but user forgot to ask for read mode. + */ + if ((lobj->flags & IFS_RDLOCK) == 0) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("large object descriptor %d was not opened for reading", + fd))); status = inv_read(lobj, buf, len); @@ -197,27 +178,13 @@ lo_write(int fd, const char *buf, int len) errmsg("invalid large-object descriptor: %d", fd))); lobj = cookies[fd]; + /* see comment in lo_read() */ if ((lobj->flags & IFS_WRLOCK) == 0) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("large object descriptor %d was not opened for writing", fd))); - /* Permission checks --- first time through only */ - if ((lobj->flags & IFS_WR_PERM_OK) == 0) - { - if (!lo_compat_privileges && - pg_largeobject_aclcheck_snapshot(lobj->id, - GetUserId(), - ACL_UPDATE, - lobj->snapshot) != ACLCHECK_OK) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("permission denied for large object %u", - lobj->id))); - lobj->flags |= IFS_WR_PERM_OK; - } - status = inv_write(lobj, buf, len); return status; @@ -342,7 +309,11 @@ be_lo_unlink(PG_FUNCTION_ARGS) { Oid lobjId = PG_GETARG_OID(0); - /* Must be owner of the largeobject */ + /* + * Must be owner of the large object. It would be cleaner to check this + * in inv_drop(), but we want to throw the error before not after closing + * relevant FDs. + */ if (!lo_compat_privileges && !pg_largeobject_ownercheck(lobjId, GetUserId())) ereport(ERROR, @@ -574,27 +545,13 @@ lo_truncate_internal(int32 fd, int64 len) errmsg("invalid large-object descriptor: %d", fd))); lobj = cookies[fd]; + /* see comment in lo_read() */ if ((lobj->flags & IFS_WRLOCK) == 0) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("large object descriptor %d was not opened for writing", fd))); - /* Permission checks --- first time through only */ - if ((lobj->flags & IFS_WR_PERM_OK) == 0) - { - if (!lo_compat_privileges && - pg_largeobject_aclcheck_snapshot(lobj->id, - GetUserId(), - ACL_UPDATE, - lobj->snapshot) != ACLCHECK_OK) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("permission denied for large object %u", - lobj->id))); - lobj->flags |= IFS_WR_PERM_OK; - } - inv_truncate(lobj, len); } @@ -770,17 +727,6 @@ lo_get_fragment_internal(Oid loOid, int64 offset, int32 nbytes) loDesc = inv_open(loOid, INV_READ, fscxt); - /* Permission check */ - if (!lo_compat_privileges && - pg_largeobject_aclcheck_snapshot(loDesc->id, - GetUserId(), - ACL_SELECT, - loDesc->snapshot) != ACLCHECK_OK) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("permission denied for large object %u", - loDesc->id))); - /* * Compute number of bytes we'll actually read, accommodating nbytes == -1 * and reads beyond the end of the LO. diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c index aa43b46c30..12940e5075 100644 --- a/src/backend/storage/large_object/inv_api.c +++ b/src/backend/storage/large_object/inv_api.c @@ -51,6 +51,11 @@ #include "utils/tqual.h" +/* + * GUC: backwards-compatibility flag to suppress LO permission checks + */ +bool lo_compat_privileges; + /* * All accesses to pg_largeobject and its index make use of a single Relation * reference, so that we only need to open pg_relation once per transaction. @@ -250,47 +255,79 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt) Snapshot snapshot = NULL; int descflags = 0; + /* + * Historically, no difference is made between (INV_WRITE) and (INV_WRITE + * | INV_READ), the caller being allowed to read the large object + * descriptor in either case. + */ if (flags & INV_WRITE) - { - snapshot = NULL; /* instantaneous MVCC snapshot */ - descflags = IFS_WRLOCK | IFS_RDLOCK; - } - else if (flags & INV_READ) - { - snapshot = GetActiveSnapshot(); - descflags = IFS_RDLOCK; - } - else + descflags |= IFS_WRLOCK | IFS_RDLOCK; + if (flags & INV_READ) + descflags |= IFS_RDLOCK; + + if (descflags == 0) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("invalid flags for opening a large object: %d", flags))); + /* Get snapshot. If write is requested, use an instantaneous snapshot. */ + if (descflags & IFS_WRLOCK) + snapshot = NULL; + else + snapshot = GetActiveSnapshot(); + /* Can't use LargeObjectExists here because we need to specify snapshot */ if (!myLargeObjectExists(lobjId, snapshot)) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("large object %u does not exist", lobjId))); - /* - * We must register the snapshot in TopTransaction's resowner, because it - * must stay alive until the LO is closed rather than until the current - * portal shuts down. Do this after checking that the LO exists, to avoid - * leaking the snapshot if an error is thrown. - */ - if (snapshot) - snapshot = RegisterSnapshotOnOwner(snapshot, - TopTransactionResourceOwner); + /* Apply permission checks, again specifying snapshot */ + if ((descflags & IFS_RDLOCK) != 0) + { + if (!lo_compat_privileges && + pg_largeobject_aclcheck_snapshot(lobjId, + GetUserId(), + ACL_SELECT, + snapshot) != ACLCHECK_OK) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied for large object %u", + lobjId))); + } + if ((descflags & IFS_WRLOCK) != 0) + { + if (!lo_compat_privileges && + pg_largeobject_aclcheck_snapshot(lobjId, + GetUserId(), + ACL_UPDATE, + snapshot) != ACLCHECK_OK) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied for large object %u", + lobjId))); + } - /* All set, create a descriptor */ + /* OK to create a descriptor */ retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt, sizeof(LargeObjectDesc)); retval->id = lobjId; retval->subid = GetCurrentSubTransactionId(); retval->offset = 0; - retval->snapshot = snapshot; retval->flags = descflags; + /* + * We must register the snapshot in TopTransaction's resowner, because it + * must stay alive until the LO is closed rather than until the current + * portal shuts down. Do this last to avoid uselessly leaking the + * snapshot if an error is thrown above. + */ + if (snapshot) + snapshot = RegisterSnapshotOnOwner(snapshot, + TopTransactionResourceOwner); + retval->snapshot = snapshot; + return retval; } @@ -312,7 +349,7 @@ inv_close(LargeObjectDesc *obj_desc) /* * Destroys an existing large object (not to be confused with a descriptor!) * - * returns -1 if failed + * Note we expect caller to have done any required permissions check. */ int inv_drop(Oid lobjId) @@ -333,6 +370,7 @@ inv_drop(Oid lobjId) */ CommandCounterIncrement(); + /* For historical reasons, we always return 1 on success. */ return 1; } @@ -397,6 +435,11 @@ inv_seek(LargeObjectDesc *obj_desc, int64 offset, int whence) Assert(PointerIsValid(obj_desc)); + /* + * We allow seek/tell if you have either read or write permission, so no + * need for a permission check here. + */ + /* * Note: overflow in the additions is possible, but since we will reject * negative results, we don't need any extra test for that. @@ -439,6 +482,11 @@ inv_tell(LargeObjectDesc *obj_desc) { Assert(PointerIsValid(obj_desc)); + /* + * We allow seek/tell if you have either read or write permission, so no + * need for a permission check here. + */ + return obj_desc->offset; } @@ -458,6 +506,12 @@ inv_read(LargeObjectDesc *obj_desc, char *buf, int nbytes) Assert(PointerIsValid(obj_desc)); Assert(buf != NULL); + if ((obj_desc->flags & IFS_RDLOCK) == 0) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied for large object %u", + obj_desc->id))); + if (nbytes <= 0) return 0; @@ -563,7 +617,11 @@ inv_write(LargeObjectDesc *obj_desc, const char *buf, int nbytes) Assert(buf != NULL); /* enforce writability because snapshot is probably wrong otherwise */ - Assert(obj_desc->flags & IFS_WRLOCK); + if ((obj_desc->flags & IFS_WRLOCK) == 0) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied for large object %u", + obj_desc->id))); if (nbytes <= 0) return 0; @@ -749,7 +807,11 @@ inv_truncate(LargeObjectDesc *obj_desc, int64 len) Assert(PointerIsValid(obj_desc)); /* enforce writability because snapshot is probably wrong otherwise */ - Assert(obj_desc->flags & IFS_WRLOCK); + if ((obj_desc->flags & IFS_WRLOCK) == 0) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied for large object %u", + obj_desc->id))); /* * use errmsg_internal here because we don't want to expose INT64_FORMAT diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index da061023f5..c4c1afa084 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -43,7 +43,6 @@ #include "commands/trigger.h" #include "funcapi.h" #include "libpq/auth.h" -#include "libpq/be-fsstubs.h" #include "libpq/libpq.h" #include "libpq/pqformat.h" #include "miscadmin.h" @@ -71,6 +70,7 @@ #include "storage/dsm_impl.h" #include "storage/standby.h" #include "storage/fd.h" +#include "storage/large_object.h" #include "storage/pg_shmem.h" #include "storage/proc.h" #include "storage/predicate.h" @@ -4900,7 +4900,7 @@ ResetAllOptions(void) if (conf->assign_hook) conf->assign_hook(conf->reset_val, - conf->reset_extra); + conf->reset_extra); *conf->variable = conf->reset_val; set_extra_field(&conf->gen, &conf->gen.extra, conf->reset_extra); @@ -4912,7 +4912,7 @@ ResetAllOptions(void) if (conf->assign_hook) conf->assign_hook(conf->reset_val, - conf->reset_extra); + conf->reset_extra); *conf->variable = conf->reset_val; set_extra_field(&conf->gen, &conf->gen.extra, conf->reset_extra); @@ -4924,7 +4924,7 @@ ResetAllOptions(void) if (conf->assign_hook) conf->assign_hook(conf->reset_val, - conf->reset_extra); + conf->reset_extra); *conf->variable = conf->reset_val; set_extra_field(&conf->gen, &conf->gen.extra, conf->reset_extra); @@ -4936,7 +4936,7 @@ ResetAllOptions(void) if (conf->assign_hook) conf->assign_hook(conf->reset_val, - conf->reset_extra); + conf->reset_extra); set_string_field(conf, conf->variable, conf->reset_val); set_extra_field(&conf->gen, &conf->gen.extra, conf->reset_extra); @@ -4948,7 +4948,7 @@ ResetAllOptions(void) if (conf->assign_hook) conf->assign_hook(conf->reset_val, - conf->reset_extra); + conf->reset_extra); *conf->variable = conf->reset_val; set_extra_field(&conf->gen, &conf->gen.extra, conf->reset_extra); diff --git a/src/include/libpq/be-fsstubs.h b/src/include/libpq/be-fsstubs.h index 96bcaa0f08..e8107a2c9f 100644 --- a/src/include/libpq/be-fsstubs.h +++ b/src/include/libpq/be-fsstubs.h @@ -14,11 +14,6 @@ #ifndef BE_FSSTUBS_H #define BE_FSSTUBS_H -/* - * compatibility option for access control - */ -extern bool lo_compat_privileges; - /* * These are not fmgr-callable, but are available to C code. * Probably these should have had the underscore-free names, diff --git a/src/include/storage/large_object.h b/src/include/storage/large_object.h index 796a8fdeea..01d0985b44 100644 --- a/src/include/storage/large_object.h +++ b/src/include/storage/large_object.h @@ -27,9 +27,9 @@ * offset is the current seek offset within the LO * flags contains some flag bits * - * NOTE: in current usage, flag bit IFS_RDLOCK is *always* set, and we don't - * bother to test for it. Permission checks are made at first read or write - * attempt, not during inv_open(), so we have other bits to remember that. + * NOTE: as of v11, permission checks are made when the large object is + * opened; therefore IFS_RDLOCK/IFS_WRLOCK indicate that read or write mode + * has been requested *and* the corresponding permission has been checked. * * NOTE: before 7.1, we also had to store references to the separate table * and index of a specific large object. Now they all live in pg_largeobject @@ -47,8 +47,6 @@ typedef struct LargeObjectDesc /* bits in flags: */ #define IFS_RDLOCK (1 << 0) /* LO was opened for reading */ #define IFS_WRLOCK (1 << 1) /* LO was opened for writing */ -#define IFS_RD_PERM_OK (1 << 2) /* read permission has been verified */ -#define IFS_WR_PERM_OK (1 << 3) /* write permission has been verified */ } LargeObjectDesc; @@ -78,6 +76,11 @@ typedef struct LargeObjectDesc #define MAX_LARGE_OBJECT_SIZE ((int64) INT_MAX * LOBLKSIZE) +/* + * GUC: backwards-compatibility flag to suppress LO permission checks + */ +extern bool lo_compat_privileges; + /* * Function definitions... */