Preventing dynamic sql from injection

I have an assigment to build account search tool in our Oracle database. I ended with dynamic sql, but I know there is risk of sql injection. The output of output cursor is then further processed by Java (passing to AccountResource and out to asking entity). Could anybody share some opinions, how could evil hacker inject code below and how to properly protect my dynamic query? This is just sample code, real procedure will have more input parameters, % replacements, ..., but the functionality is captured 100%. I already searched forums for the answers, but the main knowledge was to avoid dynamic queries.

Could he (The Evil Hacker) somehow get to top_secret_column? Could he somehow drop my_important_table?

CREATE TABLE my_accounts (account_id NUMBER, user_name VARCHAR2(30),  email VARCHAR2(30), top_secret_column VARCHAR2(30));
--CREATE TABLE my_important_table(id number);

DECLARE
  -- input variables  
  p_cur SYS_REFCURSOR; -- OUT
  p_exact_search NUMBER := 1;
  p_search_email VARCHAR2(50) := '[email protected]';
  p_search_name VARCHAR2(50) := null;
  --
  v_sql VARCHAR2(4000);
  FUNCTION get_query_text(p_column       VARCHAR2,
                          p_value        VARCHAR2,
                          p_exact_search NUMBER) RETURN VARCHAR2 IS
    v_res VARCHAR2(200);
  BEGIN
    IF p_exact_search = 1
    THEN
      v_res := ' UPPER(' || p_column || ') = UPPER(''' || p_value || ''')';
    ELSE
      v_res := ' UPPER(' || p_column || ') LIKE (UPPER(''%' || p_value ||
               '%''))';
    END IF;
    RETURN chr(10) || ' AND' || v_res;
  END;
BEGIN
  v_sql := 'SELECT account_id, user_name, email
  FROM my_accounts
 WHERE 1 = 1';
  IF p_search_email IS NOT NULL
  THEN
    v_sql := v_sql || get_query_text('email', p_search_email, p_exact_search);
  END IF;
  IF p_search_name IS NOT NULL
  THEN
    v_sql := v_sql ||
             get_query_text('user_name', p_search_name, p_exact_search);
  END IF;
  --dbms_output.put_line(v_sql);
  OPEN p_cur FOR v_sql;
  CLOSE p_cur;
END;

I already searched forums and I want my code to be Evil-Hacker-proof.

Solution

If you are using bind variables rather than || to splice in the parameters into your SQL, you will be protected from script injection (in the form of calling an unknown function injected in the string with escapes to get out of your quotes), as Oracle will not evaluate/execute any function calls that the caller attempts to embed in those strings if you pass them in as binds. Binds happen just prior to exec time, but after parse, so there's no way a structural change (like calling a function) can be introduced at that point.

So change your code to use :placeholders and binds which are enabled or disabled (so you don't have to vary your bind list) rather than assembling them conditionally:
```
OPEN p_cur FOR '
SELECT account_id, user_name, email
  FROM my_accounts
 WHERE (email = :email OR :email IS NULL OR :exact_search = 0)
   AND (user_name= :user_name OR :user_name IS NULL OR :exact_search = 0)
   AND (UPPER(email) LIKE UPPER(''%''||:email||''%'') OR :email IS NULL OR :exact_search = 1)
   AND (UPPER(user_name) LIKE UPPER(''%''||:user_name||''%'') OR :user_name IS NULL OR :exact_search = 1)
   ' 
USING IN p_search_email, p_search_email, p_exact_search, 
         p_search_name, p_search_name, p_exact_search,
         p_search_email, p_search_email, p_exact_search, 
         p_search_name, p_search_name, p_exact_search;
```
This is not only much safer than ||, and much cleaner and more succinct code, it will also perform better (by avoiding repeated hard parses) and make your DBA happy because you won't be creating innumerable unsharable cursors and shredding the shared pool.

Note: You'll notice I still have some || to handle the LIKE expressions, but those are internal to the string itself and concatenate a bind variable after the bind, not a string at parse time, so aren't vulnerable in the way your other || operators are.
Compile your procedure as AUTHID CURRENT_USER so that if you made a mistake and there's some vulnerability, the caller will operate under their own permissions and won't get any benefit from using your procedure's owner permissions. You would of course need to grant all possible callers whatever individual object permissions are needed for the procedure to do what it does (GRANT SELECT ON my_accounts TO .... )