1. In oils_sql.c: make the functions is_identifier() and is_good_operator()

global instead of static.

2. Use them to protect qstore against various forms of sql injection.

M    Open-ILS/include/openils/oils_sql.h
M    Open-ILS/src/c-apps/oils_storedq.c
M    Open-ILS/src/c-apps/oils_sql.c

git-svn-id: svn://svn.open-ils.org/ILS/trunk@16771 dcc99617-32d9-48b4-a31d-7c20da2025e4

diff --git a/Open-ILS/include/openils/oils_sql.h b/Open-ILS/include/openils/oils_sql.h
index 40c4cc0..9781a45 100644 (file)
--- a/Open-ILS/include/openils/oils_sql.h
+++ b/Open-ILS/include/openils/oils_sql.h
@@ -35,13 +35,13 @@ char* buildQuery( osrfMethodContext* ctx, jsonObject* query, int flags );
 
 char* oilsGetRelation( osrfHash* classdef );
 
-int beginTransaction ( osrfMethodContext* );
-int commitTransaction ( osrfMethodContext* );
-int rollbackTransaction ( osrfMethodContext* );
+int beginTransaction ( osrfMethodContext* ctx );
+int commitTransaction ( osrfMethodContext* ctx );
+int rollbackTransaction ( osrfMethodContext* ctx );
 
-int setSavepoint ( osrfMethodContext* );
-int releaseSavepoint ( osrfMethodContext* );
-int rollbackSavepoint ( osrfMethodContext* );
+int setSavepoint ( osrfMethodContext* ctx );
+int releaseSavepoint ( osrfMethodContext* ctx );
+int rollbackSavepoint ( osrfMethodContext* ctx );
 
 int doJSONSearch ( osrfMethodContext* ctx );
 
@@ -52,6 +52,9 @@ int doDelete( osrfMethodContext* ctx );
 int doSearch( osrfMethodContext* ctx );
 int doIdList( osrfMethodContext* ctx );
 
+int is_identifier( const char* s);
+int is_good_operator( const char* op );
+
 #ifdef __cplusplus
 }
 #endif

diff --git a/Open-ILS/src/c-apps/oils_sql.c b/Open-ILS/src/c-apps/oils_sql.c
index c4c90b6..fea9acb 100644 (file)
--- a/Open-ILS/src/c-apps/oils_sql.c
+++ b/Open-ILS/src/c-apps/oils_sql.c
@@ -102,8 +102,6 @@ static int obj_is_true( const jsonObject* obj );
 static const char* json_type( int code );
 static const char* get_primitive( osrfHash* field );
 static const char* get_datatype( osrfHash* field );
-static int is_identifier( const char* s );
-static int is_good_operator( const char* op );
 static void pop_query_frame( void );
 static void push_query_frame( void );
 static int add_query_core( const char* alias, const char* class_name );
@@ -6121,7 +6119,7 @@ static const char* get_datatype( osrfHash* field ) {
        More pedantically we should allow quoted identifiers containing arbitrary characters, but
        for the foreseeable future such quoted identifiers are not likely to be an issue.
 */
-static int is_identifier( const char* s) {
+int is_identifier( const char* s) {
        if( !s )
                return 0;
 
@@ -6178,7 +6176,7 @@ static int is_identifier( const char* s) {
        We don't do that because we want to allow custom operators like ">100*", which at this
        writing would be difficult or impossible to express otherwise in a JSON query.
 */
-static int is_good_operator( const char* op ) {
+int is_good_operator( const char* op ) {
        if( !op ) return 0;   // Sanity check
 
        const char* s = op;

diff --git a/Open-ILS/src/c-apps/oils_storedq.c b/Open-ILS/src/c-apps/oils_storedq.c
index c4f512a..f5b61f9 100644 (file)
--- a/Open-ILS/src/c-apps/oils_storedq.c
+++ b/Open-ILS/src/c-apps/oils_storedq.c
@@ -11,6 +11,8 @@
 #include "opensrf/log.h"
 #include "opensrf/string_array.h"
 #include "opensrf/osrf_hash.h"
+#include "opensrf/osrf_application.h"
+#include "openils/oils_sql.h"
 #include "openils/oils_buildq.h"
 
 #define PRINT if( verbose ) printf
@@ -685,6 +687,13 @@ static FromRelation* constructFromRelation( BuildSQLState* state, dbi_result res
 
        switch ( type ) {
                case FRT_RELATION :
+                       if( table_name && ! is_identifier( table_name )) {
+                               osrfLogWarning( OSRF_LOG_MARK, sqlAddMsg( state,
+                                       "Table name \"%s\" is not a valid identifier for FROM relation # %d",
+                                       table_name, id ));
+                               state->error = 1;
+                               return NULL;
+                       }
                        break;
                case FRT_SUBQUERY :
                        if( -1 == subquery_id ) {
@@ -1738,6 +1747,20 @@ static Expression* constructExpression( BuildSQLState* state, dbi_result result
                        }
                }
 
+       } else if( EXP_COLUMN == type ) {
+               if( !column_name ) {
+                       osrfLogWarning( OSRF_LOG_MARK, sqlAddMsg( state,
+                               "No column name for column expression # %d", id ));
+                       state->error = 1;
+                       return NULL;
+               } else if( !is_identifier( column_name )) {
+                       osrfLogWarning( OSRF_LOG_MARK, sqlAddMsg( state,
+                               "Column name \"%s\" is invalid identifier for expression # %d",
+                               column_name, id ));
+                       state->error = 1;
+                       return NULL;
+               }
+
        } else if( EXP_EXIST == type ) {
                if( -1 == subquery_id ) {
                        osrfLogWarning( OSRF_LOG_MARK, sqlAddMsg( state,
@@ -1760,6 +1783,12 @@ static Expression* constructExpression( BuildSQLState* state, dbi_result result
                                "Function call expression # %d provides no function name", id ));
                        state->error = 1;
                        return NULL;
+               } else if( !is_identifier( function_name )) {
+                       osrfLogWarning( OSRF_LOG_MARK, sqlAddMsg( state,
+                               "Function call expression # %d contains an invalid function name \"%s\"",
+                               id, function_name ));
+                       state->error = 1;
+                       return NULL;
                } else {
                        subexp_list = getExpressionList( state, id );
                        if( state->error ) {
@@ -1833,9 +1862,35 @@ static Expression* constructExpression( BuildSQLState* state, dbi_result result
                                "Numeric expression # %d provides no numeric value", id ));
                        state->error = 1;
                        return NULL;
+               } else if( !jsonIsNumeric( literal )) {
+                       // Purported number is not numeric.  We use JSON rules here to determine what
+                       // a valid number is, just because we already have a function lying around that
+                       // can do that validation.  PostgreSQL probably doesn't use the same exact rules.
+                       // If that's a problem, we can write a separate validation routine to follow
+                       // PostgreSQL's rules, once we figure out what those rules are.
+                       osrfLogWarning( OSRF_LOG_MARK, sqlAddMsg( state,
+                               "Numeric expression # %d is not a valid number: \"%s\"", id, literal ));
+                       state->error = 1;
+                       return NULL;
                }
 
        } else if( EXP_OPERATOR == type ) {
+               // Make sure we have a plausible operator
+               if( !operator ) {
+                       osrfLogWarning( OSRF_LOG_MARK, sqlAddMsg( state,
+                               "Operator expression # %d has no operator", id ));
+                       state->error = 1;
+                       return NULL;
+               } else if( !is_good_operator( operator )) {
+                       // The specified operator contains one or more characters that aren't allowed
+                       // in an operator.  This isn't a true validation; it's just a protective
+                       // measure to prevent certain kinds of sql injection.
+                       osrfLogWarning( OSRF_LOG_MARK, sqlAddMsg( state,
+                               "Operator expression # %d contains invalid operator \"%s\"", id, operator ));
+                       state->error = 1;
+                       return NULL;
+               }
+
                // Load left and/or right operands
                if( -1 == left_operand_id && -1 == right_operand_id ) {
                        osrfLogWarning( OSRF_LOG_MARK, sqlAddMsg( state,