[PATCH] mailinfo: don't decode invalid =XY quoted-printable sequences

[PATCH] mailinfo: don't decode invalid =XY quoted-printable sequences

[René Scharfe]

Decode =XY in quoted-printable segments only if X and Y are hexadecimal
digits, otherwise just copy them.  That's at least better than
interpreting negative results from hexval() as a character.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 mailinfo.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index f2387a3267..3d8ac07399 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -367,11 +367,16 @@ static struct strbuf *decode_q_segment(const struct strbuf *q_seg, int rfc2047)
 
 	while ((c = *in++) != 0) {
 		if (c == '=') {
-			int d = *in++;
+			int ch, d = *in;
 			if (d == '\n' || !d)
 				break; /* drop trailing newline */
-			strbuf_addch(out, (hexval(d) << 4) | hexval(*in++));
-			continue;
+			ch = hex2chr(in);
+			if (ch >= 0) {
+				strbuf_addch(out, ch);
+				in += 2;
+				continue;
+			}
+			/* garbage -- fall through */
 		}
 		if (rfc2047 && c == '_') /* rfc2047 4.2 (2) */
 			c = 0x20;
-- 
2.14.1

Re: [PATCH] mailinfo: don't decode invalid =XY quoted-printable sequences

[René Scharfe]

Am 23.09.2017 um 20:04 schrieb René Scharfe:
> Decode =XY in quoted-printable segments only if X and Y are hexadecimal
> digits, otherwise just copy them.  That's at least better than
> interpreting negative results from hexval() as a character.

Forgot to add:

Reported-by: Jeff King <peff@peff.net>

René

Re: [PATCH] mailinfo: don't decode invalid =XY quoted-printable sequences

[Jeff King]

On Sat, Sep 23, 2017 at 08:04:40PM +0200, René Scharfe wrote:

> Decode =XY in quoted-printable segments only if X and Y are hexadecimal
> digits, otherwise just copy them.  That's at least better than
> interpreting negative results from hexval() as a character.

Thanks, this looks good to me overall.

I wondered if we should die() here, but walking over cruft may be more
friendly. The base64 case does the same, though it actually ignores the
bytes rather than copying them. Since this is never _supposed_ to
happen, it's hard to say what behavior would be preferable without
seeing a real-world broken case.

-Peff