12a0dc62bdfc01eb3f94cc675657ab21b893c67a
braney
  Tue Aug 11 09:22:06 2026 -0700
lib: reject the reserved 0x01 escape marker in sqlSafef, htmlSafef and cgiDecode, refs #38051

sqlSafef does not escape values as it formats them. It formats first, then
escapes in a second pass. To mark which spans need escaping it wraps each
quoted %s in an in-band marker byte, escPunc = 0x01. sqlEscapeAllStrings then
pairs up the 0x01 bytes, escapes the text between a pair, and copies text
outside a pair raw.

The marker shares the byte stream with the data. A value carrying its own pair
of 0x01 bytes forges an extra pair and shifts the pairing, so part of the value
is copied raw instead of escaped. For a value of "x\x01' OR '1'='1\x01x" the
query came out as name='x' OR '1'='1x' with live quotes. cgiDecode turns %01
into a literal 0x01 and filtered nothing, so this was reachable from a plain
GET parameter with no login. htmshell.c uses the same trick for htmlSafef, where
the payoff is XSS instead of SQL.

Two guards:

vaSqlSafefNoAbort now rejects a quoted %s value that already contains escPunc.
vaHtmlSafefNoAbort counts the markers after formatting and requires exactly two
per escaped string. Both use errAbort rather than sqlCheckError or the noAbort
return. sqlCheckError honors noSqlInj.level and can be downgraded to warn and
then return, which would leave the injection live, and vaHtmlDyStringPrintf
reads a negative return as "buffer too small" and would retry forever. A raw
0x01 is never legitimate in either place, so there is no false-positive cost.

cgiDecode and cgiDecodeFull now drop 0x01, both percent-encoded and raw, so the
marker never enters a cart or CGI string. Only 0x01. Tab, newline and CR are
left alone because custom-track textarea uploads need them. Multipart uploads do
not pass through cgiDecode, so the sqlSafef and htmlSafef guards are what cover
that path.

Pre-escaped %-s arguments are not checked individually. They come either from
sqlSafef output, which now aborts before it can produce a marker, or from
sqlCheckIdentifier and sqlCkIl, which whitelist from a disallow-all table that
never permits 0x01.

Removing the in-band marker altogether is the durable fix and is tracked
separately, since it rewrites a hot path and needs its own performance testing.

diff --git src/hg/lib/jksql.c src/hg/lib/jksql.c
index 7b576592618..66f16733799 100644
--- src/hg/lib/jksql.c
+++ src/hg/lib/jksql.c
@@ -4241,31 +4241,39 @@
 		    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);
+			    /* SECURITY (refs #38051): the value must not contain our in-band
+			     * escape marker.  Two of them forge an extra delimiter pair, and
+			     * sqlEscapeAllStrings then copies the text between them raw
+			     * instead of escaping it, smuggling live quotes into the query.
+			     * A raw 0x01 is never legitimate in a SQL string value. */
+			    int sLen = strlen(s);
+			    if (memchr(s, escPunc, sLen) != NULL)
+				errAbort("Illegal control character in SQL string value.");
+			    escStringsSize += sLen;
 			    }
 			}
 		    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;