From c20a3699075369e5516af47100220dab18435a91 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Date: Mon, 19 Feb 2018 14:12:03 +1300
Subject: [PATCH] util/rfc1738: simplify and fix rfc1738_unescape()

Improvements:

* NULL is returned when the string is incorrectly formed.

* Badly formed escapes like "% b" that were accepted by sscanf() are now
  rejected.

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
---
 lib/util/rfc1738.c           | 61 +++++++++++++++++++-----------------
 selftest/knownfail.d/rfc1738 |  2 +-
 2 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/lib/util/rfc1738.c b/lib/util/rfc1738.c
index 5474ea8a4f9..4e4c6236121 100644
--- a/lib/util/rfc1738.c
+++ b/lib/util/rfc1738.c
@@ -51,6 +51,7 @@
 #include "replace.h"
 #include <talloc.h>
 #include "lib/util/samba_util.h"
+#include "lib/util/util_str_hex.h"
 
 /*
  *  RFC 1738 defines that these characters should be escaped, as well
@@ -190,37 +191,41 @@ rfc1738_escape_part(TALLOC_CTX *mem_ctx, const char *url)
 }
 
 /*
- *  rfc1738_unescape() - Converts escaped characters (%xy numbers) in
- *  given the string.  %% is a %. %ab is the 8-bit hexadecimal number "ab"
+ * rfc1738_unescape() - Converts url-escaped characters in the string.
+ *
+ * The two characters following a '%' in a string should be hex digits that
+ * describe an encoded byte. For example, "%25" is hex 0x25 or '%' in ASCII;
+ * this is the only way to include a % in the unescaped string. Any character
+ * can be escaped, including plain letters (e.g. "%61" for "a"). Anything
+ * other than 2 hex characters following the % is an error.
+ *
+ * The conversion is done in-place, which is always safe as unescapes can only
+ * shorten the string.
+ *
+ * Returns a pointer to the end of the string (that is, the '\0' byte), or
+ * NULL on error, at which point s is in an undefined state.
+ *
+ * Note that after `char *e = rfc_unescape(s)`, `strlen(s)` will not equal
+ * `e - s` if s originally contained "%00". You might want to check for this.
  */
 
 _PUBLIC_ char *rfc1738_unescape(char *s)
 {
-    char hexnum[3];
-    int i, j;			/* i is write, j is read */
-    unsigned int x;
-    for (i = j = 0; s[j]; i++, j++) {
-        s[i] = s[j];
-        if (s[i] != '%')
-            continue;
-        if (s[j + 1] == '%') {	/* %% case */
-            j++;
-            continue;
-        }
-        if (s[j + 1] && s[j + 2]) {
-            if (s[j + 1] == '0' && s[j + 2] == '0') {	/* %00 case */
-                j += 2;
-                continue;
-            }
-            hexnum[0] = s[j + 1];
-            hexnum[1] = s[j + 2];
-            hexnum[2] = '\0';
-            if (1 == sscanf(hexnum, "%x", &x)) {
-                s[i] = (char) (0x0ff & x);
-                j += 2;
-            }
-        }
-    }
-    s[i] = '\0';
+	size_t i, j;	    /* i is write, j is read */
+	uint64_t x;
+	NTSTATUS status;
+	for (i = 0, j = 0; s[j] != '\0'; i++, j++) {
+		if (s[j] == '%') {
+			status = read_hex_bytes(&s[j + 1], 2, &x);
+			if (! NT_STATUS_IS_OK(status)) {
+				return NULL;
+			}
+			j += 2; /* OK; read_hex_bytes() has checked ahead */
+			s[i] = (unsigned char)x;
+		} else {
+			s[i] = s[j];
+		}
+	}
+	s[i] = '\0';
 	return s + i;
 }
diff --git a/selftest/knownfail.d/rfc1738 b/selftest/knownfail.d/rfc1738
index 6ff17bdc51f..3f5497ee413 100644
--- a/selftest/knownfail.d/rfc1738
+++ b/selftest/knownfail.d/rfc1738
@@ -1 +1 @@
-^samba.unittests.rfc1738
+^samba.unittests.rfc1738.test_escape
-- 
2.34.1