has_table_privilege for a table in unprivileged schema causes an error

Started by Yugo Nagataover 7 years ago11 messages

nagata@sraoss.co.jp

over 7 years ago

1 attachment(s)

Hi,

I found that has_table_privilege returns an error when a table is specified
by schema-qualified name and the user doen't have privilege for its schema.

postgres=> select has_table_privilege('myschema.tbl','select');
ERROR: permission denied for schema myschema

I think that this function should return false because the user doesn't have
the privilege on this table eventually. It is more useful for users because
it is not needed to parse the schema-qualified table name and check the
privilege on the schema in advance.

Attached is a patch to modify the function like that. This is WIP patch, so
only has_table_previlege is modified and other familiy functions are left as
they are. Also, there is no additional test yet.

One consern on this patch is that modifying the function can break the
back-compatibility, so it might be better to add a new parameter to
control the behavior of the function.

Any comments would be appriciated.

Regards,
--
Yugo Nagata <nagata@sraoss.co.jp>

Attachments:

has_table_privilege.patchtext/x-diff; name=has_table_privilege.patchDownload

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index a45e093de7..6628385277 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -95,7 +95,9 @@ static AclMode convert_any_priv_string(text *priv_type_text,
 						const priv_map *privileges);
 
 static Oid	convert_table_name(text *tablename);
+static Oid	convert_table_schema_name(text *tablename);
 static AclMode convert_table_priv_string(text *priv_type_text);
+static AclMode convert_table_schema_priv_string(text *priv_type_text);
 static AclMode convert_sequence_priv_string(text *priv_type_text);
 static AttrNumber convert_column_name(Oid tableoid, text *column);
 static AclMode convert_column_priv_string(text *priv_type_text);
@@ -1856,10 +1858,19 @@ has_table_privilege_name_name(PG_FUNCTION_ARGS)
 	text	   *priv_type_text = PG_GETARG_TEXT_PP(2);
 	Oid			roleid;
 	Oid			tableoid;
+	Oid			schemaoid;
 	AclMode		mode;
 	AclResult	aclresult;
 
 	roleid = get_role_oid_or_public(NameStr(*rolename));
+
+	schemaoid = convert_table_schema_name(tablename);
+	mode = convert_table_schema_priv_string(priv_type_text);
+
+	aclresult = pg_namespace_aclcheck(schemaoid, roleid, mode);
+	if (aclresult != ACLCHECK_OK)
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_table_priv_string(priv_type_text);
 
@@ -1881,10 +1892,19 @@ has_table_privilege_name(PG_FUNCTION_ARGS)
 	text	   *priv_type_text = PG_GETARG_TEXT_PP(1);
 	Oid			roleid;
 	Oid			tableoid;
+	Oid			schemaoid;
 	AclMode		mode;
 	AclResult	aclresult;
 
 	roleid = GetUserId();
+
+	schemaoid = convert_table_schema_name(tablename);
+	mode = convert_table_schema_priv_string(priv_type_text);
+
+	aclresult = pg_namespace_aclcheck(schemaoid, roleid, mode);
+	if (aclresult != ACLCHECK_OK)
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_table_priv_string(priv_type_text);
 
@@ -1957,9 +1977,17 @@ has_table_privilege_id_name(PG_FUNCTION_ARGS)
 	text	   *tablename = PG_GETARG_TEXT_PP(1);
 	text	   *priv_type_text = PG_GETARG_TEXT_PP(2);
 	Oid			tableoid;
+	Oid			schemaoid;
 	AclMode		mode;
 	AclResult	aclresult;
 
+	schemaoid = convert_table_schema_name(tablename);
+	mode = convert_table_schema_priv_string(priv_type_text);
+
+	aclresult = pg_namespace_aclcheck(schemaoid, roleid, mode);
+	if (aclresult != ACLCHECK_OK)
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_table_priv_string(priv_type_text);
 
@@ -1996,6 +2024,20 @@ has_table_privilege_id_id(PG_FUNCTION_ARGS)
  *		Support routines for has_table_privilege family.
  */
 
+/*
+ * Given a table name expressed as a string, return its schema Oid
+ */
+static Oid
+convert_table_schema_name(text *tablename)
+{
+	RangeVar   *relrv;
+
+	relrv = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
+
+	return get_namespace_oid(relrv->schemaname, false);
+}
+
+
 /*
  * Given a table name expressed as a string, look it up and return Oid
  */
@@ -2040,6 +2082,36 @@ convert_table_priv_string(text *priv_type_text)
 	return convert_any_priv_string(priv_type_text, table_priv_map);
 }
 
+/*
+ * convert_table_schema_priv_string
+ *		Convert text string to AclMode value.
+ */
+static AclMode
+convert_table_schema_priv_string(text *priv_type_text)
+{
+	static const priv_map table_priv_map[] = {
+		{"SELECT", ACL_USAGE},
+		{"SELECT WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"INSERT", ACL_USAGE},
+		{"INSERT WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"UPDATE", ACL_USAGE},
+		{"UPDATE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"DELETE", ACL_USAGE},
+		{"DELETE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"TRUNCATE", ACL_USAGE},
+		{"TRUNCATE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"REFERENCES", ACL_USAGE},
+		{"REFERENCES WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"TRIGGER", ACL_USAGE},
+		{"TRIGGER WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
+		{"RULE", 0},			/* ignore old RULE privileges */
+		{"RULE WITH GRANT OPTION", 0},
+		{NULL, 0}
+	};
+
+	return convert_any_priv_string(priv_type_text, table_priv_map);
+}
+
 /*
  * has_sequence_privilege variants
  *		These are all named "has_sequence_privilege" at the SQL level.
@@ -3832,7 +3904,6 @@ convert_schema_priv_string(text *priv_type_text)
 	return convert_any_priv_string(priv_type_text, schema_priv_map);
 }
 
-
 /*
  * has_server_privilege variants
  *		These are all named "has_server_privilege" at the SQL level.

Tom Lane

tgl@sss.pgh.pa.us

over 7 years ago

In reply to: Yugo Nagata (#1)

Re: has_table_privilege for a table in unprivileged schema causes an error

Yugo Nagata <nagata@sraoss.co.jp> writes:

I found that has_table_privilege returns an error when a table is specified
by schema-qualified name and the user doen't have privilege for its schema.

postgres=> select has_table_privilege('myschema.tbl','select');
ERROR: permission denied for schema myschema

I think that this function should return false because the user doesn't have
the privilege on this table eventually. It is more useful for users because
it is not needed to parse the schema-qualified table name and check the
privilege on the schema in advance.

Sounds reasonable, but if we're going to do that, we should do it for
every one of these functions that concerns a schema-qualifiable object
type. Not just tables.

Also, looking at the code, why are you bothering with
convert_table_schema_priv_string? ISTM what's relevant on the schema is
always going to be USAGE privilege, independently of the mode being
checked on the object. So you shouldn't need a bunch of duplicative
tables.

Plus, I don't think this implementation approach is going to work for
unqualified table names. You don't know which schema they're in until you
look them up. (Although I vaguely remember that the path search logic just
ignores unreadable schemas, so maybe all you have to do with unqualified
names is nothing. But that's not what this patch is doing now.)

Some test cases would likely be a good idea.

regards, tom lane

Yugo Nagata

nagata@sraoss.co.jp

over 7 years ago

In reply to: Tom Lane (#2)

Re: has_table_privilege for a table in unprivileged schema causes an error

On Thu, 16 Aug 2018 19:37:42 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yugo Nagata <nagata@sraoss.co.jp> writes:

I found that has_table_privilege returns an error when a table is specified
by schema-qualified name and the user doen't have privilege for its schema.

postgres=> select has_table_privilege('myschema.tbl','select');
ERROR: permission denied for schema myschema

I think that this function should return false because the user doesn't have
the privilege on this table eventually. It is more useful for users because
it is not needed to parse the schema-qualified table name and check the
privilege on the schema in advance.

Sounds reasonable, but if we're going to do that, we should do it for
every one of these functions that concerns a schema-qualifiable object
type. Not just tables.

OK. I will fix all of these functions that can take a schema-qualifiable
object name as a parameter.

Also, looking at the code, why are you bothering with
convert_table_schema_priv_string? ISTM what's relevant on the schema is
always going to be USAGE privilege, independently of the mode being
checked on the object. So you shouldn't need a bunch of duplicative
tables.

I thought we needed to consider also USAGE GRANT OPTION, but I might be
misunderstnding something. I will look into this again.

Plus, I don't think this implementation approach is going to work for
unqualified table names. You don't know which schema they're in until you
look them up. (Although I vaguely remember that the path search logic just
ignores unreadable schemas, so maybe all you have to do with unqualified
names is nothing. But that's not what this patch is doing now.)

Oops. I overlooked these cases. I'll fix the patch to allow to handle
unqualified table names.

Thanks,

--
Yugo Nagata <nagata@sraoss.co.jp>

Yugo Nagata

nagata@sraoss.co.jp

over 7 years ago

In reply to: Yugo Nagata (#3)

1 attachment(s)

Re: has_table_privilege for a table in unprivileged schema causes an error

Hi,

I updated the patch to support other has_*_privilege functions with some
refactoring, but tests for theses fixes are not included yet.

But, while running the regression test after this patch is applied, I found
that my patch doesn't pass privilege test. One of different behaviours
is as bellow.

-- Grant on all objects of given type in a schema
\c -

CREATE SCHEMA testns;
CREATE TABLE testns.t1 (f1 int);
CREATE TABLE testns.t2 (f1 int);

SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT'); -- false

GRANT ALL ON ALL TABLES IN SCHEMA testns TO regress_priv_user1;

SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT'); -- true

This is a part of src/test/regress/sql/privileges.sql. SELECT privilege on testns.t1
is granted to regress_priv_user1, so the has_table_privilege of the original implementation
returns true. On the other hand, after applied my patch, the function returns false
because the regress_priv_user1 doesn't have USAGE privilege on schema testns. Accually,
the user can not access to the table using SELECT statement.

Although the behavior of the original function would reflect pg_class.relacl, it seems to
me that the function fixed in my patch is more useful for users because this reflects
the actual accessibility during query execution.

Is there chance to change the behaviour of the functions as I am proposing?

If is is not allowed to change it to keep back-compatibility, I would like to propose
to add a parameter to the function to consider the privilege of the schema, for
example as bellow. Assuming false as the default values will keep the back-compatibility.

has_table_privilege(user, table, privilege[, consider_schema=false])

On Fri, 17 Aug 2018 12:02:57 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Thu, 16 Aug 2018 19:37:42 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yugo Nagata <nagata@sraoss.co.jp> writes:

I found that has_table_privilege returns an error when a table is specified
by schema-qualified name and the user doen't have privilege for its schema.

postgres=> select has_table_privilege('myschema.tbl','select');
ERROR: permission denied for schema myschema

I think that this function should return false because the user doesn't have
the privilege on this table eventually. It is more useful for users because
it is not needed to parse the schema-qualified table name and check the
privilege on the schema in advance.

Sounds reasonable, but if we're going to do that, we should do it for
every one of these functions that concerns a schema-qualifiable object
type. Not just tables.

OK. I will fix all of these functions that can take a schema-qualifiable
object name as a parameter.

In addition to has_table_privilege, has_any_column_privilege, has_column_privilege,
has_function_privilege, and has_sequence_privilege are fixed.

Also, looking at the code, why are you bothering with
convert_table_schema_priv_string? ISTM what's relevant on the schema is
always going to be USAGE privilege, independently of the mode being
checked on the object. So you shouldn't need a bunch of duplicative
tables.

I thought we needed to consider also USAGE GRANT OPTION, but I might be
misunderstnding something. I will look into this again.

Fixed to check only USAGE privilege on the schema.

Plus, I don't think this implementation approach is going to work for
unqualified table names. You don't know which schema they're in until you
look them up. (Although I vaguely remember that the path search logic just
ignores unreadable schemas, so maybe all you have to do with unqualified
names is nothing. But that's not what this patch is doing now.)

Oops. I overlooked these cases. I'll fix the patch to allow to handle
unqualified table names.

Fixed.

Regards,
--
Yugo Nagata <nagata@sraoss.co.jp>

Attachments:

has_table_privilege_v2.patchtext/x-diff; name=has_table_privilege_v2.patchDownload

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index a45e093de7..262472e424 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -94,6 +94,7 @@ static AclMode convert_priv_string(text *priv_type_text);
 static AclMode convert_any_priv_string(text *priv_type_text,
 						const priv_map *privileges);
 
+static bool check_schema_usage_privilege(text *objectname, Oid roleid);
 static Oid	convert_table_name(text *tablename);
 static AclMode convert_table_priv_string(text *priv_type_text);
 static AclMode convert_sequence_priv_string(text *priv_type_text);
@@ -1860,6 +1861,10 @@ has_table_privilege_name_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = get_role_oid_or_public(NameStr(*rolename));
+
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_table_priv_string(priv_type_text);
 
@@ -1885,6 +1890,10 @@ has_table_privilege_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = GetUserId();
+
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_table_priv_string(priv_type_text);
 
@@ -1960,6 +1969,9 @@ has_table_privilege_id_name(PG_FUNCTION_ARGS)
 	AclMode		mode;
 	AclResult	aclresult;
 
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_table_priv_string(priv_type_text);
 
@@ -1996,6 +2008,30 @@ has_table_privilege_id_id(PG_FUNCTION_ARGS)
  *		Support routines for has_table_privilege family.
  */
 
+/*
+ * Given a possibly-qualified object name as a string, checking a user's
+ * usage privilege to the object's namespace.  Return true if an unqualified
+ * name is specified because we don't have to consern its namespace.
+ */
+static bool
+check_schema_usage_privilege(text *objectname, Oid roleid)
+{
+	char	   *nspname = NULL;
+	char	   *objname = NULL;
+	Oid			schemaoid;
+
+	DeconstructQualifiedName(textToQualifiedNameList(objectname), &nspname, &objname);
+
+	if (!nspname)
+		return true;
+
+	schemaoid = get_namespace_oid(nspname, false);
+	if (pg_namespace_aclcheck(schemaoid, roleid, ACL_USAGE) == ACLCHECK_OK)
+		return true;
+	else
+		return false;
+}
+
 /*
  * Given a table name expressed as a string, look it up and return Oid
  */
@@ -2068,6 +2104,10 @@ has_sequence_privilege_name_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = get_role_oid_or_public(NameStr(*rolename));
+
+	if (!check_schema_usage_privilege(sequencename, roleid))
+		PG_RETURN_BOOL(false);
+
 	mode = convert_sequence_priv_string(priv_type_text);
 	sequenceoid = convert_table_name(sequencename);
 	if (get_rel_relkind(sequenceoid) != RELKIND_SEQUENCE)
@@ -2098,6 +2138,10 @@ has_sequence_privilege_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = GetUserId();
+
+	if (!check_schema_usage_privilege(sequencename, roleid))
+		PG_RETURN_BOOL(false);
+
 	mode = convert_sequence_priv_string(priv_type_text);
 	sequenceoid = convert_table_name(sequencename);
 	if (get_rel_relkind(sequenceoid) != RELKIND_SEQUENCE)
@@ -2190,6 +2234,9 @@ has_sequence_privilege_id_name(PG_FUNCTION_ARGS)
 	AclMode		mode;
 	AclResult	aclresult;
 
+	if (!check_schema_usage_privilege(sequencename, roleid))
+		PG_RETURN_BOOL(false);
+
 	mode = convert_sequence_priv_string(priv_type_text);
 	sequenceoid = convert_table_name(sequencename);
 	if (get_rel_relkind(sequenceoid) != RELKIND_SEQUENCE)
@@ -2282,6 +2329,10 @@ has_any_column_privilege_name_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = get_role_oid_or_public(NameStr(*rolename));
+
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_column_priv_string(priv_type_text);
 
@@ -2311,6 +2362,10 @@ has_any_column_privilege_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = GetUserId();
+
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_column_priv_string(priv_type_text);
 
@@ -2398,6 +2453,9 @@ has_any_column_privilege_id_name(PG_FUNCTION_ARGS)
 	AclMode		mode;
 	AclResult	aclresult;
 
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_column_priv_string(priv_type_text);
 
@@ -2524,6 +2582,10 @@ has_column_privilege_name_name_name(PG_FUNCTION_ARGS)
 	int			privresult;
 
 	roleid = get_role_oid_or_public(NameStr(*rolename));
+
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	colattnum = convert_column_name(tableoid, column);
 	mode = convert_column_priv_string(priv_type_text);
@@ -2552,6 +2614,10 @@ has_column_privilege_name_name_attnum(PG_FUNCTION_ARGS)
 	int			privresult;
 
 	roleid = get_role_oid_or_public(NameStr(*rolename));
+
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_column_priv_string(priv_type_text);
 
@@ -2630,6 +2696,9 @@ has_column_privilege_id_name_name(PG_FUNCTION_ARGS)
 	AclMode		mode;
 	int			privresult;
 
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	colattnum = convert_column_name(tableoid, column);
 	mode = convert_column_priv_string(priv_type_text);
@@ -2656,6 +2725,9 @@ has_column_privilege_id_name_attnum(PG_FUNCTION_ARGS)
 	AclMode		mode;
 	int			privresult;
 
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	mode = convert_column_priv_string(priv_type_text);
 
@@ -2732,6 +2804,10 @@ has_column_privilege_name_name(PG_FUNCTION_ARGS)
 	int			privresult;
 
 	roleid = GetUserId();
+
+	if (!check_schema_usage_privilege(tablename, roleid))
+		PG_RETURN_BOOL(false);
+
 	tableoid = convert_table_name(tablename);
 	colattnum = convert_column_name(tableoid, column);
 	mode = convert_column_priv_string(priv_type_text);
@@ -3276,6 +3352,10 @@ has_function_privilege_name_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = get_role_oid_or_public(NameStr(*username));
+
+	if (!check_schema_usage_privilege(functionname, roleid))
+		PG_RETURN_BOOL(false);
+
 	functionoid = convert_function_name(functionname);
 	mode = convert_function_priv_string(priv_type_text);
 
@@ -3301,6 +3381,10 @@ has_function_privilege_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = GetUserId();
+
+	if (!check_schema_usage_privilege(functionname, roleid))
+		PG_RETURN_BOOL(false);
+
 	functionoid = convert_function_name(functionname);
 	mode = convert_function_priv_string(priv_type_text);
 
@@ -3376,6 +3460,9 @@ has_function_privilege_id_name(PG_FUNCTION_ARGS)
 	AclMode		mode;
 	AclResult	aclresult;
 
+	if (!check_schema_usage_privilege(functionname, roleid))
+		PG_RETURN_BOOL(false);
+
 	functionoid = convert_function_name(functionname);
 	mode = convert_function_priv_string(priv_type_text);
 
@@ -4223,6 +4310,10 @@ has_type_privilege_name_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = get_role_oid_or_public(NameStr(*username));
+
+	if (!check_schema_usage_privilege(typename, roleid))
+		PG_RETURN_BOOL(false);
+
 	typeoid = convert_type_name(typename);
 	mode = convert_type_priv_string(priv_type_text);
 
@@ -4248,6 +4339,10 @@ has_type_privilege_name(PG_FUNCTION_ARGS)
 	AclResult	aclresult;
 
 	roleid = GetUserId();
+
+	if (!check_schema_usage_privilege(typename, roleid))
+		PG_RETURN_BOOL(false);
+
 	typeoid = convert_type_name(typename);
 	mode = convert_type_priv_string(priv_type_text);
 
@@ -4323,6 +4418,9 @@ has_type_privilege_id_name(PG_FUNCTION_ARGS)
 	AclMode		mode;
 	AclResult	aclresult;
 
+	if (!check_schema_usage_privilege(typename, roleid))
+		PG_RETURN_BOOL(false);
+
 	typeoid = convert_type_name(typename);
 	mode = convert_type_priv_string(priv_type_text);

Robert Haas

robertmhaas@gmail.com

over 7 years ago

In reply to: Yugo Nagata (#4)

Re: has_table_privilege for a table in unprivileged schema causes an error

On Fri, Aug 24, 2018 at 5:31 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote:

Although the behavior of the original function would reflect pg_class.relacl, it seems to
me that the function fixed in my patch is more useful for users because this reflects
the actual accessibility during query execution.

I'm not sure that it's a good idea to change this behavior.

In the case of an unqualified name, the permissions on the schemas in
the search path can affect which table is chosen in the first place.
For instance, suppose bob has search_path = a, b and has usage
permission on b but not on a. In that case, if both a.x and b.x
exist, bob's reference to x will latch onto b.x, ignoring a.x
completely. So, if I'm not mistaken, this change will never make any
difference for an unqualified name, because if you don't have usage
permission on the schema, you'll never decide that an unqualified name
references something in that schema in the first place. So I think
this only matters for qualified names.

And if you've got a qualified name, you know what schema it's in. If
you are concerned about a.b, nothing keeps you from writing a test
against schema a's permissions as well as a test against table a.b's
permissions. But on the other hand, if for some reason you want to
know about pg_class.relacl specifically, then having the function
consider both that and the schema's ACL could be awkward.

Also, the current system generally tries not to reveal any information
about the contents of schemas for which you have no permissions. For
instance, suppose there is a table a.x:

rhaas=> select has_table_privilege('a.x', 'select');
ERROR: permission denied for schema a
rhaas=> select has_table_privilege('a.nonexistent', 'select');
ERROR: permission denied for schema a

With this change, you could potentially learn something about which
tables exist in a schema to which you have no access. You could argue
that this is harmless; after all, right now, an unprivileged user can
run 'select * from pg_class', so the jig is up anyway. But that might
be something upon which we'd like to improve someday.

I admit that these are only mild disadvantages, but I think we should
reject breaking backward compatibility unless the result is a clear
improvement, and if anything this seems slightly worse to me. YMMV,
of course....

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tom Lane

tgl@sss.pgh.pa.us

over 7 years ago

In reply to: Robert Haas (#5)

Re: has_table_privilege for a table in unprivileged schema causes an error

Robert Haas <robertmhaas@gmail.com> writes:

I'm not sure that it's a good idea to change this behavior.

In the case of an unqualified name, the permissions on the schemas in
the search path can affect which table is chosen in the first place.
... So I think this only matters for qualified names.

Yeah, that agrees with my expectations.

Also, the current system generally tries not to reveal any information
about the contents of schemas for which you have no permissions.

I don't think that argument holds up, at least not if this is implemented
the way I'd expect. It would change the results for a schema on which
you lack usage permission from "permission denied for schema a" to
"false", but it still would not matter whether there is such a table
inside "a".

And if you've got a qualified name, you know what schema it's in. If
you are concerned about a.b, nothing keeps you from writing a test
against schema a's permissions as well as a test against table a.b's
permissions. But on the other hand, if for some reason you want to
know about pg_class.relacl specifically, then having the function
consider both that and the schema's ACL could be awkward.

Mmm ... maybe, but I don't think that exactly holds water either, given
that the current behavior is to fail if you lack permission on schema a.
Yes, you can write "case when has_schema_privilege() then
has_table_privilege() else false end", but if you're worried that you
might possibly lack schema privilege, then the current behavior of
has_table_privilege is useless to you: it doesn't matter whether or not
you would like to know about pg_class.relacl specifically, because you
won't be allowed to find out.

Also, in some use-cases the extra test would require writing code that can
split a qualified name into pieces, which isn't really all that easy in
SQL.

There's a backwards-compatibility argument for not changing this behavior,
sure, but I don't buy the other arguments you made here. And I don't
think there's all that much to the backwards-compatibility argument,
considering that the current behavior is to fail.

regards, tom lane

Yugo Nagata

nagata@sraoss.co.jp

over 7 years ago

In reply to: Tom Lane (#6)

Re: has_table_privilege for a table in unprivileged schema causes an error

On Sat, 25 Aug 2018 23:29:27 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I'm not sure that it's a good idea to change this behavior.

In the case of an unqualified name, the permissions on the schemas in
the search path can affect which table is chosen in the first place.
... So I think this only matters for qualified names.

Yeah, that agrees with my expectations.

Yes. I consider only the cases of qualified names and the patch doesn't
change any behavior about unqualified name cases.

Also, the current system generally tries not to reveal any information
about the contents of schemas for which you have no permissions.

I don't think that argument holds up, at least not if this is implemented
the way I'd expect. It would change the results for a schema on which
you lack usage permission from "permission denied for schema a" to
"false", but it still would not matter whether there is such a table
inside "a".

Yes, Tom's explanation is right. The proposal functions doesn't reveal
any information about the contents of unprivileged schemas, either.

And if you've got a qualified name, you know what schema it's in. If
you are concerned about a.b, nothing keeps you from writing a test
against schema a's permissions as well as a test against table a.b's
permissions. But on the other hand, if for some reason you want to
know about pg_class.relacl specifically, then having the function
consider both that and the schema's ACL could be awkward.

Mmm ... maybe, but I don't think that exactly holds water either, given
that the current behavior is to fail if you lack permission on schema a.
Yes, you can write "case when has_schema_privilege() then
has_table_privilege() else false end", but if you're worried that you
might possibly lack schema privilege, then the current behavior of
has_table_privilege is useless to you: it doesn't matter whether or not
you would like to know about pg_class.relacl specifically, because you
won't be allowed to find out.

Also, in some use-cases the extra test would require writing code that can
split a qualified name into pieces, which isn't really all that easy in
SQL.

This is a reason why we proposed to fix the function. However, with regard to
splitting a qualified name, making a new SQL function to do this might resolve
it, for example, as below.

select case when has_schema_privilege(x.nspname)
then has_table_privilege(x.objname)
else false end
from pg_split_qualified_name(tablename) x;

There's a backwards-compatibility argument for not changing this behavior,
sure, but I don't buy the other arguments you made here. And I don't
think there's all that much to the backwards-compatibility argument,
considering that the current behavior is to fail.

With regarding to keeping the backwards-compatibility, to add a new paramater
to has_*_privilege functions is a solution as proposed previously.

has_table_privilege(user, table, privilege[, consider_schema=false])

How do you think this proposal?

Regards,

--
Yugo Nagata <nagata@sraoss.co.jp>

Tom Lane

tgl@sss.pgh.pa.us

over 7 years ago

In reply to: Yugo Nagata (#7)

Re: has_table_privilege for a table in unprivileged schema causes an error

Yugo Nagata <nagata@sraoss.co.jp> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

There's a backwards-compatibility argument for not changing this behavior,
sure, but I don't buy the other arguments you made here. And I don't
think there's all that much to the backwards-compatibility argument,
considering that the current behavior is to fail.

With regarding to keeping the backwards-compatibility, to add a new paramater
to has_*_privilege functions is a solution as proposed previously.
has_table_privilege(user, table, privilege[, consider_schema=false])
How do you think this proposal?

I think that'd be a disaster, because we already have variants of these
functions with several different parameter counts. Adding optional
arguments to them will cause ambiguous-function errors where there were
none before.

Also, it's just plain ugly. We should either decide we want this change,
or decide we don't. Trying to have it both ways is not going to be
better.

regards, tom lane

David G. Johnston

david.g.johnston@gmail.com

over 7 years ago

In reply to: Tom Lane (#6)

Re: has_table_privilege for a table in unprivileged schema causes an error

On Sat, Aug 25, 2018 at 8:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

There's a backwards-compatibility argument for not changing this behavior,
sure, but I don't buy the other arguments you made here. And I don't
think there's all that much to the backwards-compatibility argument,
considering that the current behavior is to fail.

+1; any code using these functions can reasonably be expected to handle
true and false responses. Converting a present error into a false under
that presumption results in similar, and arguably improved, semantics.

While null is something to be avoided generally this is one of those cases
where if we did want to have a "cannot answer the question because
pre-conditions are not met" response I'd strongly consider using null to
represent that response instead of an error - using coalesce is
considerably easier than performing error handling. That isn't an option
here and the while there is information loss involved in the proposed
change its seems worth it to me to make such a change to make using the
function for its primary behavior easier at the cost of a removing a
hard-to-use side effect. Adding a new default argument or function is not
desirable.

To be clear, I do not consider this is not a backpatchable change.

David J.

#10

Tom Lane

tgl@sss.pgh.pa.us

about 7 years ago

In reply to: David G. Johnston (#9)

Re: has_table_privilege for a table in unprivileged schema causes an error

"David G. Johnston" <david.g.johnston@gmail.com> writes:

On Sat, Aug 25, 2018 at 8:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

There's a backwards-compatibility argument for not changing this behavior,
sure, but I don't buy the other arguments you made here. And I don't
think there's all that much to the backwards-compatibility argument,
considering that the current behavior is to fail.

+1; any code using these functions can reasonably be expected to handle
true and false responses. Converting a present error into a false under
that presumption results in similar, and arguably improved, semantics.

I took a closer look at what's going on here, and realized that the
existing code is a bit confused, or confusing, about whose privileges
it's testing. Consider this exchange (with HEAD, not the patch):

regression=# CREATE SCHEMA testns;
CREATE SCHEMA
regression=# CREATE TABLE testns.t1 (f1 int);
CREATE TABLE
regression=# CREATE USER regress_priv_user1;
CREATE ROLE
regression=# SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT');
has_table_privilege
---------------------
f
(1 row)

regression=# \c - regress_priv_user1
You are now connected to database "regression" as user "regress_priv_user1".
regression=> SELECT has_table_privilege('regress_priv_user1', 'testns.t1', 'SELECT');
ERROR: permission denied for schema testns

That is, whether you get an error or not depends on your *own*
privileges for the schema, not those of the user you're supposedly
testing. This seems rather inconsistent. If we apply the proposed
patch, then (I'd expect, but haven't tested) you should always get
the same results from has_table_privilege('u', 's.t', 'p') as from
doing has_table_privilege('s.t', 'p') as user u.

However ... if we do that, then Robert's previous concern about
information leakage comes to life, because an unprivileged user
could run has_table_privilege('postgres', 'testns.t1', 'SELECT')
and thereby find out whether t1 exists regardless of whether he
has any privilege for testns.

Mind you, I think that argument is mostly bunk, because that
same user can trivially find out whether t1 exists, and what
its privilege grants are, just by looking into pg_catalog.
So there's no real security improvement from disallowing this.

Anyway, it seems to me that the principle of least astonishment
suggests that the results of has_table_privilege should depend only
on the privileges of the user allegedly being tested, not those of
the calling user. Or if we decide differently, somebody has to write
some doc text explaining that it's not so.

Getting all the way to that might be a bit difficult though.
For example, in

SELECT has_function_privilege('someuser', 'func(schema.type)', 'usage');

the lookup and permission check for "schema" are buried a long
way down from the has_function_privilege code. It'd take a
lot of refactoring to keep it from throwing an error. I guess
maybe you could argue that privileges on the type are a different
question from privileges on the function, but it still seems ugly.

A related thought is that it's not clear whether there should be
any behavioral difference between

SELECT has_function_privilege('someuser', 'func(schema.type)'::text, 'usage');

SELECT has_function_privilege('someuser', 'func(schema.type)'::regprocedure, 'usage');

In the latter case, it's entirely unsurprising that the schema lookup
is done as the current user. However, if we define the first case
as being equivalent to the second, then the error that Yugo-san is
complaining of is something we can't/shouldn't fix, because certainly
"SELECT 'testns.t1'::regclass" is going to throw an error if you lack
usage privilege for testns.

So at this point I'm not sure what to think, other than that things
are inconsistent (and underdocumented).

regards, tom lane

#11

Michael Paquier

michael@paquier.xyz

almost 7 years ago

In reply to: Tom Lane (#10)

Re: has_table_privilege for a table in unprivileged schema causes an error

On Sun, Nov 18, 2018 at 06:32:42PM -0500, Tom Lane wrote:

So at this point I'm not sure what to think, other than that things
are inconsistent (and underdocumented).

Nagata-san, do you have some plans to do something about the comments
raised. The thread has been inactive for a couple of months now, so I
am marking the patch as returned with feedback.
--
Michael