createXYZ functions accept a potentially unsafe parameter (E)

[smurthys]

The parameter initialPwd in functions ClassDB.createStudent, ClassDB.createInstructor, and ClassDB.createDBManager is transmitted in clear if the connection used to invoke these functions is not secure.

The potential loss of security is not really an error in ClassDB, but rather of the user. However, in the past, we have removed the function changePassword for the reason that we want to remove any scope for insecure practices.

I propose the following changes:

• Add a new version of the functions without the pwd parameter and use the default pwd policy currently in place
• Alter the existing functions to ignore any pwd passed. Simply call the new functions and also raise a warning that the function is deprecated.

[smurthys]

The code that needs to be fixed is as follows. Because parameter initialPwd has a default value, I believe the order of defining function overloads is crucial it is not possible to define an overload without just that parameter. I will propose a workaround in the next comment.

EDIT: strike-through region for correctness

ClassDB.createStudent
(userName ClassDB.IDNameDomain,
 fullName ClassDB.RoleBase.FullName%Type,
 schemaName ClassDB.IDNameDomain DEFAULT NULL,
 extraInfo ClassDB.RoleBase.ExtraInfo%Type DEFAULT NULL,
 okIfRoleExists BOOLEAN DEFAULT TRUE,
 okIfSchemaExists BOOLEAN DEFAULT TRUE,
 initialPwd VARCHAR(128) DEFAULT NULL
)

[smurthys]

The ideal solution is to define an overload function without the initialPwd parameter (the last parameter), but such an overload should result in an error at function call due to the ambiguity the overload introduces.

However, unlike C++, it seems Postgres overload rules permit overloads that normally result in ambiguous function calls, by simply calling the first matching function. The trouble is the docs mention "search path" which does not apply in our case because both overloads will be in the same schema. And, within a schema, the order of function look up is unclear.

So, I suggest the following actions:

• For starters, define an overload of function createStudent which omits just the initialPwd parameter. Have this function provide all the functionality and enforce the default pwd policy
• Alter the existing function to call the new function and then raise a warning that the function (the one that already exists) is deprecated

If the suggested approach does not work:

• Alter the existing function to implement only the default pwd policy and raise a warning if a non-NULL value is supplied for parameter initialPwd
• This approach is flawed in that it cannot detect function calls that explicitly pass NULL for initialPwd

[KevinKelly25]

I still need to test to see if it works properly but I have started these changes in Add-Safe-CreateXYZ-Functions. I also changed the some of the related documentation and test scripts to support these changes. However, I am not sure how we want to treat testClassDBRolesMgmt.sql because it has a password tests that check for non-default passwords. One possibility of still having those password tests with non-default passwords is to add a second step after each of those which changes the password for that user to the desired password. This way the check functionality of the test script stays intact.

I did some more research on function overloading and also did some tests outside of ClassDB. The overloading in these updates should work but need to be tested once testClassDBRolesMgmt.sql is updated.

[KevinKelly25]

To see which approach suggested by @smurthys would be possible I did some testing. I have found that if a function A is created with (relevant code only)

CREATE OR REPLACE FUNCTION A(a integer, b VARCHAR DEFUALT NULL)

And another function is overloaded to test with

CREATE OR REPLACE FUNCTION A(a integer)

The following things happen.

• Using select A(1, ‘yellow’) will work fine.
• Using select A(1) will result in “HINT: Could not choose a best candidate function. You might need to add explicit type casts.”

This is due to the DEFAULT NULL being in the creation of the function. If you remove the DEFAULT NULL the previous functions work properly.

Extending this test to the current issue I added 2 functions (relevant code only):

CREATE OR REPLACE FUNCTION test2 (userName VARCHAR(10),
                         fullName VARCHAR(10),
                         schemaName VARCHAR(10) DEFAULT NULL,
                         extraInfo VARCHAR(10) DEFAULT NULL,
                         okIfRoleExists BOOLEAN DEFAULT TRUE,
                         okIfSchemaExists BOOLEAN DEFAULT TRUE)

CREATE OR REPLACE FUNCTION test2 (userName VARCHAR(10),
                         fullName VARCHAR(10),
                         schemaName VARCHAR(10) DEFAULT NULL,
                         extraInfo VARCHAR(10) DEFAULT NULL,
                         okIfRoleExists BOOLEAN DEFAULT TRUE,
                         okIfSchemaExists BOOLEAN DEFAULT TRUE,
                     initialPwd VARCHAR(128) DEFAULT NULL)

As expected with the tests of select test2('a','b','c','d',false,false, 'e') aka all 7 parameters the function works correctly and select test2('a','b','c','d',false,false) resulted in “HINT: Could not choose a best candidate function. You might need to add explicit type casts.”

I also tried to get rid of the default and tried to replace the second function overload with

CREATE OR REPLACE FUNCTION test2 (userName VARCHAR(10),
                         fullName VARCHAR(10),
                         schemaName VARCHAR(10) DEFAULT NULL,
                         extraInfo VARCHAR(10) DEFAULT NULL,
                         okIfRoleExists BOOLEAN DEFAULT TRUE,
                         okIfSchemaExists BOOLEAN DEFAULT TRUE,
                     initialPwd VARCHAR(128))

As expected the function cannot be created because there cannot be any input parameters without default after parameters with default.

This means we will have to use @smurthys second solution to solve this issue:

If the suggested approach does not work:

• Alter the existing function to implement only the default pwd policy and raise a warning if a non- NULL value is supplied for parameter initialPwd
• This approach is flawed in that it cannot detect function calls that explicitly pass NULL for initialPwd

[smurthys]

Thanks @KevinKelly25 for testing the approaches. The second approach it is then.

PS: Warning should be raised at the end of the function.

PPS: I wonder if function ClassDB.createRole should also be changed. If we decide to, we can simply remove the last parameter to that function, because that is an internal function with no need to preserve backward compatibility.

[KevinKelly25]

fixed with #211