FOR PORTION OF vs. object_aclcheck
Hi,
I noticed today that ExecForPortionOfLeftovers initializes an FmgrInfo
for forPortionOf->withoutPortionProc without checking whether the user
has ACL_EXECUTE on that function. Currently, this does not really
matter, because transformForPortionOfClause only ever sets
withoutPortionProc to F_RANGE_MINUS_MULTI or F_MULTIRANGE_MINUS_MULTI,
which are publicly executable anyway, at least by default. However,
there's a comment that says this:
* XXX: Find a more extensible way to look up the function, permitting
* user-defined types. An opclass support function doesn't make sense,
* since there is no index involved. Perhaps a type support function.
So maybe in the future this will be able to call a wider range of
functions. It's possible that won't really matter from a security
perspective either since maybe you'd have to be superuser to get a
function OID of your own choosing into withoutPortionProc. However, I
think it might be a good idea to add a permission check here anyway,
just to be on the safe side. I can't see any real downside to
insisting that the user actually executing the plan is currently in
possession of the required ACL_EXECUTE permission. In the unlikely
event that a superuser did revoke public execute permission on one of
the range-minus functions, they might expect that said function would
also no longer be callable by this mechanism. Even if they did not
expect that, it's not an unreasonable consequence. Conversely, if it
happens to become possible for a non-superuser to choose the called
function here at some point in the future, then failure to check
permissions at this point in the code would be a CVE.
Thoughts?
--
Robert Haas
EDB: http://www.enterprisedb.com
On 04.05.26 15:36, Robert Haas wrote:
Hi,
I noticed today that ExecForPortionOfLeftovers initializes an FmgrInfo
for forPortionOf->withoutPortionProc without checking whether the user
has ACL_EXECUTE on that function. Currently, this does not really
matter, because transformForPortionOfClause only ever sets
withoutPortionProc to F_RANGE_MINUS_MULTI or F_MULTIRANGE_MINUS_MULTI,
which are publicly executable anyway, at least by default. However,
there's a comment that says this:* XXX: Find a more extensible way to look up the function, permitting
* user-defined types. An opclass support function doesn't make sense,
* since there is no index involved. Perhaps a type support function.So maybe in the future this will be able to call a wider range of
functions. It's possible that won't really matter from a security
perspective either since maybe you'd have to be superuser to get a
function OID of your own choosing into withoutPortionProc. However, I
think it might be a good idea to add a permission check here anyway,
just to be on the safe side. I can't see any real downside to
insisting that the user actually executing the plan is currently in
possession of the required ACL_EXECUTE permission. In the unlikely
event that a superuser did revoke public execute permission on one of
the range-minus functions, they might expect that said function would
also no longer be callable by this mechanism. Even if they did not
expect that, it's not an unreasonable consequence. Conversely, if it
happens to become possible for a non-superuser to choose the called
function here at some point in the future, then failure to check
permissions at this point in the code would be a CVE.
If we assume the type support function approach, I think we don't
normally check permissions on type support functions. It is implied
that if you make use of a type, then the type support functions could be
executed at any time without further permission checking.
Maybe we should at least expand the comment to remind future developers
of this concern, though?
On Tue, May 12, 2026 at 11:37 AM Peter Eisentraut <peter@eisentraut.org> wrote:
If we assume the type support function approach, I think we don't
normally check permissions on type support functions. It is implied
that if you make use of a type, then the type support functions could be
executed at any time without further permission checking.
I'm not totally sure we've made the right policy choice there, and I'm
also not sure that we're entirely consistent, but there's certainly
precedent for that approach.
Maybe we should at least expand the comment to remind future developers
of this concern, though?
Yeah, if we're not going to add a check, we should definitely write
something in the comment.
--
Robert Haas
EDB: http://www.enterprisedb.com