Re: Fw: Isn't pg_statistic a security hole - Solution Proposal

"Joe Conway" <joe(at)conway-family(dot)com> writes:
> Here's a new patch for has_table_privilege

Looks like you're getting there. Herewith some miscellaneous comments
on minor matters like coding style:

> I used int instead of oid for the usesysid type.

I am not sure if that's a good idea or not. Peter E. has proposed
changing usesysid to type OID or even eliminating it entirely
(identifying users by pg_shadow row OID alone). While this hasn't
happened yet, it'd be a good idea to minimize the dependencies of your
code on which type is used for user ID. In particular I'd suggest using
"name" and "id" in the names of your variant functions, not "text" and
"oid" and "int", so that they don't have to be renamed if the types
change.

> I'm also attaching a test script and expected output.

The script doesn't seem to demonstrate that any attention is paid to the
mode input --- AFAICT all the tested cases are either all privileges
available or no privileges available. It could probably be a lot
shorter without being materially less effective, too; quasi-exhaustive
tests are usually not worth the cycles to run.

> +Datum
> +text_oid_has_table_privilege(PG_FUNCTION_ARGS)

This is just my personal preference, but I'd put the type identifiers at
the end (has_table_privilege_name_id) rather than giving them pride of
place at the start of the name.

> +Datum
> +has_table_privilege(int usesysid, char *relname, char *priv_type)

Since has_table_privilege is just an internal function and is not
fmgr-callable, there's no percentage in declaring it to return Datum;
it should just return bool and not use the PG_RETURN_ macros. You have
in fact called it as though it returned bool, which would be a type
violation if C were not so lax about type conversions.

Actually, though, I'm wondering why has_table_privilege is a function
at all. Its tests for valid usesysid and relname are a waste of cycles;
pg_aclcheck will do those for itself. The only actually useful code in
it is the conversion from a priv_type string to an AclMode code, which
would seem to be better handled as a separate function that just does
that part. The has_table_privilege_foo_bar routines could call
pg_aclcheck for themselves without any material loss of concision.

> + result = pg_aclcheck(relname, usesysid, mode);
> +
> + if (result == 1) {

This is not only non-symbolic, but outright wrong. You should be
testing pg_aclcheck's result to see if it is ACLCHECK_OK or not.

> +/* Privilege names for oid_oid_has_table_privilege */
> +#define PRIV_INSERT "INSERT\0"
> +#define PRIV_SELECT "SELECT\0"
> +#define PRIV_UPDATE "UPDATE\0"
> +#define PRIV_DELETE "DELETE\0"
> +#define PRIV_RULE "RULE\0"
> +#define PRIV_REFERENCES "REFERENCES\0"
> +#define PRIV_TRIGGER "TRIGGER\0"

You need not write these strings with those redundant null terminators.
For that matter, since they're only known in one function, it's not
clear that they should be exported to all and sundry in a header file
in the first place. I'd be inclined to just code

AclMode convert_priv_string (char * str)
{
if (strcasecmp(str, "SELECT") == 0)
return ACL_SELECT;
if (strcasecmp(str, "INSERT") == 0)
return ACL_INSERT;
... etc ...
elog(ERROR, ...);
}

and keep all the knowledge right there in that function. (Possibly it
should take a text* not char*, so as to avoid duplicated conversion code
in the callers, but this is minor.)

Despite all these gripes, it looks pretty good. One more round of
revisions ...

regards, tom lane