Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recursive stored procedure shouldnt require execute right to call itself [CORE3242] #3612

Closed
firebird-automations opened this issue Nov 17, 2010 · 19 comments

Comments

@firebird-automations
Copy link
Collaborator

Submitted by: Ain Valtin (ain)

Create these objects as some other user than "test":

CREATE TABLE TAB_Foo ( Fld INTEGER );

SET TERM ^ ;

CREATE PROCEDURE SP_R(ID INTEGER) AS
BEGIN
IF(ID IS NULL AND ID IS NOT NULL)THEN EXECUTE PROCEDURE SP_R(ID);
END^

CREATE TRIGGER Foo_BI FOR TAB_Foo
ACTIVE BEFORE INSERT POSITION 15
AS
BEGIN
EXECUTE PROCEDURE SP_R(NEW.Fld);
END^

SET TERM ; ^

GRANT ALL ON TAB_Foo TO test;
GRANT EXECUTE ON PROCEDURE SP_R TO TRIGGER Foo_BI;

connect as "test" and execute:

INSERT INTO TAB_Foo(Fld) VALUES(1);

Error: *** IBPP::SQLException ***
Context: Statement::Prepare( INSERT INTO TAB_Foo(Fld) VALUES(1) )
Message: isc_dsql_prepare failed

SQL Message : -551
This user does not have privilege to perform this operation on this object.

Engine Code : 335544352
Engine Message : no permission for execute access to PROCEDURE SP_R

Workaround is to grant execute right on SP_R to itself:
GRANT EXECUTE ON PROCEDURE SP_R TO PROCEDURE SP_R;

but this shouldn't really be necessary as whoever/whatever called SP_R must have right to do so, thus the SP should have right to recursively call itself without any excplicit grant.

Commits: bf82df1 b412d2d 68431df FirebirdSQL/fbt-repository@e0a2e47 FirebirdSQL/fbt-repository@7d1e70a FirebirdSQL/fbt-repository@610e42a

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

assignee: Alexander Peshkov [ alexpeshkoff ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

> Workaround is to grant execute right on SP_R to itself:
> GRANT EXECUTE ON PROCEDURE SP_R TO PROCEDURE SP_R;

> but this shouldn't really be necessary as whoever/whatever called SP_R must have right to do so, thus the SP should have right to recursively call itself without any excplicit grant.

Why do you think so ? Any proof (from standard, for example) ?

@firebird-automations
Copy link
Collaborator Author

Commented by: Ain Valtin (ain)

Vlad - I don't have any proof for that other than logic I already mentioned: if the "original caller" had right to invoke the prodcedure, then the procedure should have a right to recurse as the original caller has right to execute the procedure.
I can't see any advantage in the current implementation, only additional work for the DBA to maintain rights.

@firebird-automations
Copy link
Collaborator Author

Commented by: Sean Leyne (seanleyne)

Ain,

While I would agree that the grants for recursive procedures might seem obvious, "obvious" is not a concept that the SQL committee/standard has ever embraced.

Further, adding a single grant for the SP to call itself can hardly be described as a significant effort for the DBA.

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

Ain, if we follow your logic in this example:

CREATE PROCEDURE SP1 AS
BEGIN
IF(la-la-la) THEN EXECUTE PROCEDURE SP2;
END^

CREATE PROCEDURE SP2 AS
BEGIN
IF(bla-bla-bla) THEN EXECUTE PROCEDURE SP3;
END^

CREATE PROCEDURE SP3 AS
BEGIN
IF(bla-bla-bla) THEN EXECUTE PROCEDURE SP2;
END^

GRANT EXECUTE ON PROCEDURE SP2 TO PROCEDURE SP1;
GRANT EXECUTE ON PROCEDURE SP3 TO PROCEDURE SP2;
GRANT EXECUTE ON PROCEDURE SP1 TO USER TEST;

we must say that SP3 must be 'auto-granted' execute on SP2 cause otherwise initial call to SP2 will fail. I.e. each object in the trace of SP calls must add it's rights to the context of a call. I'm far not sure that this is correct from security POV.

@firebird-automations
Copy link
Collaborator Author

Commented by: Ain Valtin (ain)

Alexander, I see your point. However, in this case it seems obvious that SP3 must be granted right to execute SP2 (unless SP3 is only meant to be called indirectly, via SP2, in which case "cascading rights" would cover it).

I guess the question really boils down to what standard says about user rights in such a situation and/or how other major players have implemented it...

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

Standard says that something may be granted only to user or role. According to standard when procedure begins to execute security context of session is saved and security context of user, who owns procedure, is used _instead_. I.e. instead of "cascading rights" standard suggests "replacing rights".

This means that in my sample FB works according to standard, but in initial case self recursive call is always possible - any user always has full rights on the objects owned by him. But this is definitely security schema different from one used by FB. Now I'm unsure should we always allow self-call or not.

@firebird-automations
Copy link
Collaborator Author

Commented by: @dyemanov

I would vote for allowing recursive calls. We already have similar permission shortcuts, i.e. triggers have all access to their base relation (at the table level) and its fields (at the field level) without any explicit grants.

@firebird-automations
Copy link
Collaborator Author

Commented by: Ain Valtin (ain)

> Standard says that something may be granted only to user or role.

So granting rights to SP and trigger is FireBird's non-standard extension?

> According to standard when procedure begins to execute security context of session is saved and security context of user,
> who owns procedure, is used _instead_. I.e. instead of "cascading rights" standard suggests "replacing rights".

I personaly prefer FB's way - it gives more control over user rights.

> Now I'm unsure should we always allow self-call or not.

Could the current implementation viewed as useful feature? In my opinion no, so I would change it to always allow self-call.

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

Let's be decided with suggested solution - and therefore increase priority.

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

priority: Minor [ 4 ] => Major [ 3 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

status: Open [ 1 ] => Open [ 1 ]

Target: 3.0 Beta 1 [ 10332 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

issuetype: Bug [ 1 ] => Improvement [ 4 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

Fix Version: 3.0 Beta 1 [ 10332 ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @sim1984

Please, do the same for stored functions.

AS SYSDBA

set term ^;

create or alter function fn_rec(i int) returns int
as
begin
if (i>1) then
return i*fn_rec(i-1);
else
return i;
end^

create or alter function fn_fact(i int) returns int
as
begin
return fn_rec(i);
end^

set term ;^

revoke execute on function fn_rec from test;
grant execute on function fn_rec to fn_fact;
grant execute on function fn_fact to test;

AS TEST

SQL> select fn_fact(10) from rdb$database;
Statement failed, SQLSTATE = 28000
no permission for EXECUTE access to FUNCTION FN_REC
SQL>

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

Denis, it will be great not to hurry so much when preparing samples - correct DDL is no
grant execute on function fn_rec to fn_fact;
but
grant execute on function fn_rec to function fn_fact;

@firebird-automations
Copy link
Collaborator Author

Commented by: @sim1984

I'm sorry.

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Resolved [ 5 ]

QA Status: Done successfully

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Closed [ 6 ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment