Bad headers check is incorrect

[asyschikov]

Flask-mail checks for newline in email which is actually perfectly valid and legal:
http://tools.ietf.org/html/rfc5322#section-2.2.3
Basically flask-mail fails to send certain correct mail.

[asyschikov]

If the subject is longer than 78 chars many mail clients break it into lines with white space padding, this is what gmail does for example:

Subject: sadfihusdfgiluhsfgui sadfihusdfgiluhsfgui sadfihusdfgiluhsfgui
 sadfihusdfgiluhsfgui sadfihusdfgiluhsfgui sadfihusdfgiluhsfgui
 sadfihusdfgiluhsfgui sadfihusdfgiluhsfgui sadfihusdfgiluhsfgui

which is perfectly legal according to specification. We use flask-mail to forward user mail, so if this stuff comes from gmail and we try to forward it via flask-mail the sending fails due to "BadHeaders", which is very nasty. Maybe my understanding is wrong but it looks like flask-mail handles this situation incorrectly.

[jamesonjlee]

The reason for bad headers is because of CRLF can lead to header injections, but for subject this should explicitly check if it's a CRLF+WSP, so for subject we should be safe to call strip(), and check that each new line start with a WSP (as to prevent it from being ingested as a header line)

[jamesonjlee]

or do a replacement of CRLF + WSP with WSP?

[asyschikov]

@jamesonjlee it is very insecure method of preventing headers injection, because it checks only certain fields (subject, sender, reply_to, recipients). It should throw error only if CRLF is not followed by space.

[jamesonjlee]

@asyschikov https://github.com/mattupstate/flask-mail/blob/b8ab0858f86bbb215f5b8a11c843b71bcb8ab3b5/flask_mail.py#L360 that can be switched for a specific CRLF + WSP check for Subject and then regular CRLF for the rest of the headers.

[jamesonjlee]

so got stuck reading RFC5322, here is a possible approach

    def _is_bad_header(line):
        if '\r' in line or '\n' in line:
            return True
        return False

    def has_bad_headers(self):
        """Checks for bad headers i.e. newlines in subject, sender or recipients.
        RFC5322: Allows multiline CRLF with trailing whitespace (FWS) in headers
        """

        headers = [self.sender, self.reply_to] + self.recipients
        for header in headers:
            if is_bad_header(header):
                return True

        if self.subject:
            if is_bad_header(self.subject):
                for linenum, line in enumerate(self.subject.split('\r\n')):
                    if linenum > 0 and line[0] not in '\t ':
                        return True
                    if _is_bad_header(line):
                        return True
                    if len(line.strip()) == 0:
                        return True
        return False

the unstructured headers are only comment and subject so I split them out.

[asyschikov]

@jamesonjlee looks good. small things: 1) is_bad_header is with or without leading underscore (you have mix) 2) '\t' or ' ' right? 2) if _is_bad_header(line): is always false because it is after split (all new lines removed) . But yeah that it basically it

[jamesonjlee]

1. yep, should be leading underscore
2. not in '\t ' as not a horizontal tab nor space
3. only if you don't stick a \r or \n in there, e.g. testing\n string \r\n (RFC says CRLF is the only acceptable format