Simplify ACL handling for large objects and removal of superuser() checks

From e01d38eac7b5dd54b5e03d90fb78bc66f88f49d7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 19 Sep 2017 16:02:26 +0900
Subject: [PATCH 1/3] Remove ALLOW_DANGEROUS_LO_FUNCTIONS for LO-related
 superuser checks

This switch dated of 4cd4a54c, which is old and not being used anymore by
modern distrubutions bundling PostgreSQL.
---
 src/backend/libpq/be-fsstubs.c |  4 ----
 src/include/pg_config_manual.h | 10 ----------
 2 files changed, 14 deletions(-)

diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index bf45461b2f..4d75c60979 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -448,13 +448,11 @@ lo_import_internal(text *filename, Oid lobjOid)
 	LargeObjectDesc *lobj;
 	Oid			oid;
 
-#ifndef ALLOW_DANGEROUS_LO_FUNCTIONS
 	if (!superuser())
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("must be superuser to use server-side lo_import()"),
 				 errhint("Anyone can use the client-side lo_import() provided by libpq.")));
-#endif
 
 	CreateFSContext();
 
@@ -514,13 +512,11 @@ be_lo_export(PG_FUNCTION_ARGS)
 	LargeObjectDesc *lobj;
 	mode_t		oumask;
 
-#ifndef ALLOW_DANGEROUS_LO_FUNCTIONS
 	if (!superuser())
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("must be superuser to use server-side lo_export()"),
 				 errhint("Anyone can use the client-side lo_export() provided by libpq.")));
-#endif
 
 	CreateFSContext();
 
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index f3b35297d1..529a554e57 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -66,16 +66,6 @@
  */
 #define NUM_ATOMICS_SEMAPHORES		64
 
-/*
- * Define this if you want to allow the lo_import and lo_export SQL
- * functions to be executed by ordinary users.  By default these
- * functions are only available to the Postgres superuser.  CAUTION:
- * These functions are SECURITY HOLES since they can read and write
- * any file that the PostgreSQL server has permission to access.  If
- * you turn this on, don't say we didn't warn you.
- */
-/* #define ALLOW_DANGEROUS_LO_FUNCTIONS */
-
 /*
  * MAXPGPATH: standard size of a pathname buffer in PostgreSQL (hence,
  * maximum usable pathname length is one less).
-- 
2.14.1

From 639ac5e87eafdc9a38955933cd81988da047734e Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 14 Aug 2017 11:15:31 +0900
Subject: [PATCH 2/3] Replace superuser checks of large object import/export by
 ACL checks

The large object functions lo_import and lo_export are restricted to
superusers since their creation, and since Postgres 10 it is possible
to dump permission checks related to system functions. Let's remove
those hardcoded checks and replace them wiht proper GRANT permissions
at initialization. This also gives more flexibility to administrators.
---
 src/backend/catalog/system_views.sql     |  4 ++++
 src/backend/libpq/be-fsstubs.c           | 12 ------------
 src/test/regress/expected/privileges.out | 10 ++++++----
 src/test/regress/sql/privileges.sql      |  2 ++
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index dc40cde424..cd7d0b208d 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1121,6 +1121,10 @@ AS 'jsonb_insert';
 -- system to REVOKE access to those functions at initdb time.  Administrators
 -- can later change who can access these functions, or leave them as only
 -- available to superuser / cluster owner, if they choose.
+REVOKE EXECUTE ON FUNCTION lo_import(text) FROM public;
+REVOKE EXECUTE ON FUNCTION lo_import(text, oid) FROM public;
+REVOKE EXECUTE ON FUNCTION lo_export(oid, text) FROM public;
+
 REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean, boolean) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stop_backup(boolean, boolean) FROM public;
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 4d75c60979..507843c386 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -448,12 +448,6 @@ lo_import_internal(text *filename, Oid lobjOid)
 	LargeObjectDesc *lobj;
 	Oid			oid;
 
-	if (!superuser())
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("must be superuser to use server-side lo_import()"),
-				 errhint("Anyone can use the client-side lo_import() provided by libpq.")));
-
 	CreateFSContext();
 
 	/*
@@ -512,12 +506,6 @@ be_lo_export(PG_FUNCTION_ARGS)
 	LargeObjectDesc *lobj;
 	mode_t		oumask;
 
-	if (!superuser())
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("must be superuser to use server-side lo_export()"),
-				 errhint("Anyone can use the client-side lo_export() provided by libpq.")));
-
 	CreateFSContext();
 
 	/*
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index f37df6c709..7f3b7ac25d 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1346,8 +1346,11 @@ ERROR:  permission denied for large object 1002
 SELECT lo_unlink(1002);					-- to be denied
 ERROR:  must be owner of large object 1002
 SELECT lo_export(1001, '/dev/null');			-- to be denied
-ERROR:  must be superuser to use server-side lo_export()
-HINT:  Anyone can use the client-side lo_export() provided by libpq.
+ERROR:  permission denied for function lo_export
+SELECT lo_import('/dev/null');				-- to be denied
+ERROR:  permission denied for function lo_import
+SELECT lo_import('/dev/null', 2003);			-- to be denied
+ERROR:  permission denied for function lo_import
 \c -
 SET lo_compat_privileges = true;	-- compatibility mode
 SET SESSION AUTHORIZATION regress_user4;
@@ -1376,8 +1379,7 @@ SELECT lo_unlink(1002);
 (1 row)
 
 SELECT lo_export(1001, '/dev/null');			-- to be denied
-ERROR:  must be superuser to use server-side lo_export()
-HINT:  Anyone can use the client-side lo_export() provided by libpq.
+ERROR:  permission denied for function lo_export
 -- don't allow unpriv users to access pg_largeobject contents
 \c -
 SELECT * FROM pg_largeobject LIMIT 0;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index e2c13e08a4..d22d08ca62 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -824,6 +824,8 @@ SELECT lo_truncate(lo_open(1002, x'20000'::int), 10);	-- to be denied
 SELECT lo_put(1002, 1, 'abcd');				-- to be denied
 SELECT lo_unlink(1002);					-- to be denied
 SELECT lo_export(1001, '/dev/null');			-- to be denied
+SELECT lo_import('/dev/null');				-- to be denied
+SELECT lo_import('/dev/null', 2003);			-- to be denied
 
 \c -
 SET lo_compat_privileges = true;	-- compatibility mode
-- 
2.14.1

From 2193250820378542d67042f4708e0068ca96517f Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
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                     |  25 +++----
 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, 121 insertions(+), 127 deletions(-)

diff --git a/doc/src/sgml/lobj.sgml b/doc/src/sgml/lobj.sgml
index 7757e1e441..2025545c75 100644
--- a/doc/src/sgml/lobj.sgml
+++ b/doc/src/sgml/lobj.sgml
@@ -276,20 +276,17 @@ int lo_open(PGconn *conn, Oid lobjId, int mode);
     </para>
 
     <para>
-     The server currently does not distinguish between modes
-     <symbol>INV_WRITE</symbol> and <symbol>INV_READ</> <literal>|</>
-     <symbol>INV_WRITE</symbol>: you are allowed to read from the descriptor
-     in either case.  However there is a significant difference between
-     these modes and <symbol>INV_READ</> alone: with <symbol>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 <function>lo_open</> was executed,
-     regardless of later writes by this or other transactions.  Reading
-     from a descriptor opened with <symbol>INV_WRITE</symbol> 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 <literal>REPEATABLE READ</> versus <literal>READ COMMITTED</> transaction
-     modes for ordinary SQL <command>SELECT</> commands.
+     With only <symbol>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
+     <function>lo_open</> was executed, regardless of later writes by this
+     or other transactions.  Reading from a descriptor opened with
+     <symbol>INV_WRITE</symbol> or <symbol>INV_READ</> <literal>|</>
+     <symbol>INV_WRITE</symbol> 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
+     <literal>REPEATABLE READ</> versus <literal>READ COMMITTED</>
+     transaction modes for ordinary SQL <command>SELECT</> commands.
     </para>
 
     <para>
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 6cac2dfd1d..ded4b38f31 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 507843c386..60763ff1a5 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,
@@ -565,27 +536,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);
 }
 
@@ -761,17 +718,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 bc9f09a086..2203f8385d 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

From 0432ed9c61a88aa77a221eec71f37c570af6353d Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 19 Sep 2017 16:02:26 +0900
Subject: [PATCH 1/3] Remove ALLOW_DANGEROUS_LO_FUNCTIONS for LO-related
 superuser checks

This switch dated of 4cd4a54c, which is old and not being used anymore by
modern distrubutions bundling PostgreSQL.
---
 src/backend/libpq/be-fsstubs.c |  4 ----
 src/include/pg_config_manual.h | 10 ----------
 2 files changed, 14 deletions(-)

diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 84c2d26402..9266d68569 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -448,13 +448,11 @@ lo_import_internal(text *filename, Oid lobjOid)
 	LargeObjectDesc *lobj;
 	Oid			oid;
 
-#ifndef ALLOW_DANGEROUS_LO_FUNCTIONS
 	if (!superuser())
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("must be superuser to use server-side lo_import()"),
 				 errhint("Anyone can use the client-side lo_import() provided by libpq.")));
-#endif
 
 	CreateFSContext();
 
@@ -514,13 +512,11 @@ be_lo_export(PG_FUNCTION_ARGS)
 	LargeObjectDesc *lobj;
 	mode_t		oumask;
 
-#ifndef ALLOW_DANGEROUS_LO_FUNCTIONS
 	if (!superuser())
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("must be superuser to use server-side lo_export()"),
 				 errhint("Anyone can use the client-side lo_export() provided by libpq.")));
-#endif
 
 	CreateFSContext();
 
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index b048175321..6f2238b330 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -72,16 +72,6 @@
  */
 #define NUM_ATOMICS_SEMAPHORES		64
 
-/*
- * Define this if you want to allow the lo_import and lo_export SQL
- * functions to be executed by ordinary users.  By default these
- * functions are only available to the Postgres superuser.  CAUTION:
- * These functions are SECURITY HOLES since they can read and write
- * any file that the PostgreSQL server has permission to access.  If
- * you turn this on, don't say we didn't warn you.
- */
-/* #define ALLOW_DANGEROUS_LO_FUNCTIONS */
-
 /*
  * MAXPGPATH: standard size of a pathname buffer in PostgreSQL (hence,
  * maximum usable pathname length is one less).
-- 
2.14.1

From decfd72535ec245366d33465b0be627299423be7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 14 Aug 2017 11:15:31 +0900
Subject: [PATCH 2/3] Replace superuser checks of large object import/export by
 ACL checks

The large object functions lo_import and lo_export are restricted to
superusers since their creation, and since Postgres 10 it is possible
to dump permission checks related to system functions. Let's remove
those hardcoded checks and replace them wiht proper GRANT permissions
at initialization. This also gives more flexibility to administrators.
---
 src/backend/catalog/system_views.sql     |  4 ++++
 src/backend/libpq/be-fsstubs.c           | 12 ------------
 src/test/regress/expected/privileges.out | 10 ++++++----
 src/test/regress/sql/privileges.sql      |  2 ++
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index dc40cde424..cd7d0b208d 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1121,6 +1121,10 @@ AS 'jsonb_insert';
 -- system to REVOKE access to those functions at initdb time.  Administrators
 -- can later change who can access these functions, or leave them as only
 -- available to superuser / cluster owner, if they choose.
+REVOKE EXECUTE ON FUNCTION lo_import(text) FROM public;
+REVOKE EXECUTE ON FUNCTION lo_import(text, oid) FROM public;
+REVOKE EXECUTE ON FUNCTION lo_export(oid, text) FROM public;
+
 REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean, boolean) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stop_backup(boolean, boolean) FROM public;
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 9266d68569..50c70dd66d 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -448,12 +448,6 @@ lo_import_internal(text *filename, Oid lobjOid)
 	LargeObjectDesc *lobj;
 	Oid			oid;
 
-	if (!superuser())
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("must be superuser to use server-side lo_import()"),
-				 errhint("Anyone can use the client-side lo_import() provided by libpq.")));
-
 	CreateFSContext();
 
 	/*
@@ -512,12 +506,6 @@ be_lo_export(PG_FUNCTION_ARGS)
 	LargeObjectDesc *lobj;
 	mode_t		oumask;
 
-	if (!superuser())
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("must be superuser to use server-side lo_export()"),
-				 errhint("Anyone can use the client-side lo_export() provided by libpq.")));
-
 	CreateFSContext();
 
 	/*
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index f37df6c709..7f3b7ac25d 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1346,8 +1346,11 @@ ERROR:  permission denied for large object 1002
 SELECT lo_unlink(1002);					-- to be denied
 ERROR:  must be owner of large object 1002
 SELECT lo_export(1001, '/dev/null');			-- to be denied
-ERROR:  must be superuser to use server-side lo_export()
-HINT:  Anyone can use the client-side lo_export() provided by libpq.
+ERROR:  permission denied for function lo_export
+SELECT lo_import('/dev/null');				-- to be denied
+ERROR:  permission denied for function lo_import
+SELECT lo_import('/dev/null', 2003);			-- to be denied
+ERROR:  permission denied for function lo_import
 \c -
 SET lo_compat_privileges = true;	-- compatibility mode
 SET SESSION AUTHORIZATION regress_user4;
@@ -1376,8 +1379,7 @@ SELECT lo_unlink(1002);
 (1 row)
 
 SELECT lo_export(1001, '/dev/null');			-- to be denied
-ERROR:  must be superuser to use server-side lo_export()
-HINT:  Anyone can use the client-side lo_export() provided by libpq.
+ERROR:  permission denied for function lo_export
 -- don't allow unpriv users to access pg_largeobject contents
 \c -
 SELECT * FROM pg_largeobject LIMIT 0;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index e2c13e08a4..d22d08ca62 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -824,6 +824,8 @@ SELECT lo_truncate(lo_open(1002, x'20000'::int), 10);	-- to be denied
 SELECT lo_put(1002, 1, 'abcd');				-- to be denied
 SELECT lo_unlink(1002);					-- to be denied
 SELECT lo_export(1001, '/dev/null');			-- to be denied
+SELECT lo_import('/dev/null');				-- to be denied
+SELECT lo_import('/dev/null', 2003);			-- to be denied
 
 \c -
 SET lo_compat_privileges = true;	-- compatibility mode
-- 
2.14.1

From ba8d845720d48585361dd4ceca50cf487ed55c72 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
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);
     </para>
 
     <para>
-     The server currently does not distinguish between modes
-     <symbol>INV_WRITE</symbol> and <symbol>INV_READ</> <literal>|</>
-     <symbol>INV_WRITE</symbol>: you are allowed to read from the descriptor
-     in either case.  However there is a significant difference between
-     these modes and <symbol>INV_READ</> alone: with <symbol>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 <function>lo_open</> was executed,
-     regardless of later writes by this or other transactions.  Reading
-     from a descriptor opened with <symbol>INV_WRITE</symbol> returns
+     With only <symbol>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
+     <function>lo_open</> was executed, regardless of later writes by this
+     or other transactions.  Reading from a descriptor opened with
+     <symbol>INV_READ</> <literal>|</> <symbol>INV_WRITE</symbol> 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 <literal>REPEATABLE READ</> versus <literal>READ COMMITTED</> transaction
-     modes for ordinary SQL <command>SELECT</> commands.
+     as writes of the current transaction.  This is similar to the behavior of
+     <literal>REPEATABLE READ</> versus <literal>READ COMMITTED</>
+     transaction modes for ordinary SQL <command>SELECT</> commands.
     </para>
 
     <para>
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

From 2c78b85b2a53fcfba78694cc80ec86a7e4f5a0c0 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 9 Nov 2017 10:30:39 +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.

Patch by Tom Lane and Michael Paquier.
---
 src/backend/catalog/objectaddress.c        |   2 +-
 src/backend/libpq/be-fsstubs.c             |  88 +++++------------------
 src/backend/storage/large_object/inv_api.c | 108 +++++++++++++++++++++++------
 src/backend/utils/misc/guc.c               |   2 +-
 src/include/libpq/be-fsstubs.h             |   5 --
 src/include/storage/large_object.h         |  13 ++--
 6 files changed, 112 insertions(+), 106 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..aa00253c85 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,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
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a609619f4d..0b0a72127a 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...
  */
-- 
2.15.0

From ecadd2d4d68f30a979ea592e866eb1b0d640bbca Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 19 Sep 2017 16:02:26 +0900
Subject: [PATCH 1/3] Remove ALLOW_DANGEROUS_LO_FUNCTIONS for LO-related
 superuser checks

This switch dated of 4cd4a54c, which is old and not being used anymore by
modern distrubutions bundling PostgreSQL.
---
 src/backend/libpq/be-fsstubs.c |  4 ----
 src/include/pg_config_manual.h | 10 ----------
 2 files changed, 14 deletions(-)

diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 84c2d26402..9266d68569 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -448,13 +448,11 @@ lo_import_internal(text *filename, Oid lobjOid)
 	LargeObjectDesc *lobj;
 	Oid			oid;
 
-#ifndef ALLOW_DANGEROUS_LO_FUNCTIONS
 	if (!superuser())
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("must be superuser to use server-side lo_import()"),
 				 errhint("Anyone can use the client-side lo_import() provided by libpq.")));
-#endif
 
 	CreateFSContext();
 
@@ -514,13 +512,11 @@ be_lo_export(PG_FUNCTION_ARGS)
 	LargeObjectDesc *lobj;
 	mode_t		oumask;
 
-#ifndef ALLOW_DANGEROUS_LO_FUNCTIONS
 	if (!superuser())
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("must be superuser to use server-side lo_export()"),
 				 errhint("Anyone can use the client-side lo_export() provided by libpq.")));
-#endif
 
 	CreateFSContext();
 
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index b048175321..6f2238b330 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -72,16 +72,6 @@
  */
 #define NUM_ATOMICS_SEMAPHORES		64
 
-/*
- * Define this if you want to allow the lo_import and lo_export SQL
- * functions to be executed by ordinary users.  By default these
- * functions are only available to the Postgres superuser.  CAUTION:
- * These functions are SECURITY HOLES since they can read and write
- * any file that the PostgreSQL server has permission to access.  If
- * you turn this on, don't say we didn't warn you.
- */
-/* #define ALLOW_DANGEROUS_LO_FUNCTIONS */
-
 /*
  * MAXPGPATH: standard size of a pathname buffer in PostgreSQL (hence,
  * maximum usable pathname length is one less).
-- 
2.15.0

From 42b92f4c3caac197b5bb84f8c8fc3d740ecf9ab0 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 14 Aug 2017 11:15:31 +0900
Subject: [PATCH 2/3] Replace superuser checks of large object import/export by
 ACL checks

The large object functions lo_import and lo_export are restricted to
superusers since their creation, and since Postgres 10 it is possible
to dump permission checks related to system functions. Let's remove
those hardcoded checks and replace them wiht proper GRANT permissions
at initialization. This also gives more flexibility to administrators.
---
 src/backend/catalog/system_views.sql     |  4 ++++
 src/backend/libpq/be-fsstubs.c           | 12 ------------
 src/test/regress/expected/privileges.out | 10 ++++++----
 src/test/regress/sql/privileges.sql      |  2 ++
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index dc40cde424..cd7d0b208d 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1121,6 +1121,10 @@ AS 'jsonb_insert';
 -- system to REVOKE access to those functions at initdb time.  Administrators
 -- can later change who can access these functions, or leave them as only
 -- available to superuser / cluster owner, if they choose.
+REVOKE EXECUTE ON FUNCTION lo_import(text) FROM public;
+REVOKE EXECUTE ON FUNCTION lo_import(text, oid) FROM public;
+REVOKE EXECUTE ON FUNCTION lo_export(oid, text) FROM public;
+
 REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean, boolean) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stop_backup(boolean, boolean) FROM public;
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 9266d68569..50c70dd66d 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -448,12 +448,6 @@ lo_import_internal(text *filename, Oid lobjOid)
 	LargeObjectDesc *lobj;
 	Oid			oid;
 
-	if (!superuser())
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("must be superuser to use server-side lo_import()"),
-				 errhint("Anyone can use the client-side lo_import() provided by libpq.")));
-
 	CreateFSContext();
 
 	/*
@@ -512,12 +506,6 @@ be_lo_export(PG_FUNCTION_ARGS)
 	LargeObjectDesc *lobj;
 	mode_t		oumask;
 
-	if (!superuser())
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("must be superuser to use server-side lo_export()"),
-				 errhint("Anyone can use the client-side lo_export() provided by libpq.")));
-
 	CreateFSContext();
 
 	/*
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 65d950f15b..771971a095 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1358,8 +1358,11 @@ ERROR:  permission denied for large object 1002
 SELECT lo_unlink(1002);					-- to be denied
 ERROR:  must be owner of large object 1002
 SELECT lo_export(1001, '/dev/null');			-- to be denied
-ERROR:  must be superuser to use server-side lo_export()
-HINT:  Anyone can use the client-side lo_export() provided by libpq.
+ERROR:  permission denied for function lo_export
+SELECT lo_import('/dev/null');				-- to be denied
+ERROR:  permission denied for function lo_import
+SELECT lo_import('/dev/null', 2003);			-- to be denied
+ERROR:  permission denied for function lo_import
 \c -
 SET lo_compat_privileges = true;	-- compatibility mode
 SET SESSION AUTHORIZATION regress_user4;
@@ -1388,8 +1391,7 @@ SELECT lo_unlink(1002);
 (1 row)
 
 SELECT lo_export(1001, '/dev/null');			-- to be denied
-ERROR:  must be superuser to use server-side lo_export()
-HINT:  Anyone can use the client-side lo_export() provided by libpq.
+ERROR:  permission denied for function lo_export
 -- don't allow unpriv users to access pg_largeobject contents
 \c -
 SELECT * FROM pg_largeobject LIMIT 0;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 902f64c747..a900ba2f84 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -839,6 +839,8 @@ SELECT lo_truncate(lo_open(1002, x'20000'::int), 10);	-- to be denied
 SELECT lo_put(1002, 1, 'abcd');				-- to be denied
 SELECT lo_unlink(1002);					-- to be denied
 SELECT lo_export(1001, '/dev/null');			-- to be denied
+SELECT lo_import('/dev/null');				-- to be denied
+SELECT lo_import('/dev/null', 2003);			-- to be denied
 
 \c -
 SET lo_compat_privileges = true;	-- compatibility mode
-- 
2.15.0