Security fix:

After decoding numeric (\N) and one-character (\<, \> etc.)
character escape sequences, do not forget to HTML-encode the
resulting ASCII character.  Malicious manuals were able to smuggle
XSS content by roff-escaping the HTML-special characters they need.
That's a classic bug type in many web applications, actually...  :-(

Found myself while auditing the HTML formatter for safe output handling.

2 files changed, 49 insertions, 27 deletions

diff --git a/chars.c b/chars.c
index 1e9a186e..aee3fdf5 100644
--- a/chars.c
+++ b/chars.c
@@ -127,7 +127,18 @@ mchars_num2uc(const char *p, size_t sz)
 
 	if ((i = mandoc_strntoi(p, sz, 16)) < 0)
 		return('\0');
-	/* FIXME: make sure we're not in a bogus range. */
+
+	/*
+	 * Security warning:
+	 * Never extend the range of accepted characters
+	 * to overlap with the ASCII range, 0x00-0x7F
+	 * without re-auditing the callers of this function.
+	 * Some callers might relay on the fact that we never
+	 * return ASCII characters for their escaping decisions.
+	 *
+	 * XXX Code is missing here to exclude bogus ranges.
+	 */
+
 	return(i > 0x80 && i <= 0x10FFFF ? i : '\0');
 }
 
diff --git a/html.c b/html.c
index d4f2a057..13d1902c 100644
--- a/html.c
+++ b/html.c
@@ -110,6 +110,7 @@ static	const char	*const roffscales[SCALE_MAX] = {
 
 static	void	 bufncat(struct html *, const char *, size_t);
 static	void	 print_ctag(struct html *, enum htmltag);
+static	int	 print_escape(char);
 static	int	 print_encode(struct html *, const char *, int);
 static	void	 print_metaf(struct html *, enum mandoc_esc);
 static	void	 print_attr(struct html *, const char *, const char *);
@@ -324,6 +325,37 @@ html_strlen(const char *cp)
 }
 
 static int
+print_escape(char c)
+{
+
+	switch (c) {
+	case '<':
+		printf("&lt;");
+		break;
+	case '>':
+		printf("&gt;");
+		break;
+	case '&':
+		printf("&amp;");
+		break;
+	case '"':
+		printf("&quot;");
+		break;
+	case ASCII_NBRSP:
+		putchar('-');
+		break;
+	case ASCII_HYPH:
+		putchar('-');
+		/* FALLTHROUGH */
+	case ASCII_BREAK:
+		break;
+	default:
+		return(0);
+	}
+	return(1);
+}
+
+static int
 print_encode(struct html *h, const char *p, int norecurse)
 {
 	size_t		 sz;
@@ -350,30 +382,8 @@ print_encode(struct html *h, const char *p, int norecurse)
 		if ('\0' == *p)
 			break;
 
-		switch (*p++) {
-		case '<':
-			printf("&lt;");
-			continue;
-		case '>':
-			printf("&gt;");
-			continue;
-		case '&':
-			printf("&amp;");
-			continue;
-		case '"':
-			printf("&quot;");
-			continue;
-		case ASCII_NBRSP:
-			putchar('-');
+		if (print_escape(*p++))
 			continue;
-		case ASCII_HYPH:
-			putchar('-');
-			/* FALLTHROUGH */
-		case ASCII_BREAK:
-			continue;
-		default:
-			break;
-		}
 
 		esc = mandoc_escape(&p, &seq, &len);
 		if (ESCAPE_ERROR == esc)
@@ -408,21 +418,22 @@ print_encode(struct html *h, const char *p, int norecurse)
 
 		switch (esc) {
 		case ESCAPE_UNICODE:
-			/* Skip passed "u" header. */
+			/* Skip past "u" header. */
 			c = mchars_num2uc(seq + 1, len - 1);
 			if ('\0' != c)
 				printf("&#x%x;", c);
 			break;
 		case ESCAPE_NUMBERED:
 			c = mchars_num2char(seq, len);
-			if ('\0' != c)
+			if ( ! ('\0' == c || print_escape(c)))
 				putchar(c);
 			break;
 		case ESCAPE_SPECIAL:
 			c = mchars_spec2cp(h->symtab, seq, len);
 			if (c > 0)
 				printf("&#%d;", c);
-			else if (-1 == c && 1 == len)
+			else if (-1 == c && 1 == len &&
+			    !print_escape(*seq))
 				putchar((int)*seq);
 			break;
 		case ESCAPE_NOSPACE: