dbb0850c7935dec65d3394ef1ddcdc10dafac5cf braney Tue Aug 18 10:03:38 2026 -0700 cheapcgi: parse %hh escapes directly instead of with sscanf, refs #37262 cgiDecode and cgiDecodeFull read each %hh escape with sscanf(in, "%2x", &code). glibc builds a stream over the whole remaining string on every sscanf call, so each call scans to the terminating null. That makes the cost of a decode quadratic in the length of one variable's value. The cost is real on data we already have. A saved session in hgcentraltest holds a single 699 KB hgFind.matches value with 58,930 escapes; decoding it takes 0.29 s of CPU, and cart.c loadHash does it on every load of that session. The database cart has no size cap, so this is not bounded by the 1 MB limit on request input that went in for #37452. At that 1 MB limit a single request still costs over 2 s. Reading the two hex digits directly is a few hundred times faster (430x on the 699 KB value) and never walks past them. Behavior is unchanged for well-formed input: verified byte-identical over the top 200 carts of namedSessionDb, sessionDb and userDb (2.4 million values, 14.7 MB), over an exhaustive sweep of every "%" plus two arbitrary bytes, and over cgiEncode/cgiDecode round trips of all 256 byte values. Decoding now differs only where a "%" is not followed by two hex digits, which nothing legitimate produces - none of the 659,624 escapes in those carts are malformed. The old code was worse there anyway: sscanf skips leading whitespace, so "% 0Z" decoded to a null byte in the middle of the value and silently truncated it. Malformed escapes now yield '?' like other bad input. Also removes the FAST_CGI_DECODE ifdef added earlier on this ticket. It never touched cgiDecode, so it does not describe anything now that the real cost is fixed. Its per-variable caps are superseded by the total input cap from #37452, which aborts with a message rather than dropping a variable silently, and cgiParseNext's variant silently skipped oversized variables for the ENCODE/CIRM tag tools that are its only callers. diff --git src/lib/cheapcgi.c src/lib/cheapcgi.c index 2decd97f4de..8adfa653083 100644 --- src/lib/cheapcgi.c +++ src/lib/cheapcgi.c @@ -6,33 +6,30 @@ #include "common.h" #include "hash.h" #include "cheapcgi.h" #include "portable.h" #include "linefile.h" #include "errAbort.h" #include "filePath.h" #include "htmshell.h" #include "dystring.h" #ifndef GBROWSE #include "mime.h" #endif /* GBROWSE */ #include <signal.h> -// FAST_CGI_DECODE can be defined in cheapcgi.h to try a faster decode process that -// also limits variable/value lengths and applies encoding to variable names - //============ javascript inline-separation routines =============== // One of the main services that CSP (Content Security Policy) provides // is protecting from reflected and stored XSS attacks by disabling all inline javacript, // both in script tags, and in inline event handlers. The separated javascript // can be either added back to the end of the html page with a nonce or sha hashid, // or it can be saved to a temp file in trash and then included as a non-inline, off-page .js. struct dyString *jsInlineLines = NULL; void jsInlineInit() /* init if needed */ { if (!jsInlineLines) { @@ -814,49 +811,35 @@ namePt = str; while (isNotEmpty(namePt)) { dataPt = strchr(namePt, '='); if (dataPt == NULL) errAbort("Mangled Cookie input string: no = in '%s' (offset %d in complete cookie string: '%s')", namePt, (int)(namePt - str), getenv("HTTP_COOKIE")); *dataPt++ = 0; nextNamePt = strchr(dataPt, ';'); if (nextNamePt != NULL) { *nextNamePt++ = 0; if (*nextNamePt == ' ') nextNamePt++; } -#ifndef FAST_CGI_DECODE cgiDecode(dataPt,dataPt,strlen(dataPt)); AllocVar(el); el->val = dataPt; slAddHead(&list, el); hashAddSaveName(hash, namePt, el, &el->name); -#else - int dataSize = strlen(dataPt); - int nameSize = strlen(namePt); - if ((dataSize <= CGI_VAR_SIZE_LIMIT) && (nameSize < CGI_VAR_NAME_LIMIT)) - { - cgiDecode(namePt,namePt,nameSize); - cgiDecode(dataPt,dataPt,dataSize); - AllocVar(el); - el->val = dataPt; - slAddHead(&list, el); - hashAddSaveName(hash, namePt, el, &el->name); - } -#endif // FAST_CGI_DECODE namePt = nextNamePt; } haveCookiesHash = TRUE; slReverse(&list); *retList = list; *retHash = hash; } char *findCookieData(char *varName) /* Get the string associated with varName from the cookie string. */ { struct hashEl *hel; char *firstResult; @@ -955,93 +938,53 @@ next = el->next; cgiDictionaryFree(&el); } *pList = NULL; } boolean cgiParseNext(char **pInput, char **retVar, char **retVal) /* Parse out next var/val in a var=val&var=val... cgi formatted string * This will insert zeroes and other things into string. * Usage: * char *pt = cgiStringStart; * char *var, *val * while (cgiParseNext(&pt, &var, &val)) * printf("%s\t%s\n", var, val); */ { -#ifndef FAST_CGI_DECODE char *var = *pInput; if (var == NULL || var[0] == 0) return FALSE; char *val = strchr(var, '='); if (val == NULL) errAbort("Mangled CGI input string %s", var); *val++ = 0; char *end = strchr(val, '&'); if (end == NULL) end = strchr(val, ';'); // For DAS if (end == NULL) { end = val + strlen(val); *pInput = NULL; } else { *pInput = end+1; *end = 0; } *retVar = var; *retVal = val; cgiDecode(val,val,end-val); -#else -char *val = NULL; -char *var = NULL; -int varLength = 0; -int valLength = 0; -do - { - var = *pInput; - if (var == NULL || var[0] == 0) - { - *retVar = *retVal = NULL; - return FALSE; - } - val = strchr(var, '='); - if (val == NULL || var == val) - errAbort("Mangled CGI input string %s", var); - varLength = val-var; - *val++ = 0; - char *end = strchr(val, '&'); - if (end == NULL) - end = strchr(val, ';'); // For DAS - if (end == NULL) - { - end = val + strlen(val); - *pInput = NULL; - } - else - { - *pInput = end+1; - *end = 0; - } - *retVar = var; - *retVal = val; - valLength = end-val; - } while ((varLength > CGI_VAR_NAME_LIMIT) || (valLength > CGI_VAR_SIZE_LIMIT)); - // skip variables that are too big -cgiDecode(var,var,varLength); -cgiDecode(val,val,valLength); -#endif // FAST_CGI_DECODE return TRUE; } void cgiSetMaxLogLen(int l) /* set the size of variable values that are dumped to stderr. Default is 0, which means no logging */ { logCgiVarMaxLen = l; } void cgiParseInputAbort(char *input, struct hash **retHash, struct cgiVar **retList) /* Parse cgi-style input into a hash table and list. This will alter * the input data. The hash table will contain references back * into input, so please don't free input until you're done with * the hash. Prints message aborts if there's an error. @@ -1065,48 +1008,36 @@ dataPt = strchr(namePt, '='); if (dataPt == NULL) { errAbort("Mangled CGI input string %s", namePt); } *dataPt++ = 0; nextNamePt = strchr(dataPt, '&'); if (nextNamePt == NULL) nextNamePt = strchr(dataPt, ';'); /* Accomodate DAS. */ if (nextNamePt != NULL) *nextNamePt++ = 0; if (logMsg && dataPt && strlen(dataPt) < logCgiVarMaxLen) dyStringPrintf(logMsg, "%s=%s ", namePt, dataPt); // if dataPt is empty string, still print it, could be important -#ifndef FAST_CGI_DECODE - cgiDecode(namePt,namePt,strlen(namePt)); /* for unusual ct names */ - cgiDecode(dataPt,dataPt,strlen(dataPt)); - AllocVar(el); - el->val = dataPt; - slAddHead(&list, el); - hashAddSaveName(hash, namePt, el, &el->name); -#else - if ((strlen(namePt) < CGI_VAR_NAME_LIMIT) && (strlen(dataPt) < CGI_VAR_SIZE_LIMIT)) - { cgiDecode(namePt,namePt,strlen(namePt)); /* for unusual ct names */ cgiDecode(dataPt,dataPt,strlen(dataPt)); AllocVar(el); el->val = dataPt; slAddHead(&list, el); hashAddSaveName(hash, namePt, el, &el->name); - } -#endif // FAST_CGI_DECODE namePt = nextNamePt; } if (logMsg) { char *logStr = dyStringCannibalize(&logMsg); fprintf(stderr, "CGIVARS %s\n", logStr); freez(&logStr); } slReverse(&list); *retList = list; *retHash = hash; @@ -1404,76 +1335,108 @@ * It should not be used in the rest of the URL. * So in the query string part of a URL, do use cgiEncode/cgiDecode. * And in the rest of the URL, use cgiEncodeFUll/cgiDecodeFull * which do not code space as plus. * Since FTP does not use URLs with query parameters, use the Full version. */ /* SECURITY (refs #38051): 0x01 is the in-band marker that sqlSafef (jksql.c) and * htmlSafef (htmshell.c) use to delimit the values they must escape. A request * value carrying this byte can forge a delimiter pair and smuggle unescaped text * into a query or into page output, so drop it here as it is decoded. It is * never legitimate in a request. Only 0x01 - tab, newline and CR are left alone, * since those are legitimate in custom-track textarea uploads. */ #define CGI_ESCAPE_MARKER 0x01 +static int cgiHexDigit(char c) +/* Return the value 0-15 of one hexadecimal digit, or -1 if c is not one. */ +{ +if (c >= '0' && c <= '9') + return c - '0'; +if (c >= 'a' && c <= 'f') + return c - 'a' + 10; +if (c >= 'A' && c <= 'F') + return c - 'A' + 10; +return -1; +} + +static int cgiEscapedByte(char *in, int avail) +/* Decode the two hex digits of a %hh escape, where in points just past the '%' + * and avail is the number of input bytes remaining there. Return '?' if the + * escape is malformed, matching what the old sscanf-based code did for input + * with no leading hex digit at all. + * + * PERFORMANCE (refs #37262): this used to be sscanf(in, "%2x", &code). glibc + * builds a stream over the whole remaining string on each sscanf call, so it + * scans to the terminating null every time. That made decoding cost time + * quadratic in the length of the value, and a value is as long as a cart + * variable: 0.3s of CPU on every load of a session we already have on disk, + * over 2s for a request at the input size limit. Reading the two digits + * directly is a few hundred times faster and never walks past them. */ +{ +if (avail >= 2) + { + int hi = cgiHexDigit(in[0]); + int lo = cgiHexDigit(in[1]); + if (hi >= 0 && lo >= 0) + return (hi << 4) + lo; + } +return '?'; +} + void cgiDecode(char *in, char *out, int inLength) /* Decode from cgi pluses-for-spaces format to normal. * Out will be a little shorter than in typically, and * can be the same buffer. */ { char c; int i; for (i=0; i<inLength;++i) { c = *in++; if (c == '+') *out++ = ' '; else if (c == '%') { - int code; - if (sscanf(in, "%2x", &code) != 1) - code = '?'; + int code = cgiEscapedByte(in, inLength - i - 1); in += 2; i += 2; if (code == CGI_ESCAPE_MARKER) continue; // drop our reserved escape marker *out++ = code; } else if (c == CGI_ESCAPE_MARKER) continue; // drop our reserved escape marker else *out++ = c; } *out++ = 0; } void cgiDecodeFull(char *in, char *out, int inLength) /* Out will be a cgi-decoded version of in (no space from plus!). * Out will be a little shorter than in typically, and * can be the same buffer. */ { char c; int i; for (i=0; i<inLength;++i) { c = *in++; if (c == '%') { - int code; - if (sscanf(in, "%2x", &code) != 1) - code = '?'; + int code = cgiEscapedByte(in, inLength - i - 1); in += 2; i += 2; if (code == CGI_ESCAPE_MARKER) continue; // drop our reserved escape marker *out++ = code; } else if (c == CGI_ESCAPE_MARKER) continue; // drop our reserved escape marker else *out++ = c; } *out++ = 0; } char *cgiEncode(char *inString)