Search code examples
sqloracleplsqlcheckmarx

Can we use bind variable in dbms.sql.parse to avoid sql injection


We get checkmarx vulnerability wherever we use dbms.sql.parse. When I check online, bind variables can be used to avoid sql injection however, can we use bind variable in ddl statements?

Please guide me in this

Thanks

wherever we use concatenation, it throws sql injection vulnerability, and also all of them are ddl command.

Let me give the sample one,

Procedure (userid in varchar2)
is
  v_sqlstatement VARCHAR2(1000);
  v_cursor INTEGER;
  v_return INTEGER;
BEGIN
  v_cursor := DBMS_SQL.open_cursor;
  v_sqlstatement := 'DROP USER ' || pp_userid;
  DBMS_SQL.parse (v_cursor, v_sqlstatement, DBMS.SQL.native);
END;

Solution

  • First, unless you are running 8i, you don't need dbms_sql for this. EXECUTE IMMEDIATE is much more user-friendly.

    Second, bind variables aren't used in DDL. You have to string together DDL and execute as dynamic SQL if you're doing it from PL/SQL.

    As for the injection risk, the only risk you have is being asked to drop an important user you'd rather not drop. But that's not much of a risk, because as long as you don't add the "CASCADE" option (don't!), it will raise an error if someone asks it to drop a user with any objects. That's what you want. Users without objects are not important enough to be concerned about.

    However, if you want a more secure pattern, then use the parameter to lookup users in the dictionary and compile the DDL entirely from your own lookups:

    CREATE OR REPLACE PROCEDURE drop_user(in_username IN varchar2)
    AS
      var_username varchar2(128);
    BEGIN
      SELECT MAX(username)
        INTO var_username
        FROM dba_users
       WHERE username = in_username;
    
      IF var_username IS NOT NULL
      THEN
        EXECUTE IMMEDIATE 'DROP USER "'||var_username||'"';
      END IF;
    END;
    

    This ensures that the parameter actually is a valid username. You'll also want to ensure they don't try to drop the procedure owner itself or it will hang.

    Lastly, I would suggest rethinking the need for such a procedure altogether. Only a DBA should have the ability to drop users; why would they want to use a procedure to do so instead of simply issuing the DDL normally?

    If it's not for DBA use, then you shouldn't leave it open-ended. Register in a secure table a list of usernames or username patterns it is allowed to drop and don't act on anything else. Restrict it to the required use-case as much as possible.