0

can someone explain to me about my code in oracle apex, It's seem like vulnerable to sql injection. seem like DBMS_SQL.EXECUTE(VR_CURS) is vulnerable. my question is how to exploit this query, and how to patch this bug ? how about if I use dbms.assert ? is that more secure ? here my query:

  FUNCTION SQL_TO_SYS_REFCURSOR (
   P_IN_SQL_STATEMENT   CLOB,
   P_IN_BINDS           SYS.DBMS_SQL.VARCHAR2_TABLE
    ) RETURN SYS_REFCURSOR AS
  VR_CURS         BINARY_INTEGER;    VR_REF_CURSOR   SYS_REFCURSOR;
  VR_EXEC         BINARY_INTEGER;
  * TODO make size dynamic */
  VR_BINDS        VARCHAR(100);
  BEGIN
  VR_CURS         := DBMS_SQL.OPEN_CURSOR;
  DBMS_SQL.PARSE(
      VR_CURS,
      P_IN_SQL_STATEMENT,
      DBMS_SQL.NATIVE
  );
  IF P_IN_BINDS.COUNT > 0 THEN
      FOR I IN 1..P_IN_BINDS.COUNT LOOP
      /* TODO find out how to prevent ltrim */
          VR_BINDS   := LTRIM(
              P_IN_BINDS(I),
              ':'
        );
          DBMS_SQL.BIND_VARIABLE(
              VR_CURS,
              VR_BINDS,
              V(VR_BINDS)
          );
      END LOOP;
  END IF;

  VR_EXEC         := DBMS_SQL.EXECUTE(VR_CURS);
VR_REF_CURSOR   := DBMS_SQL.TO_REFCURSOR(VR_CURS);
RETURN VR_REF_CURSOR;
EXCEPTION
  WHEN OTHERS THEN
      IF DBMS_SQL.IS_OPEN(VR_CURS) THEN
          DBMS_SQL.CLOSE_CURSOR(VR_CURS);
      END IF;
      RAISE;
END;
potitit
  • 63
  • 1
  • 8
  • Why are you writing SQL like this? Why do you think there is a vulnerability? Usually if there is, it's not a bug that needs patching, it's poorly written code that needs correcting. – Scott May 22 '20 at 02:58
  • thanks scott, I got notice vulnerable sql injection using apexsec tools. Im sorry about poorly written code, can u explain to me which code need to correcting ? – potitit May 22 '20 at 06:20
  • 1
    You're taking in a complete statement as an argument. That means you are opening the door to statements as "DROP TABLE xxx", "CREATE PROCEDURE " etc. The statement that you are passing here as an argument should be the code in your database instead. Is this code needed ? What is the added value of it in your application ? Your question is similar to "I removed my front door so I can easily enter. How can I prevent thieves from getting in ?". Answer: "put the door back in". In short, try to get rid of this function unless you absolutely need it. – Koen Lostrie May 22 '20 at 07:56
  • I knew I've seen this code before. https://github.com/RonnyWeiss/APEX-CLOB-Load-2/blob/master/dynamic_action_plugin_apex_clob_load_2.sql – Jeffrey Kemp May 26 '20 at 11:46
  • Hi potitit, did you still need help with this or was Kris’ answer sufficient? If it was sufficient, please accept the answer for future viewers. ApexSec flags potential issues. You have to know where this code is being used and if its use is limited to developers (trusted) or exposed to end-users (untrusted). If the code is from the plug-in Jeffrey mentioned, you're probably fine, provided the SQL it's executing is from a developer and not something more dynamic that takes in values from an end-user (other than bind variable values, which are not vulnerable to SQL injection). – Dan McGhan May 26 '20 at 15:44

1 Answers1

0

DBMS_SQL, EXECUTE IMMEDIATE , ... Are not vulnerable themselves as they are utilities, when used wrong their usage is high vulnerable. For Example on safe use of execute immediate see this answer : Oracle - Why is EXECUTE IMMEDIATE allowed in stored procedures?

There's only 2 ways this code could be "safe".

a) The caller is highly trusted. This means any and all usage of this function is is known and the mechanism in that caller for building up the sql statement is trustable. Maybe even to the point of adding a call stack check to reject unknown callers which could be gotten from owa_util.who_called_me

b ) a full parse of P_IN_SQL_STATEMENT prior to use with DBMS_SQL.PARSE. This would means writing a full parser of sql and pl/sql to analyze the inputs for potential injection.

IF and only if one of those 2 things are met then it could be wrapped with DBMS_ASSERT.NOOP(P_IN_SQL_STATEMENT) to indicate that the value is trusted.

Kris Rice
  • 3,300
  • 15
  • 33