Dynamically Dangerous Code

There’s a right time to use dynamic SQL, but there’s never a right time for SQL injection.

By Steven Feuerstein

May/June 2015

“I’ve got a real problem on my hands, Steven,” he said. “I followed your advice to create reusable program units rather than one-offs with similar functionality. But now my program is raising errors that I can’t sort out and doing things I never intended.”

It’s never good to hear a programmer worrying about a program with a will of its own, so I hopped into my bright-red PLSQLmobile and raced over to Bob’s cubicle.

“Take a look at this,” said Bob. “I’m using my program in our HR system. You’re familiar with the employees table, right? So let’s update the salary of employee 100.”

BEGIN
 em_update_col_value ('employees',
                      'employee_id',
                      100,
                      'salary',
                      1000);
END;
/
Value of salary updated to 1000

“No problem, right? OK, now let’s try to update the department name and...kaboom!”

BEGIN
 em_update_col_value ('departments',
                      'department_id',
                      10,
                      'department_name',
                      'Jolly Fun');
END;
/
ORA-00933: SQL command not properly ended
ORA-06512: at "SYS.DBMS_SQL", line 1053
ORA-06512: at "QDB_BETA
.EM_UPDATE_COL_VALUE", line 11
ORA-06512: at line 2

“How,” wondered Bob, with a pained expression on his face, “can it work for one column and not another?”

Without even glancing at Bob’s code, I already had a pretty good idea of the problem—or problems. I grabbed his keyboard and typed.

“And how about this?” I asked.

BEGIN
   em_update_col_value ('employees',
         'employee_id=employee_id; 
          delete from employees 
          where employee_id',
          100,
         'salary',
          1000);
END;
/
SELECT * FROM employees 
WHERE employee_id = 100
/
No rows found

“Exactly!” shouted Bob, pointing at the screen. “What’s with that? How can my program delete a row from a table when all it contains is an UPDATE statement?”

To Bob I merely said, “Let’s take a look.”

Even before looking at the em_update_col_value procedure, I was pretty certain of a few things, based on what I’d just witnessed:

• The procedure was executing dynamic SQL.
• Bob’s error handling was minimal or nonexistent.
• Bob had taken no precautions against SQL injection.

But I must admit that I was not quite prepared for the awfulness that presented itself to me when Bob opened em_update_col_value in Oracle SQL Developer, as shown in Listing 1.

Code Listing 1: The original—and awful—em_update_col_value procedure

PROCEDURE em_update_col_value (
   table_in        IN VARCHAR2,
   pkey_col_in     IN VARCHAR2,
   pkey_value_in   IN INTEGER,
   update_col_in   IN VARCHAR2,
   value_in        IN VARCHAR2)
IS
   l_cursor     PLS_INTEGER := DBMS_SQL.open_cursor;
   l_feedback   PLS_INTEGER;
BEGIN
   DBMS_SQL.parse (
      l_cursor,
         'BEGIN update '
      || table_in
      || ' set '
      || update_col_in
      || ' = '
      || value_in
      || ' where '
      || pkey_col_in
      || ' = '
      || pkey_value_in
      || '; END;',
      DBMS_SQL.native);
   l_feedback := DBMS_SQL.execute (l_cursor);
   IF l_feedback > 0
   THEN
      DBMS_OUTPUT.PUT_LINE (
            'Value of '
         || update_col_in
         || ' updated to '
         || value_in);
   ELSE
      DBMS_OUTPUT.PUT_LINE (
            'Update of '
         || update_col_in
         || '  to '
         || value_in
         || ' failed.');
   END IF;
   DBMS_SQL.close_cursor (l_cursor);
END;
/

“See?” said Bob, “No deletes. Just an update. A nice generic procedure for executing an update against any column in any table. Really neat, huh?”

I decided to break the news gently. “Bob, this procedure is a total abomination, but I like your energy and enthusiasm.”

I shared my questions and concerns with him:

• Is this program too reusable to be useful?
• Why is it using DBMS_SQL?
• Where’s the error handling?
• Your program is wide open to SQL injection.

“But don’t feel bad, Bob,” I concluded. “This will be a great learning moment. Shall we explore?” Bob nodded a bit glumly.

“Tell me why you wrote this procedure,” I started off.

Bob recounted to me a session from one of my trainings: “You urged us to find every opportunity to reuse code instead of writing the same or similar code in multiple places. I noticed that in at least five places in our code, we executed updates of a single column in a single row. So I figured I could write a single procedure with dynamic SQL and that we then could call that one procedure instead of writing updates over and over again.”

Bob’s justification sounded reasonable on the surface, but in fact it was a big mistake.

It is better to reuse code whenever possible, but only when that is appropriate. There are several reasons this guideline does not apply to single-column updates: dynamic SQL is more complex than static SQL, it executes more slowly than static SQL, it’s harder to make secure, and it’s harder to debug.

So you want to use it only when absolutely necessary—and that is certainly not the case here. In em_update_col_value, dynamic SQL was used for the sake of convenience.

“When it comes to writing SQL in your PL/SQL code,” I admonished Bob, “you should use dynamic SQL only when it is required—when a user needs to supply some additional information at runtime to complete the SQL statement.

“So I suggest that you abandon em_update_col_value and instead go to wherever the procedure is called and replace it with a new procedure call, such as em_update_salary or em_update_last_name.”

“But then I end up with dozens of different procedures!” Bob exclaimed. “Why not just execute the UPDATE directly in my code?”

“You could do that, but if you put each UPDATE inside a procedure, then it is possible you will reuse that procedure—and not duplicate the UPDATE statement,” I responded. “You are also more likely to write better error handling, and you can add functionality to the update later—in just one place—as your requirements change.”

Bob nodded sadly. I could tell he didn’t like having to throw away his generic procedure.

“But I tell you what, Bob: let’s still go through this procedure and draw out some lessons for the right way to construct a procedure that relies on dynamic SQL. You are sure to run into the need soon.” Bob brightened, and off we went.

“First of all,” I told Bob, “let’s do some basic cleanup in your program so that it is easier to focus on the bigger issues. You should use DBMS_SQL only if you have very complex requirements, such as not knowing at compile time how many columns you are querying or how many variables you must bind. Because that is not the case here, EXECUTE IMMEDIATE is a better fit.

“In addition, your IF statement after the UPDATE is verbose and distracting.” I tapped at the keyboard for a minute or two. “Here. What do you think of this?” I asked, pointing to the code in Listing 2.

Code Listing 2: The updated—but still flawed—em_update_col_value procedure

PROCEDURE em_update_col_value (
   table_in        IN VARCHAR2,
   pkey_col_in     IN VARCHAR2,
   pkey_value_in   IN INTEGER,
   update_col_in   IN VARCHAR2,
   value_in        IN VARCHAR2)
IS
   PROCEDURE report_results
   IS
   BEGIN
      DBMS_OUTPUT.PUT_LINE (
            'Value of '
         || update_col_in
         || CASE SQL%ROWCOUNT WHEN 0 THEN ' NOT' END
         || ' updated to '
         || value_in);
   END;
BEGIN
   EXECUTE IMMEDIATE
         'BEGIN update '
      || table_in
      || ' set '
      || update_col_in
      || ' = '
      || value_in
      || ' where '
      || pkey_col_in
      || ' = '
      || pkey_value_in
      || '; END;';
   report_results;
END;

“Oh, right,” said Bob, “you used a nested subprogram to hide the reporting details. I like the inline CASE expression, too. And really that’s all I need to do with EXECUTE IMMEDIATE? All that other code disappears?”

“That’s correct,” I replied. “There’s no need to declare and manage cursors, no need to parse and then execute. That’s all taken care of for us. Nice, eh?”

“We’re still far from done, though,” I pointed out. “Right now this program assumes that everything is going to proceed without any problem. What’s the chance of that happening? Assume there will be an error. What can we do, then, to make it easier to figure out what went wrong and fix it?”

The challenge with most dynamic SQL requirements is usually not figuring out how to use EXECUTE IMMEDIATE; it’s a simple, elegant statement. No, programmers are much more likely to run into problems constructing the dynamic SQL at runtime. The smallest mistake (forgetting to leave a space between keywords, for example) results in SQL that cannot be parsed.

So any program that contains dynamic SQL should do the following:

• Assign the dynamically constructed SQL statement to a variable and then use EXECUTE IMMEDIATE on that variable
• Add an exception handler that logs the error along with the variable containing the SQL statement
• Reraise the exception so that the calling program knows that something went wrong

Applying these principles, Bob and I updated the em_update_col_value procedure to the code in Listing 3.

Code Listing 3: The further updated em_update_col_value procedure, with error handling

PROCEDURE em_update_col_value (
   table_in        IN VARCHAR2,
   pkey_col_in     IN VARCHAR2,
   pkey_value_in   IN INTEGER,
   update_col_in   IN VARCHAR2,
   value_in        IN VARCHAR2)
IS
   l_statement   VARCHAR2 (32767);
   PROCEDURE report_results ...
BEGIN
   l_statement :=
         'BEGIN update '
      || table_in
      || ' set '
      || update_col_in
      || ' = '
      || value_in
      || ' where '
      || pkey_col_in
      || ' = '
      || pkey_value_in
      || '; END;';
   EXECUTE IMMEDIATE l_statement;
   report_results;
EXCEPTION
   WHEN OTHERS
   THEN
      em_error_log_pkg.log_error (l_statement);
      RAISE;
END;

The new em_error_log_pkg.log_error procedure (called in the updated em_update_col_value procedure) should write out to a log table all of the following:

• SQLCODE – the current error code
• DBMS_UTILITY.FORMAT_ERROR_STACK – the current error message and/or stack (I recommend that you use this instead of SQLERRM)
• DBMS_UTILITY.FORMAT_CALL_TRACE – the execution call stack, answering the question “How did I get here?”
• DBMS_UTILITY.FORMAT_ERROR_BACKTRACE – the trace back to the line number on which the error was raised
• Any information passed to the procedure by the application developer (in this case, the value of l_statement)

Assume for this article, however, that the log_error procedure simply displays the value of l_statement with a call to DBMS_OUTPUT.PUT_LINE.

I asked Bob to run the following block again, against the latest version of the em_update_col_value procedure, to see if it would help with the “ORA-00933: SQL command not properly ended” error message he received when he ran the block against the first version of em_update_col_value. Bob ran

BEGIN
  em_update_col_value ('departments',
                       'department_id',
                       10,
                       'department_name',
                       'Jolly Fun');
END;
/

And then we saw this output, in addition to the error stack:

BEGIN update departments 
set department_name = Jolly Fun 
where department_id = 10; END;

“D’oh!” Bob groaned. “Well that’s obvious enough—now that I can see the string. I forgot to put single quotes around the value. When it’s a number, no problem. When the value is a string, big problem!”

Bob grabbed the keyboard and a moment later had the problem “fixed.”

   l_statement :=
         'BEGIN update '
      || table_in
      || ' set '
      || update_col_in
      || ' = '''
      || value_in
      || ''' where '
      || pkey_col_in
      || ' = '''
      || pkey_value_in
      || '''; END;';

I nodded. “Yep, that fixes the specific problem caused by string values, but the main lesson here is this: assign that expression to a variable so you can easily trace and log the value. You will then be able to diagnose the problem—and achieve the proper dynamic SQL statement construction—much more quickly.”

Bob smiled. One less bug to worry about. Then he frowned. “But what about the deletion from my table? That was really weird, and I have a feeling we haven’t fixed that yet.”

“You read my mind, Bob. That’s right. That problem—and it is far and away the most serious problem with your procedure—still lurks. And it has a name: SQL injection.

“SQL injection occurs when users insert their own text into your SQL statement and cause it to do things you never intended—such as delete a row.”

“Two questions: How could that possibly happen, and how do I make sure it can’t happen?” Bob asked.

“Right,” I responded. “Let’s go back to that delete example I gave you and run it with our new, error-handling-enriched version of the em_update_col_value procedure, but this time I will comment out the semicolon (;) before the delete to force an error.”

BEGIN
  em_update_col_value ('employees',
        'employee_id=employee_id /*;*/
         delete from employees 
         where employee_id',
         100,
         'salary',
         1000);
END;
/
BEGIN update employees set salary = 1000 
where employee_id=employee_id 
/*;*/ delete from employees 
where employee_id = 100; END;

“With a semicolon just before the DELETE keyword, a malicious user terminates the UPDATE statement (which is setting everyone’s salary to 1000) and then starts a brand-new statement inside the block, performing a DELETE. Those semicolons embedded in PL/SQL blocks can really wreak havoc!”

Bob sighed and nodded. “OK, now I see what is going on. What can I do about it?”

“First, Bob, I need to set expectations properly. SQL injection is a security issue. This means that you need to engage with your chief security officer to make sure you are following all of extremememe’s guidelines. It is also a very specialized topic, and I am not a security specialist. So I will share with you some basic steps you should take to shore up your defenses, but I also encourage you to check out the excellent ‘How to write SQL injection proof PL/SQL’ white paper, available on Oracle Technology Network.”

I then presented the following concerns regarding the latest version of the em_update_col_value procedure to Bob:

1. The procedure includes unnecessary construction and execution of a dynamic PL/SQL block.
2. Users can pass their own strings directly to the procedure.
3. The procedure does not check to make sure the table or column names are valid.

I elaborated on each of these to Bob:

1. Dynamic PL/SQL—a string that starts with “DECLARE” or “BEGIN” and ends with “END;”—is much more vulnerable to injection than dynamic SQL (a data manipulation language [DML] or data definition language [DDL] statement), because you can execute procedural logic, invoke stored program units, and so on.

   So if you are not actually executing PL/SQL code, do not put your SQL statements inside a PL/SQL block. In the em_update_col_value procedure, the assignment to the local variable should be nothing more than

      l_statement :=
            'update '
         || table_in
         || ' set '
         || update_col_in
         || ' = '''
         || value_in
         || ''' where '
         || pkey_col_in
         || ' = '''
         || pkey_value_in
         || '''';

2. An end user should never be able to directly insert text into a string executed dynamically. If this is allowed, it will always be very difficult to stop injection.

   User inputs should be tightly constrained and then checked before they are used in a dynamically constructed string. There is no general solution for performing this task. You must analyze each use case and decide how to guard your database from injection. The first step is to avoid concatenation whenever possible and instead bind variable values into the string. You cannot inject into variables! I updated em_update_col_value to use bind variables and showed it to Bob:

   BEGIN
      l_statement :=
            'UPDATE '
         || table_in
         || ' SET '
         || update_col_in
         || ' = :my_value WHERE '
         || pkey_col_in
         || ' = :my_pky';
      EXECUTE IMMEDIATE l_statement 
      USING value_in, pkey_value_in;

   “OK,” said Bob. “I see now that users can’t inject through the values, but what about the table and column names?”

   “Right,” I responded. “That brings us to the final point.”

3. This procedure accepts the name of a table and a column and then concatenates them directly into the string. You cannot bind a table name into a SQL statement with the USING clause; the SQL engine needs all that information before binding to ensure that it is a valid SQL statement. Theoretically, injection could still occur if there is concatenation.

“So,” I explained, “first make sure that users can never enter a table name or a column name directly. It sounds unlikely that they’d be able to, doesn’t it? But make sure! Next, you can further guard against injection via object names by using DBMS_ASSERT subprograms to check that the string is the name of a database object and/or is a valid object name.”

l_statement :=
     'UPDATE '
  || DBMS_ASSERT.SQL_OBJECT_NAME (
        table_in)
  || ' SET '
  || DBMS_ASSERT.SIMPLE_SQL_NAME (
        update_col_in)
  || ' = :my_value WHERE '
  || DBMS_ASSERT.SIMPLE_SQL_NAME (
        pkey_col_in)
  || ' = :my_pky';

“So now with a call to DBMS_ASSERT.SQL_OBJECT_NAME, if I pass a ‘bad’ name for the table, I will see

BEGIN
   em_update_col_value( 
        'employees; more code here',
        'employee_id',
        1000000,
        'salary',
        1000);
END;
/
ORA-44002: invalid object name

“And if I try to play games with the column name, Oracle Database will reject my effort.”

BEGIN
   em_update_col_value( 
       'employees',
       'employee_id;more code here',
       1000000,
       'salary',
       1000);
END;
/
ORA-44003: invalid SQL name

Bob smiled broadly. “I like it when Oracle Database takes care of the heavy lifting for me.”

“Indeed,” I agreed. “But when it comes to SQL injection, Oracle Database makes no promises. You need to do the lion’s share of the work to ensure that your code is not vulnerable to SQL injection.”

Programmers should always strive for a single point of definition (usually a subprogram) for rules, formulas, SQL statements, and magic values. Reuse those subprograms, and look for opportunities to build generic utilities, such as error loggers, that can be reused throughout an application.

Nevertheless, programs that execute dynamic SQL statements are unlikely to be a good fit for reusable code. Dynamic SQL should be utilized only when static implementations are impossible. And when you write a subprogram with dynamic SQL, the need for solid error handling and proactive protection against SQL injection rises significantly.

CREATE TABLE plch_persons
(  pky   INTEGER PRIMARY KEY,
   nm    VARCHAR2 (100))
/
CREATE TABLE plch_trees
(  pky   INTEGER PRIMARY KEY,
   nm    VARCHAR2 (100))
/
BEGIN
   INSERT INTO plch_persons
        VALUES (1, 'Sam');
   INSERT INTO plch_trees
        VALUES (1, 'Oak');
   COMMIT;
END;
/

BEGIN
   plch_show_name ('PLCH_TREES', 1);
   plch_show_name ('PLCH_PERSONS', 1);
END;
/

CREATE OR REPLACE PROCEDURE 
plch_show_name (table_in IN VARCHAR2, pky_in IN INTEGER)
IS
BEGIN
  EXECUTE IMMEDIATE
       'DECLARE l_value VARCHAR2(100); 
        BEGIN SELECT nm INTO l_value 
FROM '
     || table_in
     || ' WHERE pky = '
     || pky_in
     || '; DBMS_OUTPUT.PUT_LINE (l_value); 
          END;';
END;
/

CREATE OR REPLACE PROCEDURE 
plch_show_name (table_in IN VARCHAR2, pky_in IN INTEGER)
IS
  l_value   VARCHAR2 (100);
BEGIN
  EXECUTE IMMEDIATE
       'BEGIN SELECT nm INTO :val FROM '
     || table_in
     || ' WHERE pky = '
     || pky_in
     || '; END;'
     USING OUT l_value;
  DBMS_OUTPUT.PUT_LINE (l_value);
END;
/

CREATE OR REPLACE PROCEDURE 
plch_show_name (table_in IN VARCHAR2, pky_in IN INTEGER)
IS
  l_value   VARCHAR2 (100);
BEGIN
  EXECUTE IMMEDIATE
    'SELECT nm FROM ' || table_in || ' WHERE pky = ' || pky_in
    INTO l_value;
   DBMS_OUTPUT.PUT_LINE (l_value);
END;
/

CREATE OR REPLACE PROCEDURE 
plch_show_name (table_in IN VARCHAR2, pky_in IN INTEGER)
IS
  l_value   VARCHAR2 (100);
BEGIN
  EXECUTE IMMEDIATE
      'SELECT nm FROM '
    || DBMS_ASSERT.sql_object_name (table_in)
    || ' WHERE pky = :pky'
    USING pky_in
    INTO l_value;
   DBMS_OUTPUT.PUT_LINE (l_value);
END;
/