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/hui.c src/hg/lib/hui.c index 91be8d4..4b6597c 100644 --- src/hg/lib/hui.c +++ src/hg/lib/hui.c @@ -3955,31 +3955,31 @@ safef(suffix, sizeof(suffix), "%s.%s", "filterBy", filterBy->column); boolean parentLevel = isNameAtParentLevel(tdb,tdb->track); if (cartLookUpVariableClosestToHome(cart,tdb,parentLevel,suffix,&(filterBy->htmlName))) { filterBy->slChoices = cartOptionalSlNameList(cart,filterBy->htmlName); freeMem(filterBy->htmlName); } } if (filterBy->slChoices == NULL) // no settings in cart, initialize from trackDb { char *setting = getFilterValueDefaultsSetting(cart, tdb, field); filterBy->slChoices = slNameListFromCommaEscaped(setting); } -struct dyString *dy = newDyString(128); +struct dyString *dy = dyStringNew(128); dyStringPrintf(dy, "%s.%s.%s", name, "filterBy", filterBy->column); filterBy->htmlName = dy->string; return filterBy; } filterBy_t *filterByValues(struct trackDb *tdb, struct cart *cart, struct trackDbFilter *trackDbFilters, char *name) /* Build a filterBy_t list from tdb variables of the form *FilterValues */ { struct asObject *as = asForTdb(NULL, tdb); if (as == NULL) errAbort("Unable to get autoSql for %s", name); filterBy_t *filterByList = NULL, *filter; struct trackDbFilter *fieldFilter; while ((fieldFilter = slPopHead(&trackDbFilters)) != NULL) @@ -4095,56 +4095,56 @@ slNameFreeList(filterBy->slValues); if (filterBy->slChoices != NULL) slNameFreeList(filterBy->slChoices); if (filterBy->htmlName != NULL) freeMem(filterBy->htmlName); freeMem(filterBy->column); freeMem(filterBy); } } } static char *filterByClauseStd(filterBy_t *filterBy) // returns the SQL where clause for a single filterBy struct in the standard cases { int count = slCount(filterBy->slChoices); -struct dyString *dyClause = newDyString(256); -dyStringAppend(dyClause, sqlCkId(filterBy->column)); +struct dyString *dyClause = dyStringNew(256); +sqlDyStringPrintf(dyClause, "%s", filterBy->column); if (count == 1) - dyStringPrintf(dyClause, " = "); + sqlDyStringPrintf(dyClause, " = "); else - dyStringPrintf(dyClause, " in ("); + sqlDyStringPrintf(dyClause, " in ("); struct slName *slChoice = NULL; boolean first = TRUE; for (slChoice = filterBy->slChoices;slChoice != NULL;slChoice=slChoice->next) { if (!first) - dyStringAppend(dyClause, ","); + sqlDyStringPrintf(dyClause, ","); first = FALSE; if (filterBy->useIndex) - dyStringAppend(dyClause, slChoice->name); // a number converted to a string + sqlDyStringPrintf(dyClause, "%d",atoi(slChoice->name)); // a number converted to a string else sqlDyStringPrintf(dyClause, "\"%s\"",slChoice->name); } if (dyStringLen(dyClause) == 0) { dyStringFree(&dyClause); return NULL; } if (count > 1) - dyStringPrintf(dyClause, ")"); + sqlDyStringPrintf(dyClause, ")"); return dyStringCannibalize(&dyClause); } char *filterByClause(filterBy_t *filterBy) // returns the SQL where clause for a single filterBy struct { if (filterByAllChosen(filterBy)) return NULL; else return filterByClauseStd(filterBy); } struct dyString *dyAddFilterByClause(struct cart *cart, struct trackDb *tdb, struct dyString *extraWhere,char *column, boolean *and) @@ -4158,55 +4158,55 @@ { filterBy_t *filterBySet = filterBySetGet(tdb, cart,NULL); if (filterBySet== NULL) return extraWhere; filterBy_t *filterBy = filterBySet; for (;filterBy != NULL; filterBy = filterBy->next) { if (column != NULL && differentString(column,filterBy->column)) continue; char *clause = filterByClause(filterBy); if (clause != NULL) { if (*and) - dyStringPrintf(extraWhere, " AND "); - dyStringAppend(extraWhere, clause); + sqlDyStringPrintf(extraWhere, " AND "); + sqlDyStringPrintf(extraWhere,"%-s", clause); freeMem(clause); *and = TRUE; } } filterBySetFree(&filterBySet); return extraWhere; } char *filterBySetClause(filterBy_t *filterBySet) // returns the "column1 in (...) and column2 in (...)" clause for a set of filterBy structs { -struct dyString *dyClause = newDyString(256); +struct dyString *dyClause = dyStringNew(256); boolean notFirst = FALSE; filterBy_t *filterBy = NULL; for (filterBy = filterBySet;filterBy != NULL; filterBy = filterBy->next) { char *clause = filterByClause(filterBy); if (clause != NULL) { if (notFirst) - dyStringPrintf(dyClause, " AND "); - dyStringAppend(dyClause, clause); + sqlDyStringPrintf(dyClause, " AND "); + sqlDyStringPrintf(dyClause,"%-s", clause); freeMem(clause); notFirst = TRUE; } } if (dyStringLen(dyClause) == 0) { dyStringFree(&dyClause); return NULL; } return dyStringCannibalize(&dyClause); } void filterBySetCfgUiOption(filterBy_t *filterBy, struct slName *slValue, int ix) /* output one option for filterBy UI */ { @@ -5072,31 +5072,31 @@ hashAdd(subgroupHash, vocab->term, vocab); } sqlFreeResult(&sr); hFreeConn(&conn); } return tableHash; } static void printSubtrackTableBody(struct trackDb *parentTdb, struct slRef *subtrackRefList, struct subtrackConfigSettings *settings, struct cart *cart) /* Print list of subtracks */ { sortOrder_t *sortOrder = settings->sortOrder; boolean useDragAndDrop = settings->useDragAndDrop; boolean restrictions = settings->restrictions; -struct dyString *dyHtml = newDyString(SMALLBUF); +struct dyString *dyHtml = dyStringNew(SMALLBUF); char buffer[SMALLBUF]; char id[SMALLBUF]; char *db = cartString(cart, "db"); // The subtracks need to be sorted by priority but only sortable and dragable will have // non-default (cart) priorities to sort on if (sortOrder != NULL || useDragAndDrop) { // preserves user's prev sort/drags, ignore returned value about where // priorities come from (void) tdbRefSortPrioritiesFromCart(cart, &subtrackRefList); printf("<TBODY class='%saltColors'>\n", (sortOrder != NULL ? "sortable " : "") ); } else { @@ -5387,68 +5387,68 @@ printf("</TD>\n<TD align='center'> %s ", dateDisplay); } } // End of row and free ourselves of this subtrack puts("</TD></TR>\n"); boolean showCfg = trackDbSettingOn(subtrack, "showCfg"); if (showCfg) jsInlineF(" subCfg.cfgToggle(document.getElementById(\"%s_toggle\"),\"%s\");", subtrack->track, subtrack->track); } // End of the table puts("</TBODY>"); -dyStringFree(&dyHtml) +dyStringFree(&dyHtml); membersForAllSubGroupsFree(parentTdb,&membersForAll); } static boolean membersHaveMatrix(membersForAll_t *membersForAll) /* Check for matrix */ { if (membersForAll->members[dimX] == NULL && membersForAll->members[dimY] == NULL) return FALSE; return TRUE; } static void printSubtrackTable(struct trackDb *parentTdb, struct slRef *subtrackRefList, struct subtrackConfigSettings *settings, struct cart *cart) /* Print table of subtracks */ { // Print table tag printf("\n<TABLE CELLSPACING='2' CELLPADDING='0' border='0'"); -struct dyString *dyHtml = newDyString(SMALLBUF); +struct dyString *dyHtml = dyStringNew(SMALLBUF); if (settings->sortOrder != NULL) dyStringPrintf(dyHtml, "sortable"); if (settings->useDragAndDrop) { if (dyStringLen(dyHtml) > 0) dyStringAppendC(dyHtml,' '); dyStringPrintf(dyHtml, "tableWithDragAndDrop"); } printf(" class='subtracks"); if (dyStringLen(dyHtml) > 0) { printf(" bglevel1 %s'",dyStringContents(dyHtml)); settings->bgColorIx = COLOR_BG_ALTDEFAULT_IX; } else settings->bgColorIx = COLOR_BG_DEFAULT_IX; // Start with non-default allows alternation puts("'>"); -dyStringFree(&dyHtml) +dyStringFree(&dyHtml); // save count of subtracks for use by footer code int subCount = slCount(subtrackRefList); printSubtrackTableHeader(parentTdb, subtrackRefList, settings); printSubtrackTableBody(parentTdb, subtrackRefList, settings, cart); printSubtrackTableFooter(subCount, settings); puts("</TABLE>"); } boolean compositeHideEmptySubtracksSetting(struct trackDb *tdb, boolean *retDefault, char **retMultiBedFile, char **retSubtrackIdFile) /* Parse hideEmptySubtracks settings * Format: hideEmptySubtracks on|off * Optional index files for performance: @@ -7025,36 +7025,44 @@ } // else no default limits! if (invalid) { safef(filterLimitName, sizeof(filterLimitName), "%s%s", filter, (max!=NO_VALUE?_MIN:"")); cartRemoveVariableClosestToHome(cart,tdb,FALSE,filterLimitName); safef(filterLimitName, sizeof(filterLimitName), "%s%s", filter, _MAX); cartRemoveVariableClosestToHome(cart,tdb,FALSE,filterLimitName); } else if ((min != NO_VALUE && (minLimit == NO_VALUE || minLimit != min)) || (max != NO_VALUE && (maxLimit == NO_VALUE || maxLimit != max))) // Assumes min==NO_VALUE or min==minLimit is no filter // Assumes max==NO_VALUE or max==maxLimit is no filter! { if (max == NO_VALUE || (maxLimit != NO_VALUE && maxLimit == max)) - dyStringPrintf(extraWhere, "%s(%s >= %d)", (*and?" and ":""),field,min); // min only + { + if (*and) sqlDyStringPrintf(extraWhere, " and "); + sqlDyStringPrintf(extraWhere, "(%s >= %d)", field, min); // min only + } else if (min == NO_VALUE || (minLimit != NO_VALUE && minLimit == min)) - dyStringPrintf(extraWhere, "%s(%s <= %d)", (*and?" and ":""),field,max); // max only + { + if (*and) sqlDyStringPrintf(extraWhere, " and "); + sqlDyStringPrintf(extraWhere, "(%s <= %d)", field, max); // max only + } else - dyStringPrintf(extraWhere, "%s(%s BETWEEN %d and %d)", (*and?" and ":""), // both - field,min,max); + { + if (*and) sqlDyStringPrintf(extraWhere, " and "); + sqlDyStringPrintf(extraWhere, "(%s BETWEEN %d and %d)", field, min, max); // both + } *and=TRUE; } } //if (dyStringLen(extraWhere)) // warn("SELECT FROM %s WHERE %s",tdb->table,dyStringContents(extraWhere)); return extraWhere; } struct dyString *dyAddFilterAsDouble(struct cart *cart, struct trackDb *tdb, struct dyString *extraWhere,char *filter, char *defaultLimits, char*field, boolean *and) // creates the where clause condition to support numeric double filters. // Filters are expected to follow // {fiterName}: trackDb min or min:max - default value(s); // {filterName}Min or {filterName}: min (user supplied) cart variable; @@ -7099,36 +7107,44 @@ } if (invalid) { char filterLimitName[64]; safef(filterLimitName, sizeof(filterLimitName), "%s%s", filter, _MIN); cartRemoveVariableClosestToHome(cart,tdb,FALSE,filterLimitName); safef(filterLimitName, sizeof(filterLimitName), "%s%s", filter, _MAX); cartRemoveVariableClosestToHome(cart,tdb,FALSE,filterLimitName); } else if (((int)min != NO_VALUE && ((int)minLimit == NO_VALUE || minLimit != min)) || ((int)max != NO_VALUE && ((int)maxLimit == NO_VALUE || maxLimit != max))) // Assumes min==NO_VALUE or min==minLimit is no filter // Assumes max==NO_VALUE or max==maxLimit is no filter! { if ((int)max == NO_VALUE || ((int)maxLimit != NO_VALUE && maxLimit == max)) - dyStringPrintf(extraWhere, "%s(%s >= %g)", (*and?" and ":""),field,min); // min only + { + if (*and) sqlDyStringPrintf(extraWhere, " and "); + sqlDyStringPrintf(extraWhere, "(%s >= %g)", field, min); // min only + } else if ((int)min == NO_VALUE || ((int)minLimit != NO_VALUE && minLimit == min)) - dyStringPrintf(extraWhere, "%s(%s <= %g)", (*and?" and ":""),field,max); // max only + { + if (*and) sqlDyStringPrintf(extraWhere, " and "); + sqlDyStringPrintf(extraWhere, "(%s <= %g)", field, max); // max only + } else - dyStringPrintf(extraWhere, "%s(%s BETWEEN %g and %g)", (*and?" and ":""), // both - field,min,max); + { + if (*and) sqlDyStringPrintf(extraWhere, " and "); + sqlDyStringPrintf(extraWhere, "(%s BETWEEN %g and %g)", field,min,max); // both + } *and=TRUE; } } //if (dyStringLen(extraWhere)) // warn("SELECT FROM %s WHERE %s",tdb->table,dyStringContents(extraWhere)); return extraWhere; } struct dyString *dyAddAllScoreFilters(struct cart *cart, struct trackDb *tdb, struct dyString *extraWhere,boolean *and) // creates the where clause condition to gather together all random double filters // Filters are expected to follow // {fiterName}: trackDb min or min:max - default value(s); // {filterName}Min or {filterName}: min (user supplied) cart variable; // {filterName}Max: max (user supplied) cart variable; @@ -9712,31 +9728,31 @@ struct asColumn *col; for (col = as->columnList; col != NULL; col = col->next) { el = slNameNew(col->name); slAddHead(&list, el); } slReverse(&list); return list; } static struct dyString *subMultiField(char *pattern, int fieldCount, char *in[], char *out[]) /* Substitute $in with out values in pattern */ { int i; -struct dyString *s = newDyString(256), *d = NULL; +struct dyString *s = dyStringNew(256), *d = NULL; dyStringAppend(s, pattern); for (i=0; i<fieldCount; ++i) { if (out[i]==NULL) continue; // If a field is a prefix or suffix to another field, for example 'chrom' and 'chromStart' // we don't want to erroneously sub out the 'chrom' in 'chromStart'. Allow the wrapping // protected fields in ${} to prevent the substitution: char *field = in[i]; int fieldLen = strlen(field); char *spec = needMem(fieldLen + 2); char *strictSpec = needMem(fieldLen + 4); *spec = '$'; *strictSpec = '$'; @@ -9763,31 +9779,31 @@ return s; } char *replaceFieldInPattern(char *pattern, int fieldCount, char **fieldNames, char **fieldVals) /* Replace $fieldName in pattern with value. Used in trackDb mouseOver setting */ { struct dyString *ds = subMultiField(pattern, fieldCount, fieldNames, fieldVals); return dyStringCannibalize(&ds); } static struct dyString *subMulti(char *orig, int subCount, char *in[], char *out[]) /* Perform multiple substitions on orig. */ { int i; -struct dyString *s = newDyString(256), *d = NULL; +struct dyString *s = dyStringNew(256), *d = NULL; dyStringAppend(s, orig); for (i=0; i<subCount; ++i) { fflush(stdout); if (out[i]==NULL) continue; d = dyStringSub(s->string, in[i], out[i]); dyStringFree(&s); s = d; d = NULL; } return s; } @@ -9878,31 +9894,31 @@ outs[9] = startString; outs[10] = endString; safef(oneBasedStart, sizeof(oneBasedStart), "%d", winStart + 1); outs[13] = oneBasedStart; } ins[11] = "$n"; outs[11] = scName; ins[12] = "$taxId"; outs[12] = taxId; uUrl = subMulti(url, ArraySize(ins), ins, outs); outs[0] = eItem; eUrl = subMulti(url, ArraySize(ins), ins, outs); -freeDyString(&uUrl); +dyStringFree(&uUrl); freeMem(eItem); freeMem(scName); // substitute $<fieldName> variables if (!fields) return eUrl->string; int fieldCount = slCount(fields); char **fieldNames = NULL, **fieldVals = NULL; AllocArray(fieldNames, fieldCount); AllocArray(fieldVals, fieldCount); int i; struct slPair *field; for (i=0, field=fields; i<fieldCount; i++, field=field->next) {