Commented by: Sean Leyne (seanleyne)

As written, the case subject would imply that SYSDBA should not be allowed to:

- add new user defined columns (ie. add "Change_Datetime" to RDB$Relation_Fields)
- add new user defined triggers to track schema changes

If this case was worded to read "Restrict from DML and DDL operations on system defined (RDB$) schema objects to SYSDBA/database owner" then I would support same. But as worded, I do not.

Commented by: @AlexPeshkoff

Some system tables must be opened for dba write access by design. Also keep for dba an ability to extend structure of system tables. But disable any non-dba access to both DML and DDL operations with system tables.

Commented by: @asfernandes

> Also keep for dba an ability to extend structure of system tables.

Why?

Commented by: @AlexPeshkoff

The answer is in Sean's comment.
I was afraid to restrict too strong. But if we decide that adding new fields & triggers to system tables was bad idea - the change is just one line change.
The hardest task is to understand - should we have an ability to add custom changes to system tables or not.

Commented by: @asfernandes

They do not survive backups, so it's not a feature.

If there is a reason to add them, then it must be done completelly.

The only thing I think we should (if possible) support now is user FKs referencing system tables.

Commented by: @pavel-zotov

As of current build (LI-T3.0.0.31789) the following script still can be perform *WITHOUT* producing errors (but now - only from SYSDBA; non privileged user can not do anything with system tables from above mentioned scripts):

set autoddl off;
commit;
set count on; set echo on;
alter table RDB$AUTH_MAPPING add rdb$my_field char(31);
insert into RDB$BACKUP_HISTORY(RDB$BACKUP_ID , RDB$TIMESTAMP , RDB$BACKUP_LEVEL , RDB$GUID , RDB$SCN , RDB$FILE_NAME) values(null, null, null, null, null, null);
alter table RDB$BACKUP_HISTORY add rdb$my_field char(31);
alter table RDB$BACKUP_HISTORY add constraint RDB$BACKUP_ID_dummy check (RDB$BACKUP_ID is not distinct from RDB$BACKUP_ID);
alter table RDB$BACKUP_HISTORY alter RDB$BACKUP_ID set default 2147483647;
alter table RDB$CHARACTER_SETS add rdb$my_field char(31);
alter table RDB$CHARACTER_SETS add constraint RDB$CHARACTER_SET_NAME_dummy check (RDB$CHARACTER_SET_NAME is not distinct from RDB$CHARACTER_SET_NAME);
alter table RDB$CHARACTER_SETS alter RDB$CHARACTER_SET_NAME set default 'C';
alter table RDB$CHARACTER_SETS drop RDB$CHARACTER_SET_NAME;
alter table RDB$CHECK_CONSTRAINTS add rdb$my_field char(31);
alter table RDB$CHECK_CONSTRAINTS add constraint RDB$CONSTRAINT_NAME_dummy check (RDB$CONSTRAINT_NAME is not distinct from RDB$CONSTRAINT_NAME);
alter table RDB$CHECK_CONSTRAINTS alter RDB$CONSTRAINT_NAME set default 'C';
alter table RDB$COLLATIONS add rdb$my_field char(31);
alter table RDB$COLLATIONS add constraint RDB$COLLATION_NAME_dummy check (RDB$COLLATION_NAME is not distinct from RDB$COLLATION_NAME);
alter table RDB$COLLATIONS alter RDB$COLLATION_NAME set default 'C';
alter table RDB$COLLATIONS drop RDB$COLLATION_NAME;
alter table RDB$DATABASE add rdb$my_field char(31);
alter table RDB$DATABASE add constraint RDB$DESCRIPTION_dummy check (RDB$DESCRIPTION is not distinct from RDB$DESCRIPTION);
alter table RDB$DATABASE alter RDB$DESCRIPTION set default 'test_for_blob';
alter table RDB$DATABASE drop RDB$DESCRIPTION;
insert into RDB$DB_CREATORS(RDB$USER , RDB$USER_TYPE) values(null, null);
update RDB$DB_CREATORS set RDB$USER=null, RDB$USER_TYPE=null rows 1;
delete from RDB$DB_CREATORS;
alter table RDB$DB_CREATORS add rdb$my_field char(31);
alter table RDB$DB_CREATORS add constraint RDB$USER_dummy check (RDB$USER is not distinct from RDB$USER);
alter table RDB$DB_CREATORS alter RDB$USER set default 'C';
alter table RDB$DB_CREATORS drop RDB$USER;
alter table RDB$DEPENDENCIES add rdb$my_field char(31);
alter table RDB$EXCEPTIONS add rdb$my_field char(31);
alter table RDB$EXCEPTIONS drop RDB$EXCEPTION_NAME;
alter table RDB$FIELDS add rdb$my_field char(31);
alter table RDB$FIELDS add constraint RDB$FIELD_NAME_dummy check (RDB$FIELD_NAME is not distinct from RDB$FIELD_NAME);
alter table RDB$FIELDS alter RDB$FIELD_NAME set default 'C';
alter table RDB$FIELDS drop RDB$FIELD_NAME;
alter table RDB$FIELD_DIMENSIONS add rdb$my_field char(31);
alter table RDB$FILES add rdb$my_field char(31);
alter table RDB$FILES drop RDB$FILE_NAME;
alter table RDB$FILTERS add rdb$my_field char(31);
alter table RDB$FILTERS drop RDB$FUNCTION_NAME;
alter table RDB$FORMATS add rdb$my_field char(31);
alter table RDB$FORMATS add constraint RDB$RELATION_ID_dummy check (RDB$RELATION_ID is not distinct from RDB$RELATION_ID);
alter table RDB$FORMATS alter RDB$RELATION_ID set default 32767;
alter table RDB$FUNCTIONS add rdb$my_field char(31);
alter table RDB$FUNCTION_ARGUMENTS add rdb$my_field char(31);
alter table RDB$GENERATORS add rdb$my_field char(31);
alter table RDB$GENERATORS add constraint RDB$GENERATOR_NAME_dummy check (RDB$GENERATOR_NAME is not distinct from RDB$GENERATOR_NAME);
alter table RDB$GENERATORS alter RDB$GENERATOR_NAME set default 'C';
alter table RDB$GENERATORS drop RDB$GENERATOR_NAME;
alter table RDB$INDEX_SEGMENTS add rdb$my_field char(31);
alter table RDB$INDEX_SEGMENTS add constraint RDB$INDEX_NAME_dummy check (RDB$INDEX_NAME is not distinct from RDB$INDEX_NAME);
alter table RDB$INDEX_SEGMENTS alter RDB$INDEX_NAME set default 'C';
alter table RDB$INDICES add rdb$my_field char(31);
alter table RDB$INDICES alter RDB$INDEX_NAME set default 'C';
alter table RDB$INDICES drop RDB$INDEX_NAME;
alter table RDB$LOG_FILES add rdb$my_field char(31);
alter table RDB$LOG_FILES drop RDB$FILE_NAME;
alter table RDB$PACKAGES add rdb$my_field char(31);
alter table RDB$PACKAGES drop RDB$PACKAGE_NAME;
alter table RDB$PAGES add rdb$my_field char(31);
alter table RDB$PAGES add constraint RDB$PAGE_NUMBER_dummy check (RDB$PAGE_NUMBER is not distinct from RDB$PAGE_NUMBER);
alter table RDB$PAGES alter RDB$PAGE_NUMBER set default 2147483647;
alter table RDB$PAGES drop RDB$PAGE_NUMBER;
alter table RDB$PROCEDURES add rdb$my_field char(31);
alter table RDB$PROCEDURE_PARAMETERS add rdb$my_field char(31);
alter table RDB$REF_CONSTRAINTS add rdb$my_field char(31);
alter table RDB$REF_CONSTRAINTS drop RDB$CONSTRAINT_NAME;
alter table RDB$RELATIONS add rdb$my_field char(31);
alter table RDB$RELATIONS add constraint RDB$VIEW_BLR_dummy check (RDB$VIEW_BLR is not distinct from RDB$VIEW_BLR);
alter table RDB$RELATIONS alter RDB$VIEW_BLR set default 'test_for_blob';
alter table RDB$RELATIONS drop RDB$VIEW_BLR;
alter table RDB$RELATION_CONSTRAINTS add rdb$my_field char(31);
alter table RDB$RELATION_CONSTRAINTS alter RDB$CONSTRAINT_NAME set default 'C';
alter table RDB$RELATION_CONSTRAINTS drop RDB$CONSTRAINT_NAME;
alter table RDB$RELATION_FIELDS add rdb$my_field char(31);
alter table RDB$RELATION_FIELDS alter RDB$FIELD_NAME set default 'C';
alter table RDB$ROLES add rdb$my_field char(31);
alter table RDB$ROLES add constraint RDB$ROLE_NAME_dummy check (RDB$ROLE_NAME is not distinct from RDB$ROLE_NAME);
alter table RDB$ROLES alter RDB$ROLE_NAME set default 'C';
alter table RDB$ROLES drop RDB$ROLE_NAME;
alter table RDB$SECURITY_CLASSES add rdb$my_field char(31);
alter table RDB$SECURITY_CLASSES add constraint RDB$SECURITY_CLASS_dummy check (RDB$SECURITY_CLASS is not distinct from RDB$SECURITY_CLASS);
alter table RDB$SECURITY_CLASSES alter RDB$SECURITY_CLASS set default 'C';
alter table RDB$SECURITY_CLASSES drop RDB$SECURITY_CLASS;
alter table RDB$TRANSACTIONS add rdb$my_field char(31);
alter table RDB$TRANSACTIONS drop RDB$TRANSACTION_ID;
alter table RDB$TRIGGERS add rdb$my_field char(31);
alter table RDB$TRIGGERS add constraint RDB$TRIGGER_NAME_dummy check (RDB$TRIGGER_NAME is not distinct from RDB$TRIGGER_NAME);
alter table RDB$TRIGGERS alter RDB$TRIGGER_NAME set default 'C';
alter table RDB$TRIGGERS drop RDB$TRIGGER_NAME;
alter table RDB$TRIGGER_MESSAGES add rdb$my_field char(31);
alter table RDB$TRIGGER_MESSAGES alter RDB$TRIGGER_NAME set default 'C';
insert into RDB$TYPES(RDB$FIELD_NAME , RDB$TYPE , RDB$TYPE_NAME , RDB$DESCRIPTION , RDB$SYSTEM_FLAG) values(null, null, null, null, null);
alter table RDB$TYPES add rdb$my_field char(31);
alter table RDB$TYPES alter RDB$FIELD_NAME set default 'C';
alter table RDB$TYPES drop RDB$FIELD_NAME;
alter table RDB$USER_PRIVILEGES add rdb$my_field char(31);
alter table RDB$USER_PRIVILEGES alter RDB$USER set default 'C';
alter table RDB$VIEW_RELATIONS add rdb$my_field char(31);
rollback;

AFAIU, some or all of these statements anyway will be avaliable to SYSDBA in the future, so the test for this should be concerned only for NON-privileged user and not for SYSDBA - correct ?

Commented by: @AlexPeshkoff

Yes, some of DML commands will remain available for SYSDBA. But to say how much of them - let's wait a little for Sean to be able to say his mind.
Adriano, backups is serious argument but if someone was using such tricks in production (with nbackup instead gbak for backups) it will be very hard for him to upgrade to new FB version.
So first of all I wonder - Sean, is it just theoretical assumption re non-std fields/triggers in system tables or something more?

Commented by: @sim1984

All these tricks with expandable system tables can be easily cut off DDL triggers. To date, only statements COMMENT ON, GRANT and REVOKE are not covered by them.
So I do not see any reason to leave SYSDBA create triggers on system tables or allow to add custom fields.

Commented by: @pavel-zotov

It should be mentioned that NON-privileged user still able to create a table and add FOREIGN KEY to it which references to RDB$-tables.

Sample:

shell del C:\MIX\firebird\QA\fbt-repo\tmp\c4731.fdb 2>nul;

create database 'localhost/3330:C:\MIX\firebird\QA\fbt-repo\tmp\c4731.fdb';
commit;

drop user tmp$c4731;
commit;
create user tmp$c4731 password '123';
commit;
revoke all on all from tmp$c4731;
grant create table to tmp$c4731;
grant drop any table to tmp$c4731;
grant alter any table to tmp$c4731;
commit;

connect 'localhost/3330:C:\MIX\firebird\QA\fbt-repo\tmp\c4731.fdb' user 'tmp$c4731' password '123'; ------------- RE-connect as non-priv. user

select current_user, current_role from rdb$database;

recreate table test1(
rdb$backup_level int,
rdb$backup_id int,
constraint test1_fk foreign key (rdb$backup_level, rdb$backup_id )
references rdb$backup_history(rdb$backup_level, rdb$backup_id )
);
commit;

recreate table test2(
rdb$relation_name char(31) character set unicode_fss collate unicode_fss
,constraint test2_fk foreign key (rdb$relation_name)
references rdb$relations(rdb$relation_name)
);
commit;

recreate table test3(
rdb$field_name char(31) character set unicode_fss collate unicode_fss
,rdb$relation_name char(31) character set unicode_fss collate unicode_fss
,constraint test3_fk foreign key (rdb$field_name, rdb$relation_name)
references rdb$relation_fields (rdb$field_name, rdb$relation_name)
);
commit;

set echo on;
show table test1;
show table test2;
show table test3;
set echo off;

drop table test1;
drop table test2;
drop table test3;
commit;

STDOUT:

USER ROLE
=============================== ===============================
TMP$C4731 NONE

show table test1;
RDB$BACKUP_LEVEL INTEGER Nullable
RDB$BACKUP_ID INTEGER Nullable
CONSTRAINT TEST1_FK:
Foreign key (RDB$BACKUP_LEVEL, RDB$BACKUP_ID) References RDB$BACKUP_HISTORY (RDB$BACKUP_LEVEL, RDB$BACKUP_ID)
show table test2;
RDB$RELATION_NAME CHAR(31) CHARACTER SET UNICODE_FSS Nullable
CONSTRAINT TEST2_FK:
Foreign key (RDB$RELATION_NAME) References RDB$RELATIONS (RDB$RELATION_NAME)
show table test3;
RDB$FIELD_NAME CHAR(31) CHARACTER SET UNICODE_FSS Nullable
RDB$RELATION_NAME CHAR(31) CHARACTER SET UNICODE_FSS Nullable
CONSTRAINT TEST3_FK:
Foreign key (RDB$FIELD_NAME, RDB$RELATION_NAME) References RDB$RELATION_FIELDS (RDB$FIELD_NAME, RDB$RELATION_NAME)
set echo off;

Tested on WI-T3.0.0.31789.

PS. Adding clause 'ON UPDATE | DELETE CASCADE | SET NULL' in FK-definition is now PROHIBITED for non-privileged user (but allowed for SYSDBA).

Commented by: @pavel-zotov

> but allowed for SYSDBA

... and this is BAD.

Because:

C:\MIX\firebird\QA\fbt-repo\tmp>isql -q
SQL> create database 'localhost/3330:C:\MIX\firebird\QA\fbt-repo\tmp\c4731x.fdb' user 'sysdba' password 'masterke';
SQL> commit;
SQL> recreate table test(x int);
SQL> commit;
SQL>
SQL> recreate table test(
CON> rdb$relation_name char(31) character set unicode_fss collate unicode_fss
CON> ,constraint test_fk foreign key (rdb$relation_name) references rdb$relations(rdb$relation_name)
CON> );
SQL> commit;
SQL>
SQL> recreate table test(
CON> rdb$field_name char(31) character set unicode_fss collate unicode_fss
CON> ,rdb$relation_name char(31) character set unicode_fss collate unicode_fss
CON> ,constraint test_fk foreign key (rdb$field_name, rdb$relation_name)
CON> references rdb$relation_fields (rdb$field_name, rdb$relation_name)
CON> );
Statement failed, SQLSTATE = XX001
unsuccessful metadata update
-RECREATE TABLE TEST failed
-database file appears corrupt (C:\MIX\FIREBIRD\QA\FBT-REPO\TMP\C4731X.FDB)
-wrong page type
-page 0 is of wrong type (expected index root, found database header)
SQL>

Commented by: @dyemanov

User-defined triggers on system tables never worked reliably and should not be supported, especially given the new DDL triggers feature.
As for user-defined columns on system tables, my personal opinion is that they should be prohibited too, even if sometimes being useful.

Commented by: @pavel-zotov

Dmitry,

and what about ability to create foreign keys (with or without adding 'ON DELETE | UPDATE' clause) ? Should this be prohibited also ?

Commented by: @dyemanov

As for me, this should. Everything outside the engine should work as if system tables were read-only (or look like "virtual" tables).

Commented by: @AlexPeshkoff

Pavel, access to system tables seems to be prohibited (except a few where it's really needed). Can you recheck once more?

Commented by: @mrotteveel

What do we do with explicitly documented use cases of these loopholes, for example as documented on http://www.firebirdsql.org/file/documentation/reference_manuals/reference_material/html/langrefupd25-ddl-filter.html:
"
User-defined mnemonics: If you want to define mnemonics for your own BLOB subtypes, you can add them to the RDB$TYPES system table as shown below. Once committed, the mnemonics can be used in subsequent filter declarations.

insert into rdb$types \(rdb$field\_name, rdb$type, rdb$type\_name\)
  values \('RDB$FIELD\_SUB\_TYPE', \-33, 'MIDI'\)

The value for rdb$field_name must always be 'RDB$FIELD_SUB_TYPE'. If you define your mnemonics in all-uppercase, you can use them case-insensitively and unquoted in your filter declarations.
"

NOTE: I haven't tested this yet in recent versions

Commented by: @AlexPeshkoff

Mark, DML for user records in RDB$TYPES works.

Commented by: @pavel-zotov

> Pavel, access to system tables seems to be prohibited (except a few where it's really needed). Can you recheck once more?

Alex,

I've created more or less complex script which does following:

1) creates log table with name 'vulnerable_on_sys_tables' for accumulating vulnerable expression;

2) reads all RDB$-tables which have PK / UNIQUE constraints and build upon their data expressions like:
create table RDB_**** (
<fields_from_RDB$X>
,constraint FK1 references RDB$... ( <unique_set1> )
,constraint FK2 references RDB$... ( <unique_set2> )
);
-- i.e. 'RDB_****' is a NEW table which 'attempts' to create FOREIGN KEY(s) into master RDB$ one.
All these expressions should FAIL, no matter of user (SYSDBA or not).

3) reads again all RDB$-tables and tries to make following maniputations with each of them:
3.1) DML: insert, delete, update and "select with lock" one record;
3.2) DDL: add new field, alter column SET NULL, add check constraint, alter column set DEFAULT value, drop field, drop all table.

All these expressions should FAIL if they are issued by NON-sysdba.
SOME of them can be passed OK if current user = SYSDBA.

RESULTS:

1) NON-privileged user can NOT successfully execute any of mentioned statements;
2) SYSDBA can do 5 (five) statements:

insert into RDB$BACKUP_HISTORY(RDB$BACKUP_ID , RDB$TIMESTAMP , RDB$BACKUP_LEVEL , RDB$GUID , RDB$SCN , RDB$FILE_NAME) values(null, null, null, null, null, null);

insert into RDB$DB_CREATORS(RDB$USER , RDB$USER_TYPE) values(null, null);

update RDB$DB_CREATORS set RDB$USER=null, RDB$USER_TYPE=null rows 1;

delete from RDB$DB_CREATORS;

insert into RDB$TYPES(RDB$FIELD_NAME , RDB$TYPE , RDB$TYPE_NAME , RDB$DESCRIPTION , RDB$SYSTEM_FLAG) values(null, null, null, null, null);

Two sripts (for running by SYSDBA and by non-privileged 'COOLHACKER') and logs of their execution plese see in attach.

I can implement test (for running under fbt_run utility) based on script named "c4731-coolhacker.sql" if no any further changes are planned in FB engine related to this topic.

Commented by: @pavel-zotov

PS.

Seems that also one bad news present.
Scripts work fine only on WI-T3.0.0.31801 (client & server on the same host, OS = Win XP2).

On Linux LI-T3.0.0.31805 (also client and server on the same host) Firebird hangs -- no messages in trace after:

create user coolhacker password '123'
0 records fetched
0 ms

2015-04-22T21:08:32.5440 (24206:0x7f526f710690) TRACE_INIT
SESSION_1

2015-04-22T21:08:32.5440 (24206:0x7f526f710690) START_TRANSACTION
/opt/fb30ss/security3.fdb (ATT_50, Garbage Collector, NONE, <internal>)
(TRA_135, READ_COMMITTED | REC_VERSION | WAIT | READ_ONLY)

2015-04-22T21:08:32.5500 (24206:0x7f526f70db90) TRACE_INIT
SESSION_1

2015-04-22T21:08:32.5500 (24206:0x7f526f70db90) ATTACH_DATABASE
/opt/fb30ss/security3.fdb (ATT_55, SYSDBA:NONE, NONE, <internal>)

2015-04-22T21:08:32.5520 (24206:0x7f526f70db90) START_TRANSACTION
/opt/fb30ss/security3.fdb (ATT_55, SYSDBA:NONE, NONE, <internal>)
(TRA_136, CONCURRENCY | WAIT | READ_WRITE)

2015-04-22T21:08:42.4880 (24206:0x7f526f70b988) DETACH_DATABASE
/opt/fb30ss/security3.fdb (ATT_52, SYSDBA:NONE, NONE, <internal>)

2015-04-22T21:08:42.4880 (24206:0x7f526f70b988) TRACE_FINI
SESSION_1

No messages in firebird.log.
I get stacktrace from FB process when it was in hanged state - see attached file.

Commented by: @AlexPeshkoff

For me all your scripts work on linux as expected. In your stacktrace I see a thread, actively executing in trace plugin deep in commit handler, i.e. there is no hang at the first look. Anyway - it's bad idea to poison a ticket with ither unrelated bugs.

Commented by: @AlexPeshkoff

Pavel, that's sligtly modified test scritps FYI

Commented by: @pavel-zotov

Test was done successfully.