Michael Elkins wrote: > I responded to this on the ticket, but after reviewing the code, it appears > that all header lines are unfolded at the point that skip_email_ws() is being > used. It used to be that we didn't always unfold, so the SKIPWS() macro > considered newline to be a whitespace so it didn't prematurely terminate > processing folded lines.
I am attaching two patches to this email. The ticket-3716-dev.patch changes EMAIL_WSP to be space and tab only. It removes both \n and \r. It seems as long as we're removing \n, we might as well go for broke and remove the carriage return too. > > It looks like the quickest change would be to revert sendlib.c back to > > using: > > while (*t == ' ' || *t == '\t') > > t++; > > This is probably the safest option, at least until we are certain that > changing > the semantics of skip_email_ws() is not harmful. It will solve the crash, and > not introduce any other new behavior. I'd suggest pushing this change to > stable, and the skip_email_ws() change to the dev branch. ticket-3716-stable.patch does this, reverting to the pre [f251d523ca5a] code for the particular call in write_one_header(). Care will have to be taken the next time stable is merged into default though, to somehow prevent this particular commit from being pulled over. > > Long term, it would be better create two functions: a strict WSP and a > > loose whitespace one, and tease apart what needs which. > > Based on my review, I don't think separate functions are necessary. Okay, thank for you taking a look at all those calls! -Kevin
# HG changeset patch # User Kevin McCarthy <[email protected]> # Date 1417472364 28800 # Mon Dec 01 14:19:24 2014 -0800 # Branch stable # Node ID 54c59aaf88b9f6b50f1078fc6f7551fa9315ac3e # Parent 1b583341d5ad677c8a1935eb4110eba27606878a Revert write_one_header() to skip space and tab. (closes #3716) This patch fixes CVE-2014-9116 in the stable branch. It reverts write_one_header() to the pre [f251d523ca5a] code for skipping whitespace. Thanks to Antonio Radici and Tomas Hoger for their analysis and patches to mutt, which this patch is based off of. diff --git a/sendlib.c b/sendlib.c --- a/sendlib.c +++ b/sendlib.c @@ -1809,17 +1809,22 @@ { tagbuf = NULL; valbuf = mutt_substrdup (start, end); } else { tagbuf = mutt_substrdup (start, t); /* skip over the colon separating the header field name and value */ - t = skip_email_wsp(t + 1); + ++t; + + /* skip over any leading whitespace (WSP, as defined in RFC5322) */ + while (*t == ' ' || *t == '\t') + t++; + valbuf = mutt_substrdup (t, end); } dprint(4,(debugfile,"mwoh: buf[%s%s] too long, " "max width = %d > %d\n", NONULL(pfx), valbuf, max, wraplen)); if (fold_one_header (fp, tagbuf, valbuf, pfx, wraplen, flags) < 0) return -1; FREE (&tagbuf);
# HG changeset patch # User Kevin McCarthy <[email protected]> # Date 1417472349 28800 # Mon Dec 01 14:19:09 2014 -0800 # Node ID e8e07f3c08e5e83dbeb83ace6649c05042e5ff9c # Parent 39d3ddb56d340f66ffd0fe476003645f4cdc30bc Change EMAIL_WSP to be space and tab (references #3716) This patch fixes CVE-2014-9116 in the development branch, by changing EMAIL_WSP, skip_email_wsp(), and is_email_wsp() to define WSP as space and tab only. Michael reviewed [f251d523ca5a] and the code using these functions, and believes they all are used for unfolded headers. Furthermore, it makes sense for WSP to match RFC5322. Thanks also to Antonio Radici and Tomas Hoger for their analysis and patches to mutt, which this patch is based off of. diff --git a/lib.h b/lib.h --- a/lib.h +++ b/lib.h @@ -93,17 +93,17 @@ #define FMT_CENTER -1 #define FOREVER while (1) /* this macro must check for *c == 0 since isspace(0) has unreliable behavior on some systems */ # define SKIPWS(c) while (*(c) && isspace ((unsigned char) *(c))) c++; -#define EMAIL_WSP " \t\r\n" +#define EMAIL_WSP " \t" /* skip over WSP as defined by RFC5322. This is used primarily for parsing * header fields. */ static inline char *skip_email_wsp(const char *s) { if (s) return (char *)(s + strspn(s, EMAIL_WSP));
signature.asc
Description: PGP signature
