c01de718b2fb8e7706a0fede0e5ea4149ac6f378 braney Mon Aug 31 13:48:13 2026 -0700 htmlSanitize: six fixes from a second review of it, refs #38126 Filter the style attribute on the text a browser will see. A browser turns a character reference into the character it names before the CSS parser runs, so a value spelled url( reached the page as url( and walked past the check that is there to stop it. The value is decoded before the check now, and the author is told which property lost its value. Say something when we drop the rest of the page. A tag that never ends, most often an attribute value whose quote is never closed, threw away everything after it and reported nothing, so hubCheck stayed quiet about it. The depth cap did the same. Write a less than sign that starts no tag as <. A browser reads text like "', which could eat a closing tag we added. Write an attribute once. A browser keeps the first of a repeated attribute and drops the rest. We checked the first and then printed them all, which was only correct because of that rule. Do not stack a second descPage- prefix on an id we already renamed, and do not add a second noopener noreferrer to a rel that already has one. hgCustom hands the text we returned back to us when a custom track is edited and saved again, so both of these grew a little more on every save. The test now re-runs the filter over its own output and prints any case that changes, so a transform that compounds shows up in the diff. diff --git src/lib/htmlSanitize.c src/lib/htmlSanitize.c index 5281b97a5c9..6d236508770 100644 --- src/lib/htmlSanitize.c +++ src/lib/htmlSanitize.c @@ -106,30 +106,35 @@ "line-height letter-spacing text-decoration text-transform text-indent " "list-style list-style-type list-style-position " "float clear display overflow overflow-x overflow-y opacity"; /* URL schemes allowed in href and src. A URL with no scheme at all is allowed too. */ static char *urlSchemes = "http https mailto ftp"; /* Hosts an iframe may point at. */ static char *videoHosts = "www.youtube.com youtube.com www.youtube-nocookie.com youtube-nocookie.com youtu.be " "player.vimeo.com vimeo.com"; /* Nesting past this depth is not a document, it is a way to make us emit a huge page. */ #define maxNestDepth 256 +/* The longest attribute name we look at, and the most attributes we remember writing on one + * tag. Every name on the allowlist is well inside both. */ +#define maxAttrName 128 +#define maxTagAttrs 32 + static struct hash *keepHash = NULL, *killHash = NULL, *voidHash = NULL, *rawTextHash = NULL; static struct hash *silentKillHash = NULL; static struct hash *attrHash = NULL, *stylePropHash = NULL, *schemeHash = NULL, *videoHostHash = NULL; static struct hash *hashOfWords(char *words, int sizePow2) /* Return a hash holding each space separated word in words. */ { struct hash *hash = hashNew(sizePow2); char *dupe = cloneString(words); char *word, *s = dupe; while ((word = nextWord(&s)) != NULL) hashAdd(hash, word, NULL); freeMem(dupe); return hash; } @@ -336,31 +341,31 @@ ++s; } else { *retVal = s; while (s < tagEnd && !isspace((unsigned char)*s)) ++s; *retValLen = s - *retVal; } return s; } static void appendEscaped(struct dyString *dy, char *s) /* Append s as an attribute value, hiding the characters that could end the attribute or * start a tag. Ampersands are left alone so that entities the author wrote stay as - * they are. */ + * they are, and so that running this over its own output changes nothing. */ { for (; *s != 0; ++s) { switch (*s) { case '"': dyStringAppend(dy, """); break; case '<': dyStringAppend(dy, "<"); break; case '>': dyStringAppend(dy, ">"); break; default: @@ -540,153 +545,190 @@ char *decl = dupe; while (decl != NULL && *decl != 0) { char *next = strchr(decl, ';'); if (next != NULL) *next++ = 0; char *colon = strchr(decl, ':'); if (colon != NULL) { *colon = 0; char *prop = trimSpaces(decl); char *value = trimSpaces(colon+1); tolowers(prop); if (isNotEmpty(prop) && isNotEmpty(value)) { - char *lower = cloneString(value); + /* Look at the text a browser will see, not the text the author wrote. A + * browser turns a character reference into the character it names before the + * CSS parser runs, so url( would otherwise walk past the check below. + * Only the numeric form needs decoding here. A named reference has to end in + * a semicolon, apart from a legacy handful that all name Latin-1 punctuation, + * and a semicolon has already ended the declaration before we get here. */ + char *lower = decodeNumericRefs(value); tolowers(lower); if (hashLookup(stylePropHash, prop) == NULL) ; /* not a property we print, and nothing to explain */ else if (stringIn("url(", lower) != NULL || stringIn("expression", lower) != NULL || strchr(lower, '\\') != NULL) noteRemoved(san, "removed the value of the style property %s", prop); else dyStringPrintf(out, "%s:%s;", prop, value); freeMem(lower); } } decl = next; } freeMem(dupe); if (out->stringSize == 0) { dyStringFree(&out); return NULL; } return dyStringCannibalize(&out); } +static char *skipIdPrefix(char *name) +/* Return name past a prefix we put there ourselves. Adding a second one would work, but + * the filter runs over its own output whenever a custom track is edited and saved again, + * and a prefix that stacks grows the name a little more on every save. */ +{ +if (startsWith(htmlSanitizeIdPrefix, name)) + return name + strlen(htmlSanitizeIdPrefix); +return name; +} + +static boolean alreadyWritten(char written[][maxAttrName+1], int *pCount, char *attr) +/* Has attr already gone onto this tag? If not, remember it and return FALSE. A browser + * keeps the first of a repeated attribute and drops the rest, so we write the first and drop + * the rest too, rather than hand out a tag with two of something and lean on that rule. */ +{ +int i; +for (i = 0; i < *pCount; ++i) + { + if (sameString(written[i], attr)) + return TRUE; + } +if (*pCount < maxTagAttrs) + safecpy(written[(*pCount)++], maxAttrName+1, attr); +return FALSE; +} + static void writeAttributes(struct sanitizer *san, char *element, char *attrText, char *tagEnd) /* Write the attributes of element that we allow, from the text between attrText and tagEnd. */ { boolean isAnchor = sameString(element, "a"); boolean isFrame = sameString(element, "iframe"); boolean hasTarget = FALSE; char *relValue = NULL; +char written[maxTagAttrs][maxAttrName+1]; +int writtenCount = 0; char *s = attrText; char *name, *val; int nameLen, valLen; while ((s = nextAttribute(s, tagEnd, &name, &nameLen, &val, &valLen)) != NULL) { - if (nameLen == 0 || nameLen > 128) + if (nameLen == 0 || nameLen > maxAttrName) continue; - char attr[129]; + char attr[maxAttrName+1]; memcpy(attr, name, nameLen); attr[nameLen] = 0; tolowers(attr); char key[256]; safef(key, sizeof key, "%s.%s", element, attr); if (hashLookup(attrHash, key) == NULL) { safef(key, sizeof key, "*.%s", attr); if (hashLookup(attrHash, key) == NULL) { if (startsWith("on", attr) && allNameChars(attr)) noteRemoved(san, "removed the attribute %s", attr); continue; } } + if (alreadyWritten(written, &writtenCount, attr)) + continue; char *value = cloneStringZ(val == NULL ? "" : val, valLen); if (sameString(attr, "style")) { char *style = filterStyle(value, san); if (style != NULL) { dyStringAppend(san->out, " style=\""); appendEscaped(san->out, style); dyStringAppendC(san->out, '"'); freeMem(style); } } else if (sameString(attr, "href") || sameString(attr, "src")) { if (urlOk(value, san)) { char *fragment = NULL; if (sameString(attr, "href")) { char *trimmed = skipLeadingSpaces(value); if (trimmed[0] == '#' && trimmed[1] != 0) fragment = trimmed + 1; /* a link to a name on this same page */ } dyStringPrintf(san->out, " %s=\"", attr); if (fragment != NULL) { /* The name it points at is being renamed, so rename this to match. */ dyStringAppend(san->out, "#" htmlSanitizeIdPrefix); - appendEscaped(san->out, fragment); + appendEscaped(san->out, skipIdPrefix(fragment)); } else appendEscaped(san->out, value); dyStringAppendC(san->out, '"'); } } else if (sameString(attr, "id") || (isAnchor && sameString(attr, "name"))) { /* An id here lands in a page of ours, next to ids our own JavaScript looks up, * and it also becomes a property of that name on window. A prefix keeps the two * sets apart. An anchor name does both of those things too. */ if (isNotEmpty(value)) { dyStringPrintf(san->out, " %s=\"%s", attr, htmlSanitizeIdPrefix); - appendEscaped(san->out, value); + appendEscaped(san->out, skipIdPrefix(value)); dyStringAppendC(san->out, '"'); } } else if (isAnchor && sameString(attr, "rel")) - { - freez(&relValue); - relValue = cloneString(value); - } + relValue = cloneString(value); /* a repeat of it never reaches here */ else { if (isAnchor && sameString(attr, "target")) hasTarget = TRUE; dyStringPrintf(san->out, " %s=\"", attr); appendEscaped(san->out, value); dyStringAppendC(san->out, '"'); } freeMem(value); } if (isAnchor && (hasTarget || relValue != NULL)) { /* A link that opens a new window hands that window a handle back to ours unless we - * say otherwise. */ + * say otherwise. A noopener or noreferrer the text already carries is dropped first, + * so that running this over its own output does not stack a second pair on. */ dyStringAppend(san->out, " rel=\""); - if (relValue != NULL) + char *word, *rest = relValue; + while (rest != NULL && (word = nextWord(&rest)) != NULL) { - appendEscaped(san->out, relValue); + if (sameWord(word, "noopener") || sameWord(word, "noreferrer")) + continue; + appendEscaped(san->out, word); dyStringAppendC(san->out, ' '); } dyStringAppend(san->out, "noopener noreferrer\""); } if (isFrame) dyStringAppend(san->out, " sandbox=\"allow-scripts allow-same-origin allow-popups" " allow-presentation\""); freez(&relValue); } static char *attributeValue(char *attrText, char *tagEnd, char *wanted) /* Return a copy of the value of the named attribute, or NULL if the tag has no such * attribute. */ { char *s = attrText; @@ -746,37 +788,44 @@ { char *end = stringIn("-->", s+4); s = (end == NULL ? s + strlen(s) : end+3); continue; } if (s[1] == '!' || s[1] == '?') { char *end = strchr(s, '>'); s = (end == NULL ? s + strlen(s) : end+1); continue; } boolean closing = (s[1] == '/'); char *nameStart = s + (closing ? 2 : 1); if (!isalpha((unsigned char)*nameStart)) { - dyStringAppendC(san->out, '<'); + /* Not the start of a tag, so it is text. Spelled out, because a browser reads + * something like "'. */ + dyStringAppend(san->out, "<"); s += 1; continue; } char *tagEnd = findTagEnd(nameStart); if (tagEnd == NULL) - break; /* tag with no end, drop what is left */ + { + /* Most often an attribute value whose quote is never closed. Everything after it + * reads as part of that value, so there is nothing left we can trust. */ + noteRemoved(san, "stopped at a tag that never ends, and printed nothing after it"); + break; + } char name[64]; char *attrText = tagName(nameStart, name, sizeof name); s = tagEnd + 1; if (closing) { if (hashLookup(keepHash, name) != NULL && hashLookup(voidHash, name) == NULL) closeThrough(san, name); continue; } boolean isVoid = (hashLookup(voidHash, name) != NULL); boolean kill = (hashLookup(killHash, name) != NULL); boolean noted = FALSE; if (!kill && sameString(name, "iframe")) { char *src = attributeValue(attrText, tagEnd, "src"); @@ -804,31 +853,34 @@ if (afterClose == NULL) hashAdd(san->exhausted, name, NULL); } if (afterClose != NULL) s = afterClose; else if (rawText) s += strlen(s); /* never closed, and its content is not for reading */ } if (!noted && hashLookup(silentKillHash, name) == NULL) noteRemoved(san, "removed the %s element and everything inside it", name); continue; } if (hashLookup(keepHash, name) == NULL) continue; /* tag goes, text inside it stays */ if (!isVoid && san->depth >= maxNestDepth) + { + noteRemoved(san, "dropped tags nested more than %d deep", maxNestDepth); continue; + } dyStringPrintf(san->out, "<%s", name); writeAttributes(san, name, attrText, tagEnd); dyStringAppendC(san->out, '>'); if (!isVoid) { /* A trailing slash does not close an element like this one, whatever the author * meant by it, so remember it as open. Anything still open at the end is closed * for us, which stops a page ending up inside a hub's div. */ slNameAddHead(&san->openStack, name); san->depth += 1; } } while (san->openStack != NULL) { struct slName *top = slPopHead(&san->openStack);