From: scottmk <scottmk@dcc99617-32d9-48b4-a31d-7c20da2025e4>
Date: Tue, 22 Jun 2010 12:34:43 +0000 (+0000)
Subject: 1. In oils_sql.c: make the functions is_identifier() and is_good_operator()
X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=5f62a571b55c0651e947de839e92c7c817ce9465;p=evergreen%2Ftadl.git
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 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,