Groovy Script Parameterised Query SQL Injection

Question

I have received a pentest report several years ago reporting one of the modules we have was vulnerable to SQL Injection. I am trying to verify this with limited knowledge. The code below shows the SQL statement takes in where and where variable may be subject to SQL injection as it can be manipulated. The recommendation is to use parameterised queries. The question I have is, does queryParser sufficiently sanitise it to prevent sql.eachRow from executing a potential malicious payload?

where = where + " AND " + queryParser(query)

https://github.com/OpenRock/OpenIDM/blob/master/openidm-zip/src/main/resources/samples/sample3/tools/SearchScript.groovy

switch (objectClass) {
    case ObjectClass.ACCOUNT:

        def where = "" ;
        def whereParams = []
        def fieldMap = [      
                "__ACCOUNT__" : [
                        "__UID__" : "USER_CONTEXT_KEY",
                        "__NAME__": "USER_CONTEXT_KEY"
                ] 
        ]

        if (filter != null) {

            def query = filter.accept(MapFilterVisitor.INSTANCE, null)
             log.info("Filter query:"+ query);
            // this closure function recurses through the (potentially complex) query object in order to build an equivalent
            // SQL 'where' expression
            def queryParser
            queryParser = { queryObj ->

                if (queryObj.operation == "OR" || queryObj.operation == "AND") {
                    return "(" + queryParser(queryObj.right) + " " + queryObj.operation + " " + queryParser(queryObj.left) + ")"
                } else {

                    if (fieldMap[objectClass.objectClassValue] && fieldMap[objectClass.objectClassValue][queryObj.get("left")]) {
                        queryObj.put("left", fieldMap[objectClass.objectClassValue][queryObj.get("left")])
                    }

                    def left = queryObj.get('left')
                    def not = queryObj.get('not')
                    def template
                    
                    switch (queryObj.get('operation')) {
                        case 'CONTAINS':
                            template = "$left ${not ? "NOT " : ""}LIKE ?"
                            whereParams.add("%" + queryObj.get("right") + "%")
                            break
                        case 'ENDSWITH':
                            template = "$left ${not ? "NOT " : ""}LIKE ?"
                            whereParams.add("%" + queryObj.get("right"))
                            break
                        case 'STARTSWITH':
                            template = "$left ${not ? "NOT " : ""}LIKE ?"
                            whereParams.add(queryObj.get("right") + "%")
                            break
                        case 'EQUALS':
                            template = "$left ${not ? "<>" : "="} ?"
                            whereParams.add(queryObj.get("right"))
                            break
                        case 'GREATERTHAN':
                            template = "$left ${not ? "<=" : ">"} ?"
                            whereParams.add(queryObj.get("right"))
                            break
                        case 'GREATERTHANOREQUAL':
                            template = "$left ${not ? "<" : ">="} ?"
                            whereParams.add(queryObj.get("right"))
                            break
                        case 'LESSTHAN':
                            template = "$left ${not ? ">=" : "<"} ?"
                            whereParams.add(queryObj.get("right"))
                            break
                        case 'LESSTHANOREQUAL':
                            template = "$left ${not ? ">" : "<="} ?"
                            whereParams.add(queryObj.get("right"))
                    }
                    
                    return template.toString()
                }
            }

            where = where + " AND " + queryParser(query)
            log.info("Search WHERE clause is: " + where)
        }

       def statement = """
        select 
             a.USERUID||'^'||b.OCRCNMBR as USER_CONTEXT_KEY 
            ,b.USERUID,b.OCRCNMBR,b.AUTHRCNTXTTYPE,b.AUTHRCNTXTSTATUS,b.CNTXTSTATUSREAS,b.REVIEWDATE,b.CNTXTSTRTDATE
            ,b.CNTXTEXPDATE,b.DOFPROFILEID,b.DESC, b.DATAAUTHRREF,b.APPROVERNAME,b.AC_TIMESTMP,b.APPROVER_EMAIL_ADDRESS,b.APPROVER_PHONE_NO
            ,c.AGENTLEVEL
            ,CASE 
                WHEN a.USERTYPE = 'AD' then '1' 
                WHEN a.USERTYPE in ('AA' , 'AB') and c.AGENCYACCNO is NULL  then '1'
                ELSE c.AGENCYACCNO
            END as AGENCYACCNO,
            c.RINO,c.COMPANYSIBNO,c.AAC_TIMESTMP,c.GI_AGENCY_NO,c.GI_AGENT_LEVEL,c.COMPANY_NAME,c.IRN_NO           
        from
            ESEC.TIAUSER a,
            ESEC.TIAAUTHRCNTXT b
            LEFT JOIN ESEC.TIAAGNTAUTHR c ON b.useruid = c.useruid AND b.OCRCNMBR = c.OCRCNMBR
        where a.USERTYPE in ('AA', 'AB' , 'AD')
            and a.useruid = b.useruid
            ${where}
            order by a.USERUID, b.OCRCNMBR WITH UR
        """
        log.info("Statement is: {0}", statement);
         log.info("Statement params : {0}", whereParams);

Answer 1

My groovy is weak - but there are a at least a couple of potential issues with this:

• As daggett mentions, your $left is unvalidated - this means an attacker could put arbitrary SQL into this part
• Even if you fix that, an attacker can do anything the parser will allow semantically - because the parser can't tell what the source is. This certain includes access to all data in this table, but potentially includes callouts to subqueries & DB functions that can query other tables

If you do resolve the above issues, you should also ensure the "untrusted" where fragment is in in it's own parenthesis (to avoid bypassing parts of the existing query with "OR" clauses).

The only way to be safe in this is to validate all elements of the SQL via positive / whitelist validation. Each parsed line should be

AND/OR <column name> <operation> :<param> 

all of which are positively validated against a list.

And you need to validate that everything that can pass this validation is semantically valid for the user.