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/hgFind.c src/hg/lib/hgFind.c index 899aabb..8ba941b 100644 --- src/hg/lib/hgFind.c +++ src/hg/lib/hgFind.c @@ -649,31 +649,31 @@ gotIt = TRUE; } freez(&lowered); trixSearchResultFreeList(&tsrList); trixClose(&trix); return gotIt; } static char *getUiUrl(struct cart *cart) /* Get rest of UI from browser. */ { static struct dyString *dy = NULL; static char *s = NULL; if (dy == NULL) { - dy = newDyString(64); + dy = dyStringNew(64); if (cart != NULL) dyStringPrintf(dy, "%s=%s", cartSessionVarName(), cartSessionId(cart)); s = dy->string; } return s; } static void singlePos(struct hgPositions *hgp, char *tableDescription, char *posDescription, char *tableName, char *posName, char *browserName, char *chrom, int start, int end) /* Fill in pos for simple case single position. */ { struct hgPosTable *table; struct hgPos *pos; @@ -1008,31 +1008,31 @@ } slReverse(&pslList); sqlFreeResult(&sr); } return pslList; } static void addPslResultToHgp(struct cart *cart, struct hgPositions *hgp, char *db, char *tableName, char *shortLabel, char *acc, struct psl *pslList) /* Create an hgPosTable for the given psl search results, and add it to hgp->tableList. */ { if (pslList == NULL) return; struct hgPosTable *table; -struct dyString *dy = newDyString(1024); +struct dyString *dy = dyStringNew(1024); struct psl *psl; char hgAppCombiner = (strchr(hgAppName, '?')) ? '&' : '?'; char *ui = getUiUrl(cart); AllocVar(table); table->htmlStart = mrnaHtmlStart; table->htmlEnd = mrnaHtmlEnd; table->htmlOnePos = mrnaHtmlOnePos; slAddHead(&hgp->tableList, table); dyStringPrintf(dy, "%s Alignments in %s", acc, shortLabel); table->description = cloneString(dy->string); table->name = cloneString(tableName); char *trackName = hGetTrackForTable(db, table->name); slSort(&pslList, pslCmpScore); for (psl = pslList; psl != NULL; psl = psl->next) { @@ -1048,31 +1048,31 @@ hgAppName, hgAppCombiner, hgPosBrowserRange(pos, NULL), trackName, hCarefulTrackOpenVisCart(cart, db, trackName)); if (ui != NULL) dyStringPrintf(dy, "&%s", ui); dyStringPrintf(dy, "%s\">", hgp->extraCgi); dyStringPrintf(dy, "%5d %5.1f%% %9s %s %9d %9d %8s %5d %5d %5d</A>", psl->match + psl->misMatch + psl->repMatch + psl->nCount, 100.0 - pslCalcMilliBad(psl, TRUE) * 0.1, skipChr(psl->tName), psl->strand, psl->tStart + 1, psl->tEnd, psl->qName, psl->qStart+1, psl->qEnd, psl->qSize); dyStringPrintf(dy, "\n"); pos->description = cloneString(dy->string); slAddHead(&table->posList, pos); } slReverse(&table->posList); -freeDyString(&dy); +dyStringFree(&dy); } static boolean findMrnaPos(struct cart *cart, char *db, char *acc, struct hgPositions *hgp) /* Find MRNA or EST position(s) from accession number. * Look to see if it's an mRNA or EST. Fill in hgp and return * TRUE if it is, otherwise return FALSE. */ /* NOTE: this excludes RefSeq mrna's, as they are currently * handled in findRefGenes(), which is called later in the main function */ { struct sqlConnection *conn = hAllocConn(db); if (!sqlTableExists(conn, gbCdnaInfoTable)) { hFreeConn(&conn); return FALSE; } @@ -1237,64 +1237,64 @@ return FALSE; return TRUE;; } static void findHitsToTables(char *db, struct hgFindSpec *hfs, char *key, int limitResults, char *tables[], int tableCount, struct hash **retHash, struct slName **retList) /* Return all unique accessions that match any table. */ // Modified to return only the first 500 hits because of CGI timeouts { struct slName *list = NULL, *el; struct hash *hash = newHash(0); struct sqlConnection *conn = hAllocConn(db); struct sqlResult *sr; char **row; -char query[256]; char *field; int i; int rowCount = 0; // Excessively broad searches were leading to CGI timeouts (#11626). for (i = 0; i<tableCount && (limitResults == EXHAUSTIVE_SEARCH_REQUIRED || rowCount < limitResults); ++i) { struct slName *idList = NULL, *idEl; char *grepIndexFile = NULL; /* I'm doing this query in two steps in C rather than * in one step in SQL just because it somehow is much * faster this way (like 100x faster) when using mySQL. */ field = tables[i]; if (!sqlTableExists(conn, field)) continue; if ((grepIndexFile = getGenbankGrepIndex(db, hfs, field, "idName")) != NULL) idList = genbankGrepQuery(grepIndexFile, field, key); else idList = genbankSqlFuzzyQuery(conn, field, key, limitResults); for (idEl = idList; idEl != NULL && (limitResults == EXHAUSTIVE_SEARCH_REQUIRED || rowCount < limitResults); idEl = idEl->next) { /* don't check srcDb to exclude refseq for compat with older tables */ - sqlSafef(query, sizeof(query), + struct dyString *query = sqlDyStringCreate( "select acc, organism from %s where %s = '%s' " " and type = 'mRNA'", gbCdnaInfoTable, skipDb(field), idEl->name); // limit results to avoid CGI timeouts (#11626). if (limitResults != EXHAUSTIVE_SEARCH_REQUIRED) - sqlSafefAppend(query, sizeof(query), " limit %d", limitResults); - sr = sqlGetResult(conn, query); + sqlDyStringPrintf(query, " limit %d", limitResults); + sr = sqlGetResult(conn, dyStringContents(query)); + dyStringFree(&query); while ((row = sqlNextRow(sr)) != NULL) { char *acc = row[0]; /* will use this later to distinguish xeno mrna */ int organismID = sqlUnsigned(row[1]); if (!isRefSeqAcc(acc) && !hashLookup(hash, acc)) { el = newSlName(acc); slAddHead(&list, el); hashAddInt(hash, acc, organismID); // limit results to avoid CGI timeouts (#11626). if (rowCount++ > limitResults && limitResults != EXHAUSTIVE_SEARCH_REQUIRED) break; } } @@ -1346,31 +1346,31 @@ "select count(*) from %s where qName = '%s'", table, acc); return (sqlQuickNum(conn, query) > 0); } static int addMrnaPositionTable(char *db, struct hgPositions *hgp, struct slName **pAccList, struct hash *accOrgHash, struct cart *cart, struct sqlConnection *conn, char *hgAppName, boolean aligns, boolean isXeno) /* Generate table of positions that match criteria. * Add to hgp if any found. Return number found */ { struct hgPosTable *table = NULL; struct slName *el = NULL; struct slName *elToFree = NULL; -struct dyString *dy = newDyString(256); +struct dyString *dy = dyStringNew(256); char *ui = getUiUrl(cart); int organismID = hOrganismID(hgp->database); /* id from mrna organism table */ int alignCount = 0; char hgAppCombiner = (strchr(hgAppName, '?')) ? '&' : '?'; char *mrnaTable = isXeno ? "xenoMrna" : "all_mrna"; boolean mrnaTableExists = hTableExists(hgp->database, mrnaTable); AllocVar(table); /* Examine all accessions to see if they fit criteria for * this table. Add all matching to the position list, and * remove from the accession list */ for (el = *pAccList; el != NULL; el = el->next) { freez(&elToFree); @@ -1479,31 +1479,31 @@ } else { char title[256]; slReverse(&table->posList); safef(title, sizeof(title), "%s%s %sligned mRNA Search Results", isXeno ? "Non-" : "", organism, aligns ? "A" : "Una"); table->description = cloneString(title); table->name = cloneString(mrnaTable); table->htmlOnePos = mrnaKeysHtmlOnePos; slAddHead(&hgp->tableList, table); } freeMem(organism); } -freeDyString(&dy); +dyStringFree(&dy); return alignCount; } static boolean findMrnaKeys(struct cart *cart, char *db, struct hgFindSpec *hfs, char *keys, int limitResults, struct hgPositions *hgp) /* Find mRNA that has keyword in one of its fields. */ { int alignCount; char *tables[] = { productNameTable, geneNameTable, authorTable, tissueTable, cellTable, descriptionTable, developmentTable, }; struct hash *allKeysHash = NULL; struct slName *allKeysList = NULL; struct sqlConnection *conn = hAllocConn(db); @@ -1625,31 +1625,31 @@ sr = sqlGetResult(conn, query); while ((row = sqlNextRow(sr)) != NULL) { struct refLink *rl = refLinkLoad(row); slAddHead(pList, rl); } sqlFreeResult(&sr); } } static boolean findRefGenes(char *db, struct hgFindSpec *hfs, char *spec, struct hgPositions *hgp) /* Look up refSeq genes in table. */ { struct sqlConnection *conn = hAllocConn(db); -struct dyString *ds = newDyString(256); +struct dyString *ds = dyStringNew(256); struct refLink *rlList = NULL, *rl; boolean gotRefLink = sqlTableExists(conn, refLinkTable); boolean found = FALSE; char *specNoVersion = cloneString(spec); // chop off the version number, e.g. "NM_000454.4 ", // but if spec starts with "." like ".stuff" then specNoVersion is entirely empty. (void) chopPrefix(specNoVersion); if (gotRefLink && isNotEmpty(specNoVersion)) { if (startsWith("NM_", specNoVersion) || startsWith("NR_", specNoVersion) || startsWith("XM_", specNoVersion)) { sqlDyStringPrintf(ds, "select * from %s where mrnaAcc = '%s'", refLinkTable, specNoVersion); addRefLinks(conn, ds, &rlList); } else if (startsWith("NP_", specNoVersion) || startsWith("XP_", specNoVersion)) @@ -1693,31 +1693,31 @@ struct hash *hash = newHash(8); for (rl = rlList; rl != NULL; rl = rl->next) { char where[64]; struct genePredReader *gpr; struct genePred *gp; /* Don't return duplicate mrna accessions */ if (hashFindVal(hash, rl->mrnaAcc)) { hashAdd(hash, rl->mrnaAcc, rl); continue; } hashAdd(hash, rl->mrnaAcc, rl); - sqlSafefFrag(where, sizeof where, "name = '%s'", rl->mrnaAcc); + sqlSafef(where, sizeof where, "name = '%s'", rl->mrnaAcc); gpr = genePredReaderQuery(conn, hfs->searchTable, where); while ((gp = genePredReaderNext(gpr)) != NULL) { struct hgPos *pos = NULL; AllocVar(pos); if (table == NULL) { char desc[256]; AllocVar(table); table->name = cloneString(hfs->searchTable); if (startsWith("xeno", hfs->searchTable)) safef(desc, sizeof(desc), "Non-%s RefSeq Genes", hOrganism(db)); else safef(desc, sizeof(desc), "RefSeq Genes"); table->description = cloneString(desc); @@ -1728,57 +1728,57 @@ pos->browserName = cloneString(rl->mrnaAcc); dyStringClear(ds); dyStringPrintf(ds, "(%s) %s", rl->mrnaAcc, rl->product); pos->description = cloneString(ds->string); pos->chrom = hgOfficialChromName(db, gp->chrom); pos->chromStart = gp->txStart; pos->chromEnd = gp->txEnd; genePredFree(&gp); found = TRUE; } genePredReaderFree(&gpr); } refLinkFreeList(&rlList); freeHash(&hash); } -freeDyString(&ds); +dyStringFree(&ds); hFreeConn(&conn); return(found); } /* Lowe lab additions */ static void addTigrCmrGenes(struct sqlConnection *conn, struct dyString *query, struct tigrCmrGene **pList) /* Query database and add returned tigrCmrGenes to head of list. */ { struct sqlResult *sr = sqlGetResult(conn, query->string); char **row; while ((row = sqlNextRow(sr)) != NULL) { struct tigrCmrGene *rl = tigrCmrGeneLoad(row); slAddHead(pList, rl); } sqlFreeResult(&sr); } static void findTigrGenes(char *db, char *spec, struct hgPositions *hgp) /* Look up TIGR and Genbank genes from keyword */ { struct sqlConnection *conn = hAllocConn(db); struct sqlResult *sr = NULL; -struct dyString *ds = newDyString(256); +struct dyString *ds = dyStringNew(256); char **row; struct hgPosTable *table = NULL; struct hgPos *pos; struct bed *bed; struct tigrCmrGene *tigrList = NULL, *tigr; /* struct minGeneInfo *gbList = NULL, *gb; */ boolean gotTIGRkeys = sqlTableExists(conn, "tigrCmrORFsInfo"); if (gotTIGRkeys) { sqlDyStringPrintf(ds, "select * from tigrCmrORFsInfo where tigrCommon like '%%%s%%'", spec); addTigrCmrGenes(conn, ds, &tigrList); dyStringClear(ds); sqlDyStringPrintf(ds, "select * from tigrCmrORFsInfo where tigrMainRole like '%%%s%%'", spec); addTigrCmrGenes(conn, ds, &tigrList); @@ -1814,82 +1814,82 @@ pos->name = cloneString(tigr->name); pos->browserName = cloneString(tigr->name); dyStringClear(ds); dyStringPrintf(ds, "%s; %s; %s", tigr->tigrCommon, tigr->tigrMainRole, tigr->tigrSubRole); pos->description = cloneString(ds->string); pos->chrom = hgOfficialChromName(db, bed->chrom); pos->chromStart = bed->chromStart; pos->chromEnd = bed->chromEnd; bedFree(&bed); } sqlFreeResult(&sr); } tigrCmrGeneFreeList(&tigrList); freeHash(&hash); } -freeDyString(&ds); +dyStringFree(&ds); hFreeConn(&conn); } /* End of Lowe Lab stuff */ static boolean findGenePredPattern(char *db, char *pattern, struct hgPositions *hgp, char *tableName, struct hgPosTable *table) /* Look for position pattern in gene prediction table. */ { struct sqlConnection *conn; struct sqlResult *sr = NULL; struct dyString *query; char **row; boolean ok = FALSE; struct hgPos *pos = NULL; if (!hTableExists(db, tableName)) return FALSE; conn = hAllocConn(db); -query = newDyString(256); +query = dyStringNew(256); sqlDyStringPrintf(query, "SELECT chrom, txStart, txEnd, name FROM %s WHERE name LIKE '%s'", tableName, pattern); sr = sqlGetResult(conn, query->string); while ((row = sqlNextRow(sr)) != NULL) { if (ok == FALSE) { ok = TRUE; if (table == NULL) { AllocVar(table); - dyStringClear(query); - dyStringPrintf(query, "%s Gene Predictions", tableName); - table->description = cloneString(query->string); + struct dyString *desc = dyStringNew(256); + dyStringPrintf(desc, "%s Gene Predictions", tableName); + table->description = dyStringCannibalize(&desc); table->name = cloneString(tableName); slAddHead(&hgp->tableList, table); } } AllocVar(pos); pos->chrom = hgOfficialChromName(db, row[0]); pos->chromStart = atoi(row[1]); pos->chromEnd = atoi(row[2]); pos->name = cloneString(row[3]); pos->browserName = cloneString(row[3]); slAddHead(&table->posList, pos); } if (table != NULL) slReverse(&table->posList); -freeDyString(&query); +dyStringFree(&query); sqlFreeResult(&sr); hFreeConn(&conn); return ok; } static void addUniqYeastGene(char *db, struct hash *uniqHash, struct sqlConnection *conn, char *query, struct hgPositions *hgp, char *geneTable, struct hgPosTable **pTable) /* Execute query which returns a single row, and add genes. */ { struct sqlResult *sr = sqlGetResult(conn, query); char **row; struct hgPosTable *table = *pTable; while ((row = sqlNextRow(sr)) != NULL) @@ -1939,31 +1939,31 @@ { gotDescriptions = TRUE; sqlSafef(query, sizeof(query), "select name from sgdDescription where description like '%%%s%%'", pattern); addUniqYeastGene(db, uniqHash, conn, query, hgp, "sgdGene", &table); } hashFree(&uniqHash); /* Add descriptions to table. */ if (table != NULL) { struct hgPos *pos; for (pos = table->posList; pos != NULL; pos = pos->next) { - struct dyString *dy = newDyString(1024); + struct dyString *dy = dyStringNew(1024); if (gotNames) { sqlSafef(query, sizeof(query), "select value from sgdToName where name = '%s'", pos->name); sr = sqlGetResult(conn, query); while ((row = sqlNextRow(sr)) != NULL) dyStringPrintf(dy, "(%s) ", row[0]); sqlFreeResult(&sr); } if (gotDescriptions) { sqlSafef(query, sizeof(query), "select description from sgdDescription where name = '%s'", pos->name); sr = sqlGetResult(conn, query); @@ -2375,44 +2375,51 @@ *retFound = found; freeMem(upcTerm); return(isSpecial); } static struct slPair *getXrefTerms(char *db, struct hgFindSpec *hfs, char *term) /* Search xrefTable for xrefQuery with term. Return all matching names. */ { struct slPair *xrefList = NULL, *xrefPtr = NULL; struct sqlConnection *conn = hAllocConn(db); struct sqlResult *sr = NULL; char **row; boolean isFuzzy = sameWord(hfs->searchMethod, "fuzzy"); -// TODO wonder if we could re-work this better to get to upstream sql creation and +// TODO we could re-work this better to get to upstream sql creation and // then be able to avoid this complexity:? // hfs->refTable sometimes contains a comma-separated table list -// but we do not have control over the original sql since it is in trackDb.ra +// in trackDb.ra and hgFindSpec table. // example from human/hg19/trackDb.ra // xrefTable kgXref, ucscRetroInfo5 // xrefQuery select ucscRetroInfo5.name, spDisplayID from %s where spDisplayID like '%s%%' and kgName = kgID +// NOTE this also goes into hgFindSpec table as hti fields hfs->xrefTable and hfs->xrefQuery. +// hfs->xrefTable is sometimes a comma-separated list of fields +// xrefTable = [hgFixed.refLink, ucscRetroInfo8] + struct dyString *dy = dyStringNew(256); -dyStringAppend(dy, NOSQLINJ ""); -// in particular, if we could get to the upstream and change the first %s to %-s for the param corresponding to xrefTable, -// that would be nice. -dyStringPrintf(dy, hfs->xrefQuery, sqlCkIl(hfs->xrefTable), sqlEscapeString(term)); // keep this sqlEscape +sqlCkIl(xrefTableSafe, hfs->xrefTable) +// Replace the %s with %-s if it has not already been done in the upstream source .ra files +// it would be better to do this upstream in .ra and hgFindSpec +char *update = replaceChars(hfs->xrefQuery, " from %s ", " from %-s "); // this patches older values that still need it. +sqlDyStringPrintf(dy, update, xrefTableSafe, term); +freeMem(update); + sr = sqlGetResult(conn, dy->string); dyStringFree(&dy); while ((row = sqlNextRow(sr)) != NULL) { if (!isFuzzy || keyIsPrefixIgnoreCase(term, row[1])) { xrefPtr = slPairNew(cloneString(row[1]), cloneString(row[0])); slAddHead(&xrefList, xrefPtr); } } sqlFreeResult(&sr); hFreeConn(&conn); slReverse(&xrefList); if (xrefList == NULL && hgFindSpecSetting(hfs, "searchBoth") != NULL) xrefList = slPairNew(cloneString(""), cloneString(term)); @@ -2456,35 +2463,35 @@ if (isNotEmpty(hfs->searchDescription)) truncatef(buf, sizeof(buf), "%s", hfs->searchDescription); else safef(buf, sizeof(buf), "%s", hfs->searchTable); description = cloneString(buf); if (hgp->tableList != NULL && sameString(hgp->tableList->name, hfs->searchTable) && sameString(hgp->tableList->description, description)) table = hgp->tableList; for (tPtr = tableList; tPtr != NULL; tPtr = tPtr->next) { // we do not have control over the original sql since it comes from trackDb.ra or elsewhere? - char query[2048]; - sqlSafef(query, sizeof(query), hfs->query, tPtr->name, term); + struct dyString *query = sqlDyStringCreate(hfs->query, tPtr->name, term); if (limitResults != EXHAUSTIVE_SEARCH_REQUIRED) - sqlSafefAppend(query, sizeof(query), " limit %d", limitResults); - sr = sqlGetResult(conn, query); + sqlDyStringPrintf(query, " limit %d", limitResults); + sr = sqlGetResult(conn, dyStringContents(query)); + dyStringFree(&query); while ((row = sqlNextRow(sr)) != NULL) { if(table == NULL) { AllocVar(table); table->description = description; table->name = cloneString(hfs->searchTable); slAddHead(&hgp->tableList, table); } found = TRUE; AllocVar(pos); pos->chrom = cloneString(row[0]); pos->chromStart = atoi(row[1]); pos->chromEnd = atoi(row[2]); if (isNotEmpty(xrefTerm)) @@ -3053,31 +3060,31 @@ hgFindUsingSpec(cart, db, hfs, term, limitResults, hgp, relativeFlag, relStart, relEnd, multiTerm); } /* Lowe lab additions -- would like to replace these with specs, but * will leave in for now. */ if (!trackHubDatabase(db)) findTigrGenes(db, term, hgp); trackHubFindPos(cart, db, term, hgp); } hgFindSpecFreeList(&shortList); hgFindSpecFreeList(&longList); if (cart != NULL) { if(hgpMatchNames == NULL) - hgpMatchNames = newDyString(256); + hgpMatchNames = dyStringNew(256); dyStringClear(hgpMatchNames); int matchCount = 0; for(hgpItem = hgp; hgpItem != NULL; hgpItem = hgpItem->next) { struct hgPosTable *hpTable = NULL; for(hpTable = hgpItem->tableList; hpTable != NULL; hpTable = hpTable->next) { struct hgPos *pos = NULL; for(pos = hpTable->posList; pos != NULL; pos = pos->next) { if (limitResults != EXHAUSTIVE_SEARCH_REQUIRED && matchCount++ >= limitResults) break; dyStringPrintf(hgpMatchNames, "%s,", pos->browserName); } }