On Tue, 2023-03-28 at 13:55 -0400, Robert Haas wrote:
> Oh, interesting. I hadn't realized we were doing that, and I do think
> that narrows the vulnerability quite a bit.
It's good to know I wasn't missing something obvious.
> bsent logical replication, you don't really need to worry
> about whether that function is written securely -- it will run with
> privileges of the person performing the DML, and not impact your
> account at all.
That's not strictly true. See the example at the bottom of this email.
The second trigger is SECURITY INVOKER, and it captures the leaked
secret number that was stored in the tuple by an earlier SECURITY
DEFINER trigger.
Perhaps I'm being pedantic, but my point is that SECURITY INVOKER is
not a magical shield that protects users from themselves without bound.
It protects users against some kinds of attacks, sure, but there's a
limit to what it has to offer.
> They have reason to be scared of you, but not the
> reverse. However, if the people doing DML on the table can arrange to
> perform that DML as you, then any security vulnerabilities in your
> function potentially allow them to compromise your account.
OK, but I'd like to be clear that you've moved from your prior
statement:
"in theory they might be able to protect themselves, but in practice
they are unlikely to be careful enough"
To something very different, where it seems that we can't think of a
concrete problem even for not-so-careful users.
The dangerous cases seem to be something along the lines of a security-
invoker trigger function that builds and executes arbirary SQL based on
the row contents. And then the table owner would then still need to set
ENABLE ALWAYS TRIGGER.
Do we really want to take that case on as our security responsibility?
> It would also affect someone who uses a default
> expression or other means of associating executable code with the
> table, and something like a default expression doesn't require any
> explicit setting to make it apply in case of replication, so maybe
> the
> risk of someone messing up is a bit higher.
Perhaps, but I still don't understand that case. Unless I'm missing
something, the table owner would have to write a pretty weird default
expression or check constraint or whatever to end up with something
dangerous.
> But this definitely makes it more of a judgment call than I thought
> initially.
And I'm fine if the judgement is that we just require SET ROLE to be
conservative and make sure we don't over-promise in 16. That's a
totally reasonable thing: it's easier to loosen restrictions later than
to tighten them. Furthermore, just because you and I can't think of
exploitable problems doesn't mean they don't exist.
I have found this discussion enlightening, so thank you for going
through these details with me.
> I'm still inclined to leave the patch checking for SET ROLE
+0.5. I want the patch to go in either way, and can carry on the
discussion later and improve things more for 17.
But I want to say generally that I believe we spend too much effort
trying to protect unreasonable users from themselves, which interferes
with our ability to protect reasonable users and solve real use cases.
> However, there might be an argument
> that we ought to do something else, like have a REPLICATE privilege
Sounds cool. Not sure I have an opinion yet, but probably 17 material.
> What I would like to change in the
> patch in this release is to add a new subscription property along the
> lines of what I proposed to Jelte in my earlier email: let's provide
> an escape hatch that turns off the user-switching behavior.
I believe switching to the table owner (as done in your patch) is the
right best practice, and should be the default.
I'm fine with an escape hatch here to ease migrations or whatever. But
please do it in an unobtrusive way such that we (as hackers) can mostly
forget that the non-switching behavior exists. At least for me, our
system is already pretty complex without two kinds of subscription
security models.
Regards,
Jeff Davis
----- example follows ------
-- user foo
CREATE TABLE secret(I INT);
INSERT INTO secret VALUES(42);
CREATE TABLE r(i INT, secret INT);
CREATE OR REPLACE FUNCTION a_func() RETURNS TRIGGER
SECURITY DEFINER LANGUAGE plpgsql AS
$$
BEGIN
SELECT i INTO NEW.secret FROM secret;
RETURN NEW;
END;
$$;
CREATE OR REPLACE FUNCTION z_func() RETURNS TRIGGER
SECURITY INVOKER LANGUAGE plpgsql AS
$$
BEGIN
IF NEW.secret <> abs(NEW.secret) THEN
RAISE EXCEPTION 'no negative secret numbers allowed';
END IF;
RETURN NEW;
END;
$$;
CREATE TRIGGER a_trigger BEFORE INSERT ON r
FOR EACH ROW EXECUTE PROCEDURE a_func();
CREATE TRIGGER z_trigger BEFORE INSERT ON r
FOR EACH ROW EXECUTE PROCEDURE z_func();
GRANT INSERT ON r TO bar;
GRANT USAGE ON SCHEMA foo TO bar;
-- user bar
CREATE OR REPLACE FUNCTION bar.abs(i INT4) RETURNS INT4
LANGUAGE plpgsql AS
$$
BEGIN
INSERT INTO secret_copy VALUES(i);
RETURN pg_catalog.abs(i);
END;
$$;
CREATE TABLE secret_copy(secret int);
SET search_path = "$user", pg_catalog;
INSERT INTO foo.r VALUES(1);
