omega GET query parsing defects and fix

[Charles]

Without this patch:

• if the QUERY_STRING ends with a '%', the parsing routine will overrun the QUERY_STRING (if it is followed in memory by only a single NUL byte).
• omega stops parsing GET query parameters if there are two consecutive ampersands in QUERY_STRING.
• omega tries to parse anything following a percent sign. Other implementations do not try to parse malformed escape sequences.

The patch is also attached as a file

---

diff -Naur xapian-omega-1.2.7-original/cgiparam.cc xapian-omega-1.2.7/cgiparam.cc
--- xapian-omega-1.2.7-original/cgiparam.cc	2011-08-10 09:49:12.000000000 +0300
+++ xapian-omega-1.2.7/cgiparam.cc	2011-11-13 08:10:15.021566998 +0200
@@ -180,20 +180,28 @@
 	while (1) {
 	    ch = *q_str++;
 	    if (ch == '\0' || ch == '&') {
-		if (name.empty()) return; // end on blank line
-		add_param(name, val);
+		if (!name.empty()) add_param(name, val);
+		if (ch == '\0')
+			return;
 		break;
 	    }
 	    char orig_ch = ch;
 	    if (ch == '+')
 		ch = ' ';
-	    else if (ch == '%') {
-		int c = *q_str++;
-		ch = (c & 0xf) + ((c & 64) ? 9 : 0);
-		if (c) c = *q_str++;
-		ch = ch << 4;
-		ch |= (c & 0xf) + ((c & 64) ? 9 : 0);
-		if (!c) return; // unfinished % code
+	    else if (ch == '%' &&
+		     ((q_str[0] >= '0' && q_str[0] <= '9') ||
+		      (q_str[0] >= 'A' && q_str[0] <= 'F') ||
+		      (q_str[0] >= 'a' && q_str[0] <= 'f')) &&
+		     ((q_str[1] >= '0' && q_str[1] <= '9') ||
+		      (q_str[1] >= 'A' && q_str[1] <= 'F') ||
+		      (q_str[1] >= 'a' && q_str[1] <= 'f'))) {
+		const int c1 = q_str[0], c2 = q_str[1];
+		int c;
+		c = ( (c1 & 0xf) + ((c1 & 64) ? 9 : 0) ) << 4
+		  + ( (c2 & 0xf) + ((c2 & 64) ? 9 : 0) );
+		q_str += 2;
+		if (!c)
+		    continue;
 	    }
 	    if (had_equals) {
 		val += char(ch);

[Charles]

Generating command and patch

[Olly Betts]

Thanks.

We should fix these for 1.2.8.

[Olly Betts]

Actually, I don't believe the current code can read beyond the trailing nul byte, since q_ptr is only advanced and read if c is non-zero.

So I wonder if it's better to fix this in trunk only - fixing the other two quirks would be good overall, but doesn't change anything for normal uses (browsers won't build these problematic query strings), yet might change behaviour for people building their own query URLs.

[Charles]

Replying to olly:

Actually, I don't believe the current code can read beyond the trailing nul byte, since q_ptr is only advanced and read if c is non-zero.

Correct and sorry for the misinformation.

We stumbled across the real defect when using Apache rewriting to edit the query string and accidentally produced consecutive ampersands. Unintentional but should have been valid according to the CGI RFCs.

[Olly Betts]

There's similar code for handling POST too. I went for writing new code instead, and made it a template so we only need to maintain one copy. Committed to trunk in r16150, along with unit tests for the cases mentioned in the report and some other simple cases, and also unit tests for URL encoding.

Backported for 1.2.8 as r16151.