[open-ils-commits] r16771 - in trunk/Open-ILS: include/openils src/c-apps (scottmk)
svn at svn.open-ils.org
svn at svn.open-ils.org
Tue Jun 22 08:34:44 EDT 2010
Author: scottmk
Date: 2010-06-22 08:34:43 -0400 (Tue, 22 Jun 2010)
New Revision: 16771
Modified:
trunk/Open-ILS/include/openils/oils_sql.h
trunk/Open-ILS/src/c-apps/oils_sql.c
trunk/Open-ILS/src/c-apps/oils_storedq.c
Log:
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
Modified: trunk/Open-ILS/include/openils/oils_sql.h
===================================================================
--- trunk/Open-ILS/include/openils/oils_sql.h 2010-06-21 20:41:59 UTC (rev 16770)
+++ trunk/Open-ILS/include/openils/oils_sql.h 2010-06-22 12:34:43 UTC (rev 16771)
@@ -35,13 +35,13 @@
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 doSearch( osrfMethodContext* ctx );
int doIdList( osrfMethodContext* ctx );
+int is_identifier( const char* s);
+int is_good_operator( const char* op );
+
#ifdef __cplusplus
}
#endif
Modified: trunk/Open-ILS/src/c-apps/oils_sql.c
===================================================================
--- trunk/Open-ILS/src/c-apps/oils_sql.c 2010-06-21 20:41:59 UTC (rev 16770)
+++ trunk/Open-ILS/src/c-apps/oils_sql.c 2010-06-22 12:34:43 UTC (rev 16771)
@@ -102,8 +102,6 @@
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 @@
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 @@
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;
Modified: trunk/Open-ILS/src/c-apps/oils_storedq.c
===================================================================
--- trunk/Open-ILS/src/c-apps/oils_storedq.c 2010-06-21 20:41:59 UTC (rev 16770)
+++ trunk/Open-ILS/src/c-apps/oils_storedq.c 2010-06-22 12:34:43 UTC (rev 16771)
@@ -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 @@
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 @@
}
}
+ } 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 @@
"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 @@
"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,
More information about the open-ils-commits
mailing list