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/encode/encodeExp.c src/hg/lib/encode/encodeExp.c index ac97a5a..ce2b751 100644 --- src/hg/lib/encode/encodeExp.c +++ src/hg/lib/encode/encodeExp.c @@ -387,43 +387,45 @@ struct dyString *dy; char *which = NULL; if (sameString(action, "insert") || sameString(action, "update")) which = "NEW"; else if sameString(action, "delete") which = "OLD"; else errAbort("Invalid SQL trigger action: %s", action); dy = sqlDyStringCreate( "CREATE TRIGGER %s_%s AFTER %s ON %s FOR EACH ROW INSERT INTO %s%s VALUES \n" "(%s.ix, %s.organism, %s.lab, %s.dataType, %s.cellType, %s.expVars, %s.accession, NOW(), '%c', USER(), '')", tableName, action, action, tableName, tableName, ENCODE_EXP_HISTORY_TABLE_SUFFIX, which, which, which, which, which, which, which, toupper(action[0])); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); +dyStringFree(&dy); } static void encodExpDropHistoryTrigger(struct sqlConnection *conn, char *tableName, char *action) /* Drop history trigger from a table */ { struct dyString *dy; if (differentString(action, "insert") && differentString(action, "update") && differentString(action, "delete")) errAbort("Invalid SQL trigger action: %s", action); dy = sqlDyStringCreate("DROP TRIGGER IF EXISTS %s_%s", tableName, action); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); +dyStringFree(&dy); } static void encodExpAddTriggers(struct sqlConnection *conn, char *tableName) { /* Add history triggers to experiment table */ encodExpAddHistoryTrigger(conn, tableName, "insert"); encodExpAddHistoryTrigger(conn, tableName, "update"); encodExpAddHistoryTrigger(conn, tableName, "delete"); } static void encodeExpDropTriggers(struct sqlConnection *conn, char *tableName) { /* Drop history triggers from experiment table */ encodExpDropHistoryTrigger(conn, tableName, "insert"); encodExpDropHistoryTrigger(conn, tableName, "update"); @@ -465,143 +467,154 @@ encodeExpDropTriggers(conn, tableName); sqlDropTable(conn, tableName); safef(buf, sizeof buf, "%s%s", tableName, ENCODE_EXP_HISTORY_TABLE_SUFFIX); sqlDropTable(conn, buf); } #define ENCODE_EXP_HISTORY_FIELD_WHAT "action" #define ENCODE_EXP_HISTORY_FIELD_WHO "changedBy" #define ENCODE_EXP_HISTORY_FIELD_WHY "why" void encodeExpTableCreate(struct sqlConnection *conn, char *tableName) /* Create an encodeExp table */ { struct dyString *dy; dy = sqlDyStringCreate(sqlCreate, tableName); -sqlRemakeTable(conn, tableName, dyStringCannibalize(&dy)); +sqlRemakeTable(conn, tableName, dyStringContents(dy)); +dyStringFree(&dy); /* Create history table -- a clone with 3 additional columns (action, changedBy, why). * 'why' is only required for deletes * Remove auto-inc attribute on ix, and use ix and updateTime as primary key */ dy = sqlDyStringCreate("CREATE TABLE %s%s LIKE %s", tableName, ENCODE_EXP_HISTORY_TABLE_SUFFIX, tableName); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); +dyStringFree(&dy); dy = sqlDyStringCreate("ALTER TABLE %s%s ADD COLUMN %s CHAR(1) DEFAULT ''", tableName, ENCODE_EXP_HISTORY_TABLE_SUFFIX, ENCODE_EXP_HISTORY_FIELD_WHAT); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); +dyStringFree(&dy); dy = sqlDyStringCreate("ALTER TABLE %s%s ADD COLUMN %s VARCHAR(77) NOT NULL", tableName, ENCODE_EXP_HISTORY_TABLE_SUFFIX, ENCODE_EXP_HISTORY_FIELD_WHO); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); +dyStringFree(&dy); dy = sqlDyStringCreate("ALTER TABLE %s%s ADD COLUMN %s VARCHAR(255) NOT NULL", tableName, ENCODE_EXP_HISTORY_TABLE_SUFFIX, ENCODE_EXP_HISTORY_FIELD_WHY); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); +dyStringFree(&dy); + dy = sqlDyStringCreate("ALTER TABLE %s%s MODIFY COLUMN %s INT DEFAULT 0", tableName, ENCODE_EXP_HISTORY_TABLE_SUFFIX, ENCODE_EXP_FIELD_IX); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); +dyStringFree(&dy); dy = sqlDyStringCreate("ALTER TABLE %s%s DROP PRIMARY KEY", tableName, ENCODE_EXP_HISTORY_TABLE_SUFFIX); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); +dyStringFree(&dy); //possible TODO: if we do need a primary key, will need to add a msec or autoinc column */ //alter table %s add idx int unsigned not null auto_increment, add primary key (idx); encodExpAddTriggers(conn, tableName); } int encodeExpIdMax(struct sqlConnection *conn) /* Return largest ix value */ { -return sqlQuickNum(conn, NOSQLINJ "select max(ix) from " ENCODE_EXP_TABLE); +char query[1024]; +sqlSafef(query, sizeof query, "select max(ix) from %s", ENCODE_EXP_TABLE); +return sqlQuickNum(conn, query); } /* END schema-dependent section */ struct encodeExp *encodeExpLoadAllFromTable(struct sqlConnection *conn, char *tableName) /* Load all encodeExp in table */ { -struct encodeExp *exps = NULL; - if (!sqlTableExists(conn, tableName)) return NULL; -struct dyString *dy = newDyString(0); +struct dyString *dy = dyStringNew(0); sqlDyStringPrintf(dy, "select * from %s", tableName); +struct encodeExp *exps = NULL; exps = encodeExpLoadByQuery(conn, dyStringContents(dy)); dyStringFree(&dy); return exps; } static void encodeExpAddToLatestHistory(struct sqlConnection *conn, char *table, int id, char *field, char *value) /* Add user name to history table record */ { struct dyString *dy = sqlDyStringCreate( "select max(updateTime) from %s%s where %s='%d'", table, ENCODE_EXP_HISTORY_TABLE_SUFFIX, ENCODE_EXP_FIELD_IX, id); verbose(3, "%s\n", dy->string); -char *updateTime = sqlQuickString(conn, dyStringCannibalize(&dy)); +char *updateTime = sqlQuickString(conn, dyStringContents(dy)); +dyStringFree(&dy); dy = sqlDyStringCreate( "update %s%s set %s='%s' where updateTime = '%s'", table, ENCODE_EXP_HISTORY_TABLE_SUFFIX, field, value, updateTime); verbose(3, "%s\n", dy->string); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); +dyStringFree(&dy); } static void encodeExpAddUserToLatestHistory(struct sqlConnection *conn, char *table, int id) /* Add user name to history table record */ { encodeExpAddToLatestHistory(conn, table, id, ENCODE_EXP_HISTORY_FIELD_WHO, getlogin()); } static void encodeExpAddWhyToLatestHistory(struct sqlConnection *conn, char *table, int id, char *why) /* Add comment to history table record */ { encodeExpAddToLatestHistory(conn, table, id, ENCODE_EXP_HISTORY_FIELD_WHY, why); } void encodeExpSaveToDb(struct sqlConnection *conn, struct encodeExp *el, char *tableName, int updateSize) /* Save encodeExp as a row to the table specified by tableName. * As blob fields may be arbitrary size updateSize specifies the approx size. * of a string that would contain the entire query. Automatically * escapes all simple strings (not arrays of string). */ { // NOTE: Manually updated to handle NULL fields, including setting NULL (for expVars and accession) -struct dyString *update = newDyString(updateSize); +struct dyString *update = dyStringNew(updateSize); sqlDyStringPrintf(update, "insert into %s set %s=%u, %s='%s', %s='%s', %s='%s', %s='%s'", tableName, ENCODE_EXP_FIELD_IX, el->ix, ENCODE_EXP_FIELD_ORGANISM, el->organism, ENCODE_EXP_FIELD_LAB, el->lab, ENCODE_EXP_FIELD_DATA_TYPE, el->dataType, ENCODE_EXP_FIELD_CELL_TYPE, el->cellType); // Note: The sql literal NULL is not quoted in the final sql string. sqlDyStringPrintf(update, ", %s=", ENCODE_EXP_FIELD_FACTORS); if (el->expVars == NULL) - dyStringAppend(update, "NULL"); + sqlDyStringPrintf(update, "NULL"); else sqlDyStringPrintf(update, "'%s'", el->expVars); sqlDyStringPrintf(update, ", %s=", ENCODE_EXP_FIELD_ACCESSION); if (el->accession == NULL) - dyStringAppend(update, "NULL"); + sqlDyStringPrintf(update, "NULL"); else sqlDyStringPrintf(update, "'%s'", el->accession); sqlGetLock(conn, ENCODE_EXP_TABLE_LOCK); sqlUpdate(conn, update->string); int id = sqlLastAutoId(conn); encodeExpAddUserToLatestHistory(conn, tableName, id); sqlReleaseLock(conn, ENCODE_EXP_TABLE_LOCK); -freeDyString(&update); +dyStringFree(&update); } struct encodeExp *encodeExpFromMdb(struct sqlConnection *conn, char *db, struct mdbObj *mdb) /* Create an encodeExp from an ENCODE metaDb object */ { if (!mdbObjIsEncode(mdb)) errAbort("Metadata object is not from ENCODE"); struct mdbVar *edVars = mdbObjFindEncodeEdvs(conn,mdb,FALSE); // exclude vars where val=None // To use shared metaDb: //struct mdbVar *edVars = mdbObjFindEncodeEdvPairs(conn, MDB_DEFAULT_NAME, mdb, FALSE); if (edVars == NULL) { // Not willing to make these erraborts at this time. char *composite = mdbObjFindValue(mdb,MDB_VAR_COMPOSITE); @@ -733,34 +746,36 @@ if (differentStringNullOk(fp->get(exp), fp->get(exp2))) return FALSE; } return TRUE; } struct hash *encodeExpToRa(struct encodeExp *exp) /* Create a Ra hash from an encodeExp */ { return encodeExpToRaFile(exp, NULL); } struct encodeExp *encodeExpGetByIdFromTable(struct sqlConnection *conn, char *tableName, int id) /* Return experiment specified by id from named table */ { -struct dyString *query = NULL; +struct dyString *dy = sqlDyStringCreate("select * from %s where %s=\'%d\'", tableName, ENCODE_EXP_FIELD_IX, id); -query = sqlDyStringCreate("select * from %s where %s=\'%d\'", tableName, ENCODE_EXP_FIELD_IX, id); -return encodeExpLoadByQuery(conn, dyStringCannibalize(&query)); +struct encodeExp *exps = NULL; +exps = encodeExpLoadByQuery(conn, dyStringContents(dy)); +dyStringFree(&dy); +return exps; } struct encodeExp *encodeExpGetById(struct sqlConnection *conn, int id) /* Return experiment specified by id from default table */ { return encodeExpGetByIdFromTable(conn, ENCODE_EXP_TABLE, id); } struct encodeExp *encodeExpGetByAccession(struct sqlConnection *conn, char *accession) /* Return experiment specified by accession from default table */ { if (accession == NULL || strlen(accession) <= encodeExpIdOffset()) return NULL; int id = atoi(accession + encodeExpIdOffset()); return encodeExpGetById(conn, id); @@ -809,33 +824,34 @@ sqlGetLock(conn, ENCODE_EXP_TABLE_LOCK); exp = encodeExpGetByIdFromTable(conn, tableName, id); if (exp == NULL) errAbort("Experiment id %d not found in table %s", id, tableName); if (add) { accession = encodeExpMakeAccession(exp); safef(queryAcc, sizeof(queryAcc), "\'%s\'", accession); } else safecpy(queryAcc, sizeof(queryAcc), "NULL"); query = sqlDyStringCreate("update %s set %s=%s where %s=%d", tableName, ENCODE_EXP_FIELD_ACCESSION, queryAcc, ENCODE_EXP_FIELD_IX, exp->ix); -sqlUpdate(conn, dyStringCannibalize(&query)); +sqlUpdate(conn, query->string); encodeExpAddUserToLatestHistory(conn, tableName, id); sqlReleaseLock(conn, ENCODE_EXP_TABLE_LOCK); +dyStringFree(&query); return accession; } char *encodeExpAddAccession(struct sqlConnection *conn, char *tableName, int id) /* Add accession field to an existing "temp" experiment. This is done * after experiment is determined to be valid. * Return the accession. */ { return encodeExpAccession(conn, tableName, id, TRUE); } void encodeExpSetAccession(struct encodeExp *exp, char *tableName) /* Adds accession field to an existing experiment, updating the table. */ { struct sqlConnection *conn = sqlConnect(ENCODE_EXP_DATABASE); @@ -996,94 +1012,94 @@ { // this var not found in this experiment - add new var if (oldVal && differentString(oldVal, ENCODE_EXP_NO_VAR)) { errAbort("Attempt to change expVar %s from value %s not found in experiment %d", var, oldVal, id); } verbose(3, "Adding %s=%s to experiment %d\n", var, newVal, id); slPairAdd(&varPairs, var, newVal); slPairSortCase(&varPairs); verbose(1, "WARNING: not verifying %s is valid expVar for this experiment\n", var); } char *expVars = slPairListToString(varPairs, FALSE); dy = sqlDyStringCreate("update %s set %s=\'%s\' ", tableName, ENCODE_EXP_FIELD_FACTORS, expVars); } -dyStringPrintf(dy, " where ix=%d", id); +sqlDyStringPrintf(dy, " where ix=%d", id); sqlGetLock(conn, ENCODE_EXP_TABLE_LOCK); -sqlUpdate(conn, dyStringCannibalize(&dy)); +sqlUpdate(conn, dyStringContents(dy)); encodeExpAddUserToLatestHistory(conn, tableName, id); sqlReleaseLock(conn, ENCODE_EXP_TABLE_LOCK); +dyStringFree(&dy); } char *encodeExpKey(struct encodeExp *exp) /* Create a hash key from an encodeExp */ { -struct dyString *dy = newDyString(0); +struct dyString *dy = dyStringNew(0); dyStringPrintf(dy, "lab:%s dataType:%s cellType:%s", exp->lab, exp->dataType, exp->cellType); if (exp->expVars != NULL) dyStringPrintf(dy, " expVars:%s", exp->expVars); return dyStringCannibalize(&dy); } char *encodeExpVars(struct encodeExp *exp) // Create a string of all experiment defining vars and vals as "lab=UW dataType=ChipSeq ..." // WARNING: May be missing var=None if the var was added after composite had defined exps. { -struct dyString *dy = newDyString(0); +struct dyString *dy = dyStringNew(0); dyStringPrintf(dy, "%s=%s %s=%s", MDB_VAR_LAB, exp->lab, MDB_VAR_DATATYPE, exp->dataType ); if (exp->cellType != NULL) dyStringPrintf(dy, " %s=%s", MDB_VAR_CELL, exp->cellType); if (exp->expVars != NULL) dyStringPrintf(dy, " %s", exp->expVars); return dyStringCannibalize(&dy); } struct encodeExp *encodeExpGetFromTable(char *organism, char *lab, char *dataType, char *cell, struct slPair *varPairs, char *table) /* Return experiments matching args in named experiment table. * Organism, Lab and DataType must be non-null */ { -struct encodeExp *exps = NULL; - if (organism == NULL || lab == NULL || dataType == NULL) errAbort("Need organism, lab, and dataType to query experiment table"); if (cell == NULL) cell = ENCODE_EXP_NO_CELL; struct sqlConnection *conn = sqlConnect(ENCODE_EXP_DATABASE); struct dyString *dy = sqlDyStringCreate( "select * from %s where %s=\'%s\' and %s=\'%s\' and %s=\'%s\' and %s=\'%s\' and %s", table, ENCODE_EXP_FIELD_ORGANISM, organism, ENCODE_EXP_FIELD_LAB, lab, ENCODE_EXP_FIELD_DATA_TYPE, dataType, ENCODE_EXP_FIELD_CELL_TYPE, cell, ENCODE_EXP_FIELD_FACTORS); /* construct expVars string var=val from pairs */ if (varPairs == NULL) - dyStringAppend(dy, " is NULL"); + sqlDyStringPrintf(dy, " is NULL"); else { - dyStringAppend(dy, "="); - dyStringQuoteString(dy, '\'', slPairListToString(varPairs, FALSE)); + sqlDyStringPrintf(dy, "= '%s'", slPairListToString(varPairs, FALSE)); } verbose(4, "query: %s\n", dy->string); -exps = encodeExpLoadByQuery(conn, dyStringCannibalize(&dy)); +struct encodeExp *exps = NULL; +exps = encodeExpLoadByQuery(conn, dyStringContents(dy)); sqlDisconnect(&conn); +dyStringFree(&dy); return exps; } struct encodeExp *encodeExpGet(char *organism, char *lab, char *dataType, char *cell, struct slPair *varPairs) /* Return experiments matching args in default experiment table. * Organism, Lab and DataType must be non-null */ { return encodeExpGetFromTable(organism, lab, dataType, cell, varPairs, ENCODE_EXP_TABLE); } struct encodeExp *encodeExpGetByMdbVarsFromTable(char *db, struct mdbVar *vars, char *table) /* Return experiments by looking up mdb var list from the named experiment table */ { struct encodeExp *exp = encodeExpFromMdbVars(db,vars);