Refactor permissions checks for large objects.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 9 Nov 2017 17:56:07 +0000 (12:56 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 9 Nov 2017 17:56:07 +0000 (12:56 -0500)
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
src/backend/libpq/be-fsstubs.c
src/backend/storage/large_object/inv_api.c
src/backend/utils/misc/guc.c
src/include/libpq/be-fsstubs.h
src/include/storage/large_object.h

index c2ad7c675e531e6d13fbaf9282cec7771d8bf70e..8d55c76fc4afe4373c4b39ace537895501243137 100644 (file)
 #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"
index 50c70dd66d6975feab4fb72defd3d7650761a8d6..5a2479e6d3e0803da364845b6d42140e68804fd9 100644 (file)
 #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.
index aa43b46c305f4a9cd54fdedba2b192b42f70a87f..12940e5075e0d64f25724a3152372c0832ba5eaf 100644 (file)
 #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,46 +255,78 @@ 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)));
 
+       /* 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;
 }
@@ -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
index da061023f52f5807924a75cf57df1f65d1ae9a14..c4c1afa084ba2b99c345434c37ad2f8041d665b2 100644 (file)
@@ -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);
index 96bcaa0f0867376eaa32243197662781cd213c57..e8107a2c9f04621e49893de332c64c89b35115c7 100644 (file)
 #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,
index 796a8fdeea732bf118ddd6ab51ef472a95ccdf5e..01d0985b440251c076cf524814fd6fbc6b84e0e8 100644 (file)
@@ -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...
  */