You already know that I like to analyze other people’s code. On OTN I found a nice article (most popluar developer article) „Best Practice PL/SQL from Steven Feuerstein“ (http://www.oracle.com/technology/pub/columns/plsql/index.html).
Steven Feuerstein is a well-known expert on the Oracle PL/SQL language. His disclaimer says that „Do not take the advice and recommendations herein at face value. You should always build yourself a test case and run it on your database, for your schema, on your computer.“ That’s OK but even (or especially) sample code should be secure. Disclaimers are a simple but not a good solution.
Especially if the code is posted as „Best Practice“.
The best practice contains some PL/SQL sample code for download, e.g. str2list.
„The str2list package accepts your string, delimiter, and the name of your package-based collection. It deposits the parsed items in your string directly into your collection. The collection can either be declared in the package specification (publicly accessible) or you can define it in the package body and then provide procedures to add to and delete from the collection. These will be called by str2list to populate the collection properly. It’s a useful utility as well as a great example of dynamic PL/SQL block execution.“
As always the same problem: no input-validation in some of the procedures (e.g. showlist or parse). This could allow an attacker to run custom PL/SQL code. PL/SQL injection is more severe than SQL Injection. I know that writing secure code takes time but I think it’s worth to do this, especially for sample code which is often used by many people. Just adding a disclaimer is in my opinion not the right way to deal with vulnerabilities.
A quick analysis of the code str2list.pkg (Source is from March 2005) shows the following vulnerable code:
—————— str2list.pkg ————————————————————
PROCEDURE showlist (
pkg IN VARCHAR2,
firstrowproc IN VARCHAR2,
nextrowproc IN VARCHAR2,
getvalfunc IN VARCHAR2,
showproc IN VARCHAR2 := ‚pl‘,
datatype IN VARCHAR2 := ‚VARCHAR2(32767)‘
)
IS
dynblock VARCHAR2 (32767);
BEGIN
dynblock :=
‚DECLARE
indx PLS_INTEGER := ‚
|| pkg
|| ‚.‘
|| firstrowproc
|| ‚;
v_startloc PLS_INTEGER := 1;
v_item ‚
|| datatype
|| ‚;
BEGIN
LOOP
EXIT WHEN indx IS NULL;‘
|| showproc
|| ‚ (‚
|| pkg
|| ‚.‘
|| getvalfunc
|| ‚(indx));
indx := ‚
|| pkg
|| ‚.‘
|| nextrowproc
|| ‚(indx);
END LOOP;
END;‘;
EXECUTE IMMEDIATE dynblock;
EXCEPTION
WHEN OTHERS
THEN
disperr (dynblock);
END;—————— str2list.pkg ————————————————————