From ba8d845720d48585361dd4ceca50cf487ed55c72 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 19 Sep 2017 16:11:50 +0900 Subject: [PATCH 3/3] Move ACL checks for large objects when opening them Up to now, ACL checks for large objects happened at the the level of the SQL-callable functions, which led to CVE-2017-7548 because of a missing check. It is actually possible to simplify those checks when opening a large object, which simplifies the code checking them so as there is no need to track if a check has already been done or not and this removes any risk of missing again permission checks if a function related to large objects is added in the future. A more debatable change that this patch introduces is that opening a large object for write is equivalent to read-write, which is a behavior documented, into a complete split of the checks. This gives the user more flexibility at the risk of breaking some applications, and makes the new behavior more natural. Patch by Tom Lane and Michael Paquier. --- doc/src/sgml/lobj.sgml | 22 +++--- src/backend/catalog/objectaddress.c | 2 +- src/backend/libpq/be-fsstubs.c | 88 +++++------------------- src/backend/storage/large_object/inv_api.c | 103 ++++++++++++++++++++++------- src/backend/utils/misc/guc.c | 2 +- src/include/libpq/be-fsstubs.h | 5 -- src/include/storage/large_object.h | 13 ++-- src/test/regress/expected/privileges.out | 8 +-- src/test/regress/sql/privileges.sql | 2 +- 9 files changed, 119 insertions(+), 126 deletions(-) diff --git a/doc/src/sgml/lobj.sgml b/doc/src/sgml/lobj.sgml index 7757e1e441..15536e5437 100644 --- a/doc/src/sgml/lobj.sgml +++ b/doc/src/sgml/lobj.sgml @@ -276,20 +276,16 @@ int lo_open(PGconn *conn, Oid lobjId, int mode); - The server currently does not distinguish between modes - INV_WRITE and INV_READ | - INV_WRITE: you are allowed to read from the descriptor - in either case. However there is a significant difference between - these modes and INV_READ alone: with INV_READ - you cannot write on the descriptor, and the data read from it will - reflect the contents of the large object at the time of the transaction - snapshot that was active when lo_open was executed, - regardless of later writes by this or other transactions. Reading - from a descriptor opened with INV_WRITE returns + With only INV_READ you cannot write on the descriptor, + and the data read from it will reflect the contents of the large object + at the time of the transaction snapshot that was active when + lo_open was executed, regardless of later writes by this + or other transactions. Reading from a descriptor opened with + INV_READ | INV_WRITE returns data that reflects all writes of other committed transactions as well - as writes of the current transaction. This is similar to the behavior - of REPEATABLE READ versus READ COMMITTED transaction - modes for ordinary SQL SELECT commands. + as writes of the current transaction. This is similar to the behavior of + REPEATABLE READ versus READ COMMITTED + transaction modes for ordinary SQL SELECT commands. 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 d55d40e6f8..495b44fa93 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. @@ -269,45 +274,72 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt) int descflags = 0; 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; + 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))); + /* 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))); + } + + /* OK to create a descriptor */ + retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt, + sizeof(LargeObjectDesc)); + retval->id = lobjId; + retval->subid = GetCurrentSubTransactionId(); + retval->offset = 0; + 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 after checking that the LO exists, to avoid - * leaking the snapshot if an error is thrown. + * portal shuts down. Do this last to avoid uselessly leaking the + * snapshot if an error is thrown above. */ if (snapshot) snapshot = RegisterSnapshotOnOwner(snapshot, TopTransactionResourceOwner); - - /* All set, create a descriptor */ - retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt, - sizeof(LargeObjectDesc)); - retval->id = lobjId; - retval->subid = GetCurrentSubTransactionId(); - retval->offset = 0; retval->snapshot = snapshot; - retval->flags = descflags; return retval; } @@ -330,7 +362,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) @@ -351,6 +383,7 @@ inv_drop(Oid lobjId) */ CommandCounterIncrement(); + /* For historical reasons, we always return 1 on success. */ return 1; } @@ -415,6 +448,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. @@ -457,6 +495,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; } @@ -476,6 +519,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; @@ -581,7 +630,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; @@ -767,7 +820,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 47a5f25707..f770e05406 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" 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... */ diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 7f3b7ac25d..f07b610369 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -1238,12 +1238,8 @@ SELECT lo_create(2002); 2002 (1 row) -SELECT loread(lo_open(1001, x'20000'::int), 32); -- allowed, for now - loread --------- - \x -(1 row) - +SELECT loread(lo_open(1001, x'20000'::int), 32); -- fail, wrong mode +ERROR: large object descriptor 0 was not opened for reading SELECT lowrite(lo_open(1001, x'40000'::int), 'abcd'); -- fail, wrong mode ERROR: large object descriptor 0 was not opened for writing SELECT loread(lo_open(1001, x'40000'::int), 32); diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index d22d08ca62..8978696fa6 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -779,7 +779,7 @@ SET SESSION AUTHORIZATION regress_user2; SELECT lo_create(2001); SELECT lo_create(2002); -SELECT loread(lo_open(1001, x'20000'::int), 32); -- allowed, for now +SELECT loread(lo_open(1001, x'20000'::int), 32); -- fail, wrong mode SELECT lowrite(lo_open(1001, x'40000'::int), 'abcd'); -- fail, wrong mode SELECT loread(lo_open(1001, x'40000'::int), 32); -- 2.14.1