FWD » Core Development » Database

I posted the following in #8667-18:

My concern here about SQL injection is that these text values can be used to create the core text of a dynamic query before QUERY-PREPARE() is called.

This is a general problem that we already have, except this proposed feature extends the exposure out to the Internet instead of user input from the application code.

Consider how our dynamic query support works. The 4GL code builds up some big wad of query text and passes it to QUERY-PREPARE(). Our code ends up in DynamicQueryHelper.parse() which eventually calls our progress.g parser in the dynamic_query_proc entry point.

Here is a snippet of that parser code:

/** 
 * Top-level program definition (and entry point) of a code block handling a dynamic find/query. It is
 * created so that the resulted tree is the same as for a full {@code external_proc()} call but it does
 * supported only a limited set of statements.
 * Only the following cases/statements are supported:
 *  1. Dynamic find query:
 *        FIND FIRST <buffer> <find-predicate> <lock-type>.
 *  
 *  2. Dynamic query prepare:
 *        [ DEFINE BUFFER <buff-name> FOR [TEMP-TABLE] <table-name>. ]*
 *        OPEN QUERY DynGenQuery <query-predicate>.
 * In the future, if other statements will be supported, they will have to be added to 
 * {@code dynamic_query_stmt}.
 */
dynamic_query_proc:
   { topLevelEntry = true; }
   db:dynamic_query_block { ## = #([BLOCK, "dynamic-block"], db); }
;

/**
 * Processes a set of statements inside a dynamic program. Called only from {@code dynamic_query_proc}.
 */
dynamic_query_block:
   (dynamic_query_stmt)*
;

/**
 * Identifies one of the three known statements to the {@code dynamic_query_proc} and dispatches the call
 * to the appropriate rule. Puts the resulting sub-tree in a {@code STATEMENT} AST node and sets the
 * text accordingly.
 */
dynamic_query_stmt:
     f:find_stmt { ## = #([STATEMENT, "dynamic-find-stmt"], f); } 
   | ddb:dynamic_define_buffer_stmt  { ## = #([STATEMENT, "dynamic-create-buffer"], ddb); } 
   | oq:open_query_stmt  { ## = #([STATEMENT, "dynamic-open-query-stmt"], oq); }
;

Notice that it will match any number of statements. On a positive note, it will only match FIND, OPEN-QUERY and DEFINE BUFFER. So, just as in OE, there is runtime parsing of 4GL code and any unverified input mixed into that code is dangerous. To be clear: in FWD, there is danger here from 4GL keywords and punctuation/syntax. It is also possible that there is some way to drop unverified SQL into this mess and get it all the way through. I need to think more on that to see if I can come up with a vector. But the point here is that there is a real problem and our existing usage of prepared statements does not resolve it.

I would prefer some solution that scans to detect something dangerous. But to be fair, any such solution would likely only address the SQL part (SQL keywords and terminators like ;). I seriously doubt there is any 4GL injection scanning. :( If there was an organization that could write such a thing, it is GCD. But we don't have the time to do that. Even if we had the scanning tech, I am not proposing any kind of escaping here. I would like to just scan and if it fails the scan raise an error or throw a STOP condition or whatever.

This task is to do an evaluation of this potential and to actually try to succeed with a SQL injection "attack". If we can find ways to cause this to occur, then we will want to evaluate ways in which we can harden FWD against this.

We should also test these same approaches in OE to know it compares.