44ccfacbe3a3d4b300f80d48651c77837a4b571e
galt
Tue Apr 26 11:12:02 2022 -0700
SQL INJECTION Prevention Version 2 - this improves our methods by making subclauses of SQL that get passed around be both easy and correct to use. The way that was achieved was by getting rid of the obscure and not well used functions sqlSafefFrag and sqlDyStringPrintfFrag and replacing them with the plain versions of those functions, since these are not needed anymore. The new version checks for NOSQLINJ in unquoted %-s which is used to include SQL clauses, and will give an error the NOSQLINJ clause is not present, and this will automatically require the correct behavior by developers. sqlDyStringPrint is a very useful function, however because it was not enforced, users could use various other dyString functions and they operated without any awareness or checking for SQL correct use. Now those dyString functions are prohibited and it will produce an error if you try to use a dyString function on a SQL string, which is simply detected by the presence of the NOSQLINJ prefix.
diff --git src/hg/lib/jksql.c src/hg/lib/jksql.c
index 7aa7153..9e848c1 100644
--- src/hg/lib/jksql.c
+++ src/hg/lib/jksql.c
@@ -807,31 +807,33 @@
}
sqlFreeResult(&sr);
return databases;
}
static bool sqlTableExistsOnMain(struct sqlConnection *sc, char *tableName)
/* Return TRUE if the table can be queried using sc's main conn;
* don't check failoverConn or the table cache (showTableCache in hg.conf). */
{
// if the whole db does not exist on the main server, then the table is certainly not there
if (sqlConnMustUseFailover(sc))
return FALSE;
char query[1024];
-sqlSafef(query, sizeof(query), "SELECT 1 FROM %-s LIMIT 0", sqlCkIl(tableName));
+sqlCkIl(tableNameSafe,tableName)
+//char tableNameSafe[strlen(tableName)+9+1]; sqlCheckIdentifiersList(tableNameSafe, sizeof tableNameSafe, tableName);
+sqlSafef(query, sizeof(query), "SELECT 1 FROM %-s LIMIT 0", tableNameSafe);
struct sqlResult *sr;
// temporarily remove failover connection, we don't want the failover switch here
struct sqlConnection *failoverConn = sc->failoverConn;
sc->failoverConn=NULL;
sr = sqlUseOrStore(sc, query, DEFAULTGETTER, FALSE);
sc->failoverConn=failoverConn;
bool ret = FALSE;
if (sr!=NULL)
{
monitorPrint(sc, "SQL_TABLE_EXISTS", "%s", tableName);
ret = TRUE;
sqlFreeResult(&sr);
}
@@ -914,33 +916,33 @@
{
el = slNameNew(row[0]);
slAddHead(&list, el);
}
slReverse(&list);
sqlFreeResult(&sr);
return list;
}
static struct slName *sqlListTablesForConn(struct sqlConnection *conn, char *likeExpr)
/* run SHOW TABLES on connection and return a slName list. LIKE expression
* can be NULL or string e.g. "LIKE 'snp%'" */
{
char query[256];
if (likeExpr == NULL)
- safef(query, sizeof(query), NOSQLINJ "SHOW TABLES");
+ sqlSafef(query, sizeof(query), "SHOW TABLES");
else
- safef(query, sizeof(query), NOSQLINJ "SHOW TABLES LIKE '%s'", likeExpr);
+ sqlSafef(query, sizeof(query), "SHOW TABLES LIKE '%s'", likeExpr);
struct slName *list = NULL, *el;
struct sqlResult *sr;
char **row;
sr = sqlGetResult(conn, query);
while ((row = sqlNextRow(sr)) != NULL)
{
el = slNameNew(row[0]);
slAddHead(&list, el);
}
slReverse(&list);
sqlFreeResult(&sr);
return list;
}
@@ -1442,31 +1444,31 @@
* and the user must call next_row to see if there's anything in the resultset.
*/
{
struct sqlResult *res = NULL;
struct sqlConnection *scMain = sc;
long deltaTime;
boolean fixedMultipleNOSQLINJ = FALSE;
++sqlTotalQueries;
if (monitorFlags & JKSQL_TRACE)
monitorPrintQuery(sc, query);
if (startsWith(NOSQLINJ "", query))
{
- query += strlen(NOSQLINJ ""); // We know this query has been vetted for sql injection, skip over this tag.
+ query += NOSQLINJ_SIZE; // We know this query has been vetted for sql injection, skip over this tag.
}
else
{
sqlCheckError("Unvetted query: %s", query);
}
// additional check finds errors of multiple NOSQLINJ tags
if (strstr(query, NOSQLINJ ""))
{
sqlCheckError("Oops, multiple occurrences of NOSQLINJ tag in query: %s", query);
query = replaceChars(query, NOSQLINJ "", "");
fixedMultipleNOSQLINJ = TRUE;
}
if (sqlConnMustUseFailover(sc))
@@ -1648,31 +1650,31 @@
void sqlHardLockTables(struct sqlConnection *sc, struct slName *tableList,
boolean isWrite)
/* Hard lock given table list. Unlock with sqlHardUnlockAll. */
{
struct dyString *dy = dyStringNew(0);
struct slName *table;
char *how = (isWrite ? "WRITE" : "READ");
if (sc->hasHardLock)
errAbort("sqlHardLockTables repeated without sqlHardUnlockAll.");
sqlDyStringPrintf(dy, "LOCK TABLES ");
for (table = tableList; table != NULL; table = table->next)
{
sqlDyStringPrintf(dy, "%s %s", table->name, how);
if (table->next != NULL)
- dyStringAppendC(dy, ',');
+ sqlDyStringPrintf(dy, ",");
}
sqlUpdate(sc, dy->string);
sc->hasHardLock = TRUE;
dyStringFree(&dy);
}
void sqlHardLockTable(struct sqlConnection *sc, char *table, boolean isWrite)
/* Lock a single table. Unlock with sqlHardUnlockAll. */
{
struct slName *list = slNameNew(table);
sqlHardLockTables(sc, list, isWrite);
slFreeList(&list);
}
@@ -1769,31 +1771,33 @@
return FALSE; // mysql does not allow tables with dash (-) so it will not be found.
// hg/lib/hdb.c can generate an invalid table names with dashes while looking for split tables,
// if the first chrom name has a dash in it. Examples found were: scaffold_0.1-193456 scaffold_0.1-13376 HERVE_a-int 1-1
// Assembly hubs also may have dashes in chrom names.
}
// use the table cache if we have one
struct sqlConnection *cacheConn = sqlTableCacheFindConn(sc);
if (cacheConn)
return sqlTableCacheTableExists(cacheConn, table);
char *err;
unsigned int errNo;
const int tableNotFoundCode = 1146;
-sqlSafef(query, sizeof(query), "SELECT 1 FROM %-s LIMIT 0", sqlCkIl(table));
+sqlCkIl(tableSafe,table)
+//char tableSafe[strlen(table)+9+1]; sqlCheckIdentifiersList(tableSafe, sizeof tableSafe, table);
+sqlSafef(query, sizeof(query), "SELECT 1 FROM %-s LIMIT 0", tableSafe);
if ((sr = sqlGetResultExt(sc, query, &errNo, &err)) == NULL)
{
if (errNo == tableNotFoundCode)
return FALSE;
if (sc->failoverConn)
{
// if not found but we have a failover connection, check on it, too
if ((sr = sqlGetResultExt(sc->failoverConn, query, &errNo, &err)) == NULL)
{
if (errNo == tableNotFoundCode)
return FALSE;
}
}
}
@@ -2189,31 +2193,30 @@
char *table, char *field, char *key)
/* Return TRUE if row where field = key is in table. */
{
char query[256];
sqlSafef(query, sizeof(query), "select count(*) from %s where %s = '%s'",
table, field, key);
return sqlQuickNum(conn, query) > 0;
}
int sqlRowCount(struct sqlConnection *conn, char *queryTblAndCondition)
/* Return count of rows that match condition. The queryTblAndCondition
* should contain everying after "select count(*) FROM " */
{
char query[256];
sqlSafef(query, sizeof(query), "select count(*) from %-s",queryTblAndCondition);
-// NOSQLINJ since we cannot check the queryTblAndCondition here, the users of this function have been fixed.
return sqlQuickNum(conn, query);
}
struct sqlResult *sqlStoreResult(struct sqlConnection *sc, char *query)
/* Returns NULL if result was empty. Otherwise returns a structure
* that you can do sqlRow() on. Same interface as sqlGetResult,
* but internally this keeps the entire result in memory. */
{
return sqlUseOrStore(sc,query,mysql_store_result, TRUE);
}
char **sqlNextRow(struct sqlResult *sr)
/* Get next row from query result. */
{
@@ -3791,31 +3794,31 @@
static void sqlCheckAllowAlphaChars(char allowed[256])
/* Allow all chars by setting to 0 */
{
sqlCheckAllowUpperChars(allowed);
sqlCheckAllowLowerChars(allowed);
}
static void sqlCheckAllowAlphaNumChars(char allowed[256])
/* Allow all chars by setting to 0 */
{
sqlCheckAllowAlphaChars(allowed);
sqlCheckAllowDigitChars(allowed);
}
/* Currently used 10 times in the code via define sqlCkIl. */
-char *sqlCheckIdentifiersList(char *identifiers)
+char *sqlCheckIdentifiersListExt(char *identifiers)
/* Check that only valid identifier characters are used in a comma-separated list
* '.' is allowed also since some code uses it in place of an actual field name.
* See hgTables/bedList.c::bedSqlFieldsExceptForChrom(). */
{
static boolean init = FALSE;
static char allowed[256];
if (!init)
{
sqlCheckDisallowAllChars(allowed);
sqlCheckAllowAlphaNumChars(allowed);
sqlCheckAllowChar('.', allowed);
sqlCheckAllowChar('_', allowed);
// sqlTableExists looks like a single table check, but apparently it has become abused
// to support multiple tables e.g. sqlTableExists
sqlCheckAllowChar(' ', allowed);
@@ -3898,30 +3901,39 @@
textDone = TRUE;
}
}
++i;
}
if (needText || spaceOk)
{
sqlCheckError("Invalid Identifiers List [%s] unexpected trailing comma or space character", identifiers);
return identifiers;
}
return identifiers;
}
+void sqlCheckIdentifiersList(char* buffer, int bufSize, char *identifiers)
+/* Check that only valid identifier characters are used in a comma-separated list
+ * '.' is allowed also since some code uses it in place of an actual field name.
+ * See hgTables/bedList.c::bedSqlFieldsExceptForChrom().
+ * Save safe output to char array */
+{
+sqlCheckIdentifiersListExt(identifiers);
+safef(buffer, bufSize, "NOSQLINJ %s", identifiers);
+}
char *sqlCheckIdentifier(char *identifier)
/* Check that only valid identifier characters are used */
{
static boolean init = FALSE;
static char allowed[256];
if (!init)
{
sqlCheckDisallowAllChars(allowed);
sqlCheckAllowAlphaNumChars(allowed);
sqlCheckAllowChar('.', allowed);
sqlCheckAllowChar('_', allowed);
// NOTE it is important for security that no other characters be allowed here
init = TRUE;
}
@@ -3992,55 +4004,63 @@
s = start + 1;
*end = 0; // mark end of "input" string, replacing escPunc. input string is temporary anyway.
int inputSize = end - s;
int worstCase = inputSize*2 + 1;
if (worstCase > remainder)
errAbort("Buffer too small for escaping in sqlEscapeAllStrings. s=[%s] bufSize = %d", sOrig, bufSize);
int escSize = mysql_escape_string(buffer, s, inputSize);
buffer += escSize;
sz += escSize;
remainder -= escSize;
s = end + 1;
}
return sz;
}
+struct restoreSafeStr
+ {
+ struct restoreSafeStr *next;
+ char *s;
+ int strLen;
+ };
int vaSqlSafefNoAbort(char* buffer, int bufSize, boolean newString, char *format, va_list args)
/* VarArgs Format string to buffer, vsprintf style, only with buffer overflow
* checking. The resulting string is always terminated with zero byte.
* Scans string parameters for illegal sql chars.
* Automatically escapes quoted string values.
* This function should be efficient on statements with many strings to be escaped. */
{
va_list orig_args;
va_copy(orig_args, args);
int formatLen = strlen(format);
char escPunc = 0x01; // using char 1 as special char to denote strings needing escaping
char *newFormat = NULL;
int newFormatSize = 2*formatLen + 1;
if (newString)
- newFormatSize += strlen(NOSQLINJ "");
+ newFormatSize += NOSQLINJ_SIZE;
newFormat = needMem(newFormatSize);
char *nf = newFormat;
if (newString)
nf += safef(newFormat, newFormatSize, "%s", NOSQLINJ "");
char *lastPct = NULL;
int escStringsCount = 0;
int escStringsSize = 0;
+struct restoreSafeStr *restoreSafeStrList=NULL, *restoreSafeStr=NULL;
+
char c = 0;
int i = 0;
char quote = 0;
boolean inPct = FALSE;
boolean isLong = FALSE;
boolean isLongLong = FALSE;
boolean isNegated = FALSE;
while (i < formatLen)
{
c = format[i];
*nf++ = c;
// start quote
if (quote==0 && (c == '\'' || c == '"' || c == '`'))
quote = c;
// end quote
@@ -4088,56 +4108,68 @@
else if (c == 'g') { va_arg(args, double); }
// pointer is void *
else if (c == 'p') { va_arg(args, void *); }
// char get promoted to int by varargs process
else if (c == 'c') { va_arg(args, int); }
// finally, the string we care about!
else if (c == 's')
{
char *s = va_arg(args, char *);
if (s == NULL)
sqlCheckError("%%s value is NULL which is incorrect.");
if (quote == 0)
{ // check identifier
if (!isNegated) // Not a Pre-escaped String
sqlCheckIdentifier(s);
+ else
+ {
+ if (!startsWith(NOSQLINJ, s))
+ {
+ sqlCheckError("Internal Error: Input to %%-s should be created with safe functions.");
+ }
+ // wipe out the prefix by removing from the input string s
+ int strLen = strlen(s);
+ memmove(s, s+NOSQLINJ_SIZE, strLen - NOSQLINJ_SIZE + 1);
+ AllocVar(restoreSafeStr);
+ restoreSafeStr->s = s;
+ restoreSafeStr->strLen = strLen;
+ slAddHead(&restoreSafeStrList, restoreSafeStr);
+ }
}
else
{ // check quoted literal
if (!isNegated) // Not a Pre-escaped String
{
// go back and insert escPunc before the leading % char saved in lastPct
// move the accumulated %s descriptor
memmove(lastPct+1, lastPct, nf - lastPct); // this is typically very small, src and dest overlap.
++nf;
*lastPct = escPunc;
*nf++ = escPunc;
++escStringsCount;
if (s == NULL)
{
escStringsSize += strlen("(null)");
}
else
{
escStringsSize += strlen(s);
- // DEBUG temporary check for signs of double-escaping, can remove later for a minor speedup:
- if (strstr(s, "\\\\")) // this is really 2 backslashes
- {
- if (sameOk(cfgOption("noSqlInj.dumpStack"), "on"))
- dumpStack("potential sign of double sql-escaping in string [%s]", s);
}
}
+ else // quoted -s has no meaning or use, so not allow.
+ {
+ sqlCheckError("quoted -s in format string is not allowed.");
}
}
}
else
{
errAbort("unexpected error processing vaSqlSafef, format: %s", format);
}
isLong = FALSE;
isLongLong = FALSE;
isNegated = FALSE;
}
else if (strchr("+-.1234567890",c))
{
if (c == '-')
@@ -4171,30 +4203,38 @@
memmove(buffer, tempBuf2, sz+1); // +1 for terminating 0;
}
freeMem(tempBuf2);
}
freeMem(tempBuf);
}
else
{
sz = vsnprintf(buffer, bufSize, newFormat, orig_args);
/* note that some version return -1 if too small */
}
freeMem(newFormat);
va_end(orig_args);
+// Restore prefixes which were removed from string pointer args with %-s
+for (restoreSafeStr = restoreSafeStrList; restoreSafeStr; restoreSafeStr=restoreSafeStr->next)
+ {
+ memmove(restoreSafeStr->s+NOSQLINJ_SIZE, restoreSafeStr->s, restoreSafeStr->strLen - NOSQLINJ_SIZE + 1);
+ memmove(restoreSafeStr->s, NOSQLINJ, NOSQLINJ_SIZE);
+ }
+slFreeList(&restoreSafeStrList);
+
return sz;
}
int vaSqlSafef(char* buffer, int bufSize, char *format, va_list args)
/* VarArgs Format string to buffer, vsprintf style, only with buffer overflow
* checking. The resulting string is always terminated with zero byte. */
{
int sz = vaSqlSafefNoAbort(buffer, bufSize, TRUE, format, args);
if ((sz < 0) || (sz >= bufSize))
{
@@ -4208,184 +4248,113 @@
/* Format string to buffer, vsprintf style, only with buffer overflow
* checking. The resulting string is always terminated with zero byte.
* Scans unquoted string parameters for illegal literal sql chars.
* Escapes quoted string parameters.
* NOSLQINJ tag is added to beginning. */
{
int sz;
va_list args;
va_start(args, format);
sz = vaSqlSafef(buffer, bufSize, format, args);
va_end(args);
return sz;
}
-int vaSqlSafefFrag(char* buffer, int bufSize, char *format, va_list args)
-/* VarArgs Format string to buffer, vsprintf style, only with buffer overflow
- * checking. The resulting string is always terminated with zero byte.
- * Scans unquoted string parameters for illegal literal sql chars.
- * Escapes quoted string parameters.
- * NOSLQINJ tag is NOT added to beginning since it is assumed to be just a fragment of
- * the entire sql string. */
-{
-int sz = vaSqlSafefNoAbort(buffer, bufSize, FALSE, format, args);
-if ((sz < 0) || (sz >= bufSize))
- {
- buffer[bufSize-1] = (char) 0;
- errAbort("buffer overflow, size %d, format: %s, buffer: '%s'", bufSize, format, buffer);
- }
-return sz;
-}
-
-int sqlSafefFrag(char* buffer, int bufSize, char *format, ...)
-/* Format string to buffer, vsprintf style, only with buffer overflow
- * checking. The resulting string is always terminated with zero byte.
- * Scans unquoted string parameters for illegal literal sql chars.
- * Escapes quoted string parameters.
- * NOSLQINJ tag is NOT added to beginning since it is assumed to be just a fragment of
- * the entire sql string. */
-{
-int sz;
-va_list args;
-va_start(args, format);
-sz = vaSqlSafefFrag(buffer, bufSize, format, args);
-va_end(args);
-return sz;
-}
-
-int sqlSafefAppend(char* buffer, int bufSize, char *format, ...)
-/* Append formatted string to buffer, vsprintf style, only with buffer overflow
- * checking. The resulting string is always terminated with zero byte.
- * Scans unquoted string parameters for illegal literal sql chars.
- * Escapes quoted string parameters.
- * NOSLQINJ tag is NOT added to beginning since it is assumed to be appended to
- * a properly created sql string. */
-{
-int sz;
-va_list args;
-int len = strlen(buffer);
-if (len >= bufSize)
- errAbort("sqlSafefAppend() called on string size %d with bufSize %d too small.", len, bufSize);
-va_start(args, format);
-sz = vaSqlSafefFrag(buffer+len, bufSize-len, format, args);
-va_end(args);
-return sz;
-}
-
-
-
/* --------------------------- */
-void vaSqlDyStringPrintfExt(struct dyString *ds, boolean isFrag, char *format, va_list args)
+void vaSqlDyStringPrintfExt(struct dyString *ds, char *format, va_list args)
/* VarArgs Printf to end of dyString after scanning string parameters for illegal sql chars.
* Strings inside quotes are automatically escaped.
- * NOSLQINJ tag is added to beginning if it is a new empty string and isFrag is FALSE. */
+ * NOSLQINJ tag is added to beginning if it is a new empty string. */
{
/* attempt to format the string in the current space. If there
* is not enough room, increase the buffer size and try again */
int avail, sz;
while (TRUE)
{
va_list argscp;
va_copy(argscp, args);
avail = ds->bufSize - ds->stringSize;
if (avail <= 0)
{
/* Don't pass zero sized buffers to vsnprintf, because who knows
* if the library function will handle it. */
dyStringBumpBufSize(ds, ds->bufSize+ds->bufSize);
avail = ds->bufSize - ds->stringSize;
}
- sz = vaSqlSafefNoAbort(ds->string + ds->stringSize, avail, ds->stringSize==0 && !isFrag, format, argscp);
+ if (ds->stringSize > 0 && !startsWith(NOSQLINJ, ds->string))
+ {
+ sqlCheckError("sqlDyPrintf called on non-empty non-safe string.");
+ }
+
+ sz = vaSqlSafefNoAbort(ds->string + ds->stringSize, avail, ds->stringSize==0, format, argscp);
va_end(argscp);
/* note that some version return -1 if too small */
if ((sz < 0) || (sz >= avail))
{
dyStringBumpBufSize(ds, ds->bufSize+ds->bufSize);
}
else
{
ds->stringSize += sz;
break;
}
}
}
void vaSqlDyStringPrintf(struct dyString *ds, char *format, va_list args)
/* VarArgs Printf to end of dyString after scanning string parameters for illegal sql chars.
* Strings inside quotes are automatically escaped.
* NOSLQINJ tag is added to beginning if it is a new empty string.
* Appends to existing string. */
{
-vaSqlDyStringPrintfExt(ds, FALSE, format, args);
+vaSqlDyStringPrintfExt(ds, format, args);
}
void sqlDyStringPrintf(struct dyString *ds, char *format, ...)
/* Printf to end of dyString after scanning string parameters for illegal sql chars.
* Strings inside quotes are automatically escaped.
* NOSLQINJ tag is added to beginning if it is a new empty string.
* Appends to existing string. */
{
va_list args;
va_start(args, format);
vaSqlDyStringPrintf(ds, format, args);
va_end(args);
}
-void vaSqlDyStringPrintfFrag(struct dyString *ds, char *format, va_list args)
-/* VarArgs Printf to end of dyString after scanning string parameters for illegal sql chars.
- * Strings inside quotes are automatically escaped.
- * NOSLQINJ tag is NOT added to beginning since it is assumed to be just a fragment of
- * the entire sql string. Appends to existing string. */
-{
-vaSqlDyStringPrintfExt(ds, TRUE, format, args);
-}
-
-void sqlDyStringPrintfFrag(struct dyString *ds, char *format, ...)
-/* Printf to end of dyString after scanning string parameters for illegal sql chars.
- * Strings inside quotes are automatically escaped.
- * NOSLQINJ tag is NOT added to beginning since it is assumed to be just a fragment of
- * the entire sql string. Appends to existing string. */
-
-{
-va_list args;
-va_start(args, format);
-vaSqlDyStringPrintfFrag(ds, format, args);
-va_end(args);
-}
-
-
struct dyString *sqlDyStringCreate(char *format, ...)
/* Create a dyString with a printf style initial content
* Adds the NOSQLINJ prefix. */
{
int len = strlen(format) * 3;
-struct dyString *ds = newDyString(len);
+struct dyString *ds = dyStringNew(len);
va_list args;
va_start(args, format);
vaSqlDyStringPrintf(ds, format, args);
va_end(args);
return ds;
}
void sqlDyStringPrintIdList(struct dyString *ds, char *fields)
/* Append a comma-separated list of field identifiers. Aborts if invalid characters in list. */
{
-sqlDyStringPrintf(ds, "%-s", sqlCkIl(fields));
+sqlCkIl(fieldsSafe, fields)
+sqlDyStringPrintf(ds, "%-s", fieldsSafe);
}
void sqlDyStringPrintValuesList(struct dyString *ds, struct slName *list)
/* Append a comma-separated, quoted and escaped list of values. */
{
struct slName *el;
for (el = list; el != NULL; el = el->next)
{
if (el != list)
sqlDyStringPrintf(ds, ",");
sqlDyStringPrintf(ds, "'%s'", el->name);
}
}