From 5f62a571b55c0651e947de839e92c7c817ce9465 Mon Sep 17 00:00:00 2001 From: scottmk Date: Tue, 22 Jun 2010 12:34:43 +0000 Subject: [PATCH 1/1] 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 --- Open-ILS/include/openils/oils_sql.h | 15 ++++---- Open-ILS/src/c-apps/oils_sql.c | 6 ++-- Open-ILS/src/c-apps/oils_storedq.c | 55 +++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/Open-ILS/include/openils/oils_sql.h b/Open-ILS/include/openils/oils_sql.h index 40c4cc0a00..9781a45dbf 100644 --- 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 c4c90b675a..fea9acba8b 100644 --- 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 c4f512ad62..f5b61f9d5d 100644 --- 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, -- 2.43.2