Take the 2-minute tour ×
Database Administrators Stack Exchange is a question and answer site for database professionals who wish to improve their database skills and learn from others in the community. It's 100% free, no registration required.

I'm working on application that uses dynamic query to do a select statement based on user input, after discussing security with DBAs they want me to convert my dynamic select statement into Stored Procedure.

I have built dynamic sql using MSSQL but I can not figure out how to convert it to Oracle SQL.

CREATE PROCEDURE GetCustomer
@FirstN nvarchar(20) = NULL,
@LastN nvarchar(20) = NULL,
@CUserName nvarchar(10) = NULL, 
@CID nvarchar(15) = NULL as
DECLARE @sql nvarchar(4000),
SELECT @sql = 'C_FirstName, C_LastName, C_UserName, C_UserID ' + 
'FROM CUSTOMER ' +
'WHERE 1=1 ' +

IF @FirstN  IS NOT NULL
SELECT @sql = @sql + ' AND C_FirstName like @FirstN '
IF @LastN  IS NOT NULL 
SELECT @sql = @sql + ' AND C_LastName like @LastN '
IF @CUserName IS NOT NULL
SELECT @sql = @sql + ' AND C_UserName like @CUserName '
IF @CID IS NOT NULL 
SELECT @sql = @sql + ' AND C_UserID like @CID '
EXEC sp_executesql @sql, N'@C_FirstName nvarchar(20), @C_LastName nvarchar(20), @CUserName nvarchar(10), @CID nvarchar(15)',
                   @FirstN, @LastN, @CUserName, @CID

*please note that I want to prevent SQL injection I do not want to just add string together

**i have built a separate class for creating this dynamic query for my application in .net I have almost 1000 lines of code to handle everything and prevent sql injection, but DBAs have told me that they want stored procedures so they can control input and output.

share|improve this question
    
Why are you wasting 1000 lines of code, when your SQL-Server stored procedure is immune against sql injection cf. dba.stackexchange.com/questions/790/…? –  bernd_k Jan 22 '11 at 16:47

4 Answers 4

up vote 5 down vote accepted

This might give you an idea:

create table Customer (
  c_firstname varchar2(50),
  c_lastname  varchar2(50),
  c_userid    varchar2(50)
);

insert into Customer values ('Micky' , 'Mouse', 'mm');
insert into Customer values ('Donald', 'Duck' , 'dd');
insert into Customer values ('Peter' , 'Pan'  , 'pp');

create or replace function GetCustomer(
  FirstN    varchar2 := null,
  LastN     varchar2 := null,
  CID       varchar2 := null
) return sys_refcursor
as
  stmt varchar2(4000);
  ret sys_refcursor;
begin
  stmt := 'select * from Customer where 1=1';

  if  FirstN is not null then
      stmt := stmt || ' and c_firstname like ''%' || FirstN || '%''';
  end if;

  if  LastN is not null then
      stmt := stmt || ' and c_lastname like ''%' || LastN  || '%''';
  end if;

  if  CID is not null then
      stmt := stmt || ' and c_userid like ''%' || CID || '%''';
  end if;

  dbms_output.put_line(stmt);

  open ret for stmt;
  return ret;
end;
/

Later, in SQL*Plus:

set serveroutput on size 100000 format wrapped

declare
  c sys_refcursor;
  fn Customer.c_firstname%type;
  ln Customer.c_lastname %type;
  id Customer.c_userid   %type;
begin
  c := GetCustomer(LastN => 'u');

  fetch c into fn, ln, id;
  while  c%found loop
      dbms_output.put_line('First Name: ' || fn);
      dbms_output.put_line('Last Name:  ' || ln);
      dbms_output.put_line('user id:    ' || id);

      fetch c into fn, ln, id;
  end loop;

  close c;
end;
/

Edit: The comment is right, and the procedure is subject to SQL injection. So, in order to prevent that, you could go with bind variables such as in this modified procedure:

create or replace function GetCustomer(
  FirstN    varchar2 := null,
  LastN     varchar2 := null,
  CID       varchar2 := null
) return sys_refcursor
as
  stmt varchar2(4000);
  ret  sys_refcursor;

  type parameter_t is table of varchar2(50);
  parameters parameter_t := parameter_t();
begin
  stmt := 'select * from Customer where 1=1';

  if  FirstN is not null then
      parameters.extend;
      parameters(parameters.count) := '%' || FirstN || '%';
      stmt := stmt || ' and c_firstname like :' || parameters.count;
  end if;

  if  LastN is not null then
      parameters.extend;
      parameters(parameters.count) := '%' || LastN || '%';
      stmt := stmt || ' and c_lastname like :' || parameters.count;
  end if;

  if  CID is not null then
      parameters.extend;
      parameters(parameters.count) := '%' || CID || '%';
      stmt := stmt || ' and c_userid like :' || parameters.count;
  end if;


  if    parameters.count = 0 then
        open ret for stmt;
  elsif parameters.count = 1 then
        open ret for stmt using parameters(1);
  elsif parameters.count = 2 then
        open ret for stmt using parameters(1), parameters(2);
  elsif parameters.count = 3 then
        open ret for stmt using parameters(1), parameters(2), parameters(3);
  else  raise_application_error(-20800, 'Too many parameters');
  end   if;

  return ret;
end;
/

Note, that now, whatever the input, the select statement becomes something like select ... from ... where 1=1 and col1 like :1 and col2 :2 ... which is obviously much safer.

share|improve this answer
1  
if FirstN is not null then stmt := stmt || ' and c_firstname like ' || FirstN; end if; i can see that you in your example have combined the string? how do you know that users is not putting sql injection in parameter? –  Vladimir Oselsky Jan 17 '11 at 21:58
    
é Your solution using bind variables looks very promising. –  bernd_k Jan 19 '11 at 7:06
1  
@Sauce I added a second answer to demonstrate why René's code is immune to sql injecttion –  bernd_k Jan 22 '11 at 12:53

You don't necessarily need dynamic SQL just because certain where conditions don't apply when they are not present.

SELECT C_FirstName, C_LastName, C_UserName, C_UserID FROM CUSTOMER
WHERE (FirstN IS NULL    OR C_FirstName LIKE FirstN)
AND   (LastN IS NULL     OR C_LastName LIKE LastN)
AND   (CUserName IS NULL OR C_UserName LIKE CUserName)
AND   (CID IS NULL       OR C_UserID LIKE CID)

Placing this code in a store procedure inside a package is a excellent idea.

Oracle provides some excellent documentation that can get you up to speed on stored procedures and packages. You might want to start out with the Concepts Guide to get an understanding of how Oracle works, then move on to the SQL Language Reference and PL/SQL Language Reference for information pertinent to your current task.

share|improve this answer
    
so if i understand your code. you are only care about variable that are not null for instance if i pass FirstN and CID what i execute will look like this SELECT C_FirstName, C_LastName, C_UserName, C_UserID FROM CUSTOMER WHERE C_FirstName LIKE FirstN AND C_UserID LIKE CID –  Vladimir Oselsky Jan 17 '11 at 22:04
    
What you execute will look identical; the query logic uses takes care of whether the variable is used to limit rows from the results. –  Leigh Riffel Jan 17 '11 at 22:26
    
what about validating the variables to make sure that someone does not enter sql injection? –  Vladimir Oselsky Jan 18 '11 at 2:05
    
Assuming you put this in a package it is Embedded SQL not Dynamic SQL, therefore it is not vulnerable to SQL injection. For more information see the Oracle whitepaper "How to write SQL injection proof PL/SQL" at oracle.com/technetwork/database/features/plsql/overview/… –  Leigh Riffel Jan 18 '11 at 5:02
    
Ignoring caseinsensitive string compares this is a correct solution. Looking at performance of individual queries it is not the optimal. When used in a stored procedure it doesn't cause problems when many queries with different parameters are executed. It is a good baseline for performance comparisons. –  bernd_k Jan 19 '11 at 6:51

This is no independent answer, but an added explanation to René Nyffenegger's code using bind variables.

SaUce asked why this code is immune to sql injection.

Here I change René's code to not execute the dynamic statement, but to display it:

create or replace function GetCustomer(
  FirstN    varchar2 := null,
  LastN     varchar2 := null,
  CID       varchar2 := null
) return sys_refcursor
as
  stmt varchar2(4000);
  ret  sys_refcursor;

  type parameter_t is table of varchar2(50);
  parameters parameter_t := parameter_t();
begin
  stmt := 'select * from Customer where 1=1';

  if  FirstN is not null then
      parameters.extend;
      parameters(parameters.count) := '%' || FirstN || '%';
      stmt := stmt || ' and c_firstname like :' || parameters.count;
  end if;

  if  LastN is not null then
      parameters.extend;
      parameters(parameters.count) := '%' || LastN || '%';
      stmt := stmt || ' and c_lastname like :' || parameters.count;
  end if;

  if  CID is not null then
      parameters.extend;
      parameters(parameters.count) := '%' || CID || '%';
      stmt := stmt || ' and c_userid like :' || parameters.count;
  end if;


   OPEN ret for SELECT stmt FROM DUAL;


  return ret;
end;
/

Now I can try calls like

Var r refcursor
exec  GetCustomer(:r, 'Micky', '')
print r

The result is:

select * from Customer where 1=1 and FirstN like :1

In René's code this will be executed as:

select * from Customer where 1=1  and FirstN like :1 using 'Micky'

You see, it doesn't matter which value is supplied for FirstN. It never changes the meaning of the query.

There are further reasons to use variable binding, which are hard to grasp for developers with an SQL-Server background. They depend on the way how Oracle stores precompiled execution plans in the shared pool. Not using bind variables gives different statements and different execution plans, while using bind variables uses a single execution plan.

share|improve this answer

For your stored procedure, the best migration to oracle would go like

CREATE or replace PROCEDURE GetCustomer 
    p_FirstN nvarchar2 := NULL, 
    p_LastN nvarchar2 := NULL, 
    p_CUserName nvarchar2 := NULL, 
    p_CID nvarchar2 := NULL, 
    MyRefCursor IN OUT typRefCursor
as 
begin   

    IF p_FirstN IS NULL then
        if p_p_LastN is null then
            if p_CUserName is null then
                if  p_CID is null then
                    Open MyRefCursor for Select C_FirstName, C_LastName, C_UserName, C_UserID FROM CUSTOMER; 
                else
                    Open MyRefCursor for Select C_FirstName, C_LastName, C_UserName, C_UserID FROM CUSTOMER WHERE upper(C_UserID) like upper(p_CID) ; 
                end;        
            else
                if  p_CID is null then
                    Open MyRefCursor for Select C_FirstName, C_LastName, C_UserName, C_UserID FROM CUSTOMER WHERE UPPER(C_UserName) like UPPER(p_CUserName); 
                else
                    Open MyRefCursor for Select C_FirstName, C_LastName, C_UserName, C_UserID FROM CUSTOMER WHERE upper(C_UserID) like upper(p_CID) and UPPER(C_UserName) like UPPER(p_CUserName); 
                end;        
            end if;
        else
            if p_CUserName is null then
                if  p_CID is null then
                    ...
                else
                    ...
                end;        
            else
                if  p_CID is null then
                    ...
                else
                    ...
                end;        
            end if;
        end if;
    else
        if p_p_LastN is null then
            if p_CUserName is null then
                if  p_CID is null then
                    ...
                else
                    ...
                end;        
            else
                if  p_CID is null then
                    ...
                else
                    ...
                end;        
            end if;
        else
            if p_CUserName is null then
                if  p_CID is null then
                    ...
                else
                    ...
                end;        
            else
                if  p_CID is null then
                    ...
                else
                    ...
                end;        
            end if;
        end if;
    end if; 

end;
/

OK its late and I'm a bit lazy, but filling in the remaining 12 cases is straight forward.

Not using dynamic SQL has some advantages:

  1. You can verify your syntax at compile time
  2. You don't have to fiddle around with contexts

Don't think because it looks boring for a human, that it is bad for a computer (especially when running Oracle).

But don't add further parameters just to force me show a solution using dynamic sql, instead avoid insane designs requiring such solutions.

share|improve this answer
    
well this is good if you have only 3 parameters to go through. What about going through 10 different parameters? I know I can build this easy for something that ether has 1 or two parameters but it will not be easy to scale it to accept different parameters and to change the content. Once I finish 1 procedure i need to write about 6 more for multiple parameter input. –  Vladimir Oselsky Jan 18 '11 at 2:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.