setFromHeaderValue() and setReturnPathValue() need to be case iNSenSITive

[fortiko]

While checking our SPF, DKIM and DMARC configuration, I realised that the default comparisons in setFromHeaderValue() and setReturnPathValue() are case-sensitive, whereas they need to be case iNSenSITive, because otherwise several "From" and "Return-Path" headers might exist in one message, which in turn makes the message "invalid" for Gmail ("550-5.7.1 not RFC 2822 compliant"), check-auth2@verifier.port25.com ("DK_STAT_SYNTAX: Message is not valid syntax.") et al.

An example: the current way of comparing

        if (isset($headers["from"])) {

ignores an already existing message header added by the MUA like this (because array indices in PHP are always case sensitive)

From: john@domain.tld

and as a result adds a second header like this:

from: default-sent-from-config.php

thus invalidating the message format. Several ways to fix this are presented here, not sure which one is the best, but it should certainly be used consistently for all header comparisons, such as setReturnPathValue(): http://stackoverflow.com/questions/4240001/php-array-keys-case-insensitive-lookup

[fortiko]

OK, some more information: this is an issue in my case with S/MIME signed messages, because of the following code:

        $finalBody = "";
        $finalHeaders = array();

        // if it's a S/MIME message I don't do anything with it
        if (is_smime($message)) {
            $mobj = new Mail_mimeDecode($sm->mime);
            $parts =  $mobj->getSendArray();
            unset($mobj);
            if ($parts === false) {
                throw new StatusException(sprintf("BackendIMAP->SendMail(): Could not getSendArray for SMIME messages"), SYNC_COMMONSTATUS_MAILSUBMISSIONFAILED);
            }
            else {
                list($recipents, $finalHeaders, $finalBody) = $parts;

                $this->setFromHeaderValue($finalHeaders);
                $this->setReturnPathValue($finalHeaders, $fromaddr);

It appears that $finalHeaders uses "From" instead of "from", which also explains the following log lines:

[sender@domain.tld] BackendIMAP->SendMail(): We get the new message
[sender@domain.tld] BackendIMAP->SendMail(): We get the From and To
[sender@domain.tld] BackendIMAP->setFromHeaderValue(): From defined: first.last@domain.tld
[sender@domain.tld] BackendIMAP->SendMail(): To defined: sender@domain.tld
[[ALL GREAT, THE EXISTING HEADERS in $message->headers ARE FOUND CORRECTLY]]
[sender@domain.tld] BackendIMAP->setReturnPathValue(): No Return-Path address defined, we use From
[[NOW WE BRANCH INTO THE S/MIME PART ABOVE, AND FOR SOME REASON THE COMPARISON THAT WORKED BEFORE ON $message->headers NOW FAILS ON $finalHeaders]]]
[sender@domain.tld] BackendIMAP->setFromHeaderValue(): No From address defined, we try for a default one
[sender@domain.tld] BackendIMAP->setReturnPathValue(): No Return-Path address defined, we use From
[sender@domain.tld] BackendIMAP->SendMail(): Final mail to send:
[sender@domain.tld] BackendIMAP->sendMessage(): SendingMail with mail

and this results in the double "From" and "from" line, which makes the message invalid RFC2822.

Not sure what the right fix for this is, lower-casing $finalHeaders perhaps?

[fortiko]

Confirmed: in $message->headers we have a valid "from" (small 'f') header: GOOD.

In $finalHeaders as created in the S/MIME portion (and only there!), we have a "From" (capital 'F') header, which then fails in setFromHeaderValue(). Same for setReturnPathValue(): BAD.

Either we need to fix setFromHeaderValue() and setReturnPathValue(), or $finalHeaders needs to be generated differently which would probably fail because then the S/MIME signature will be invalidated.

All this is under iOS 7.1.2 and higher.

[fmbiete]

Hi @fortiko ,

I think this commit, would be enough. We don't need to readd the already existing headers to the message. Also, as far as I know the S/MIME signature doesn't contains the headers, only the body part signed. But google will check the correctness of the message (S/MIME or plain) and a message with multiple "From/Return-To/Subject" header is invalid.

Let me know if that fixes the problem.

[fortiko]

Hi @fmbiete:

Your fix looks like it makes sense for newly generate MIME messages without S/MIME, but it does nothing related to S/MIME messages.

The problem is around line 260. I added this ZLOG line:

                list($recipents, $finalHeaders, $finalBody) = $parts;
                ZLog::Write(LOGLEVEL_DEBUG, sprintf("BackendIMAP->SendMail(): This is an S/MIME message, finalHeaders are: From: %s from: %s", $finalHeaders["From"], $finalHeaders["from"]));
                $this->setFromHeaderValue($finalHeaders);
                $this->setReturnPathValue($finalHeaders, $fromaddr);

and get the following error message:

Z-Push-contrib/backend/imap/imap.php:259 Undefined index: from (8)

"From" (CAPITAL F) does exists as an index, and has the correct value. Unfortunately, both setFromHeaderValue() and setReturnPathValue() rely on "from" (little f) and therefore do not find the (already existing) "From" and "Return-Path" values.

Alternatives:

• setFromHeaderValue() and setReturnPathValue() need to check for all potential (index) spellings (From, from, FROM)
• the index in $finalHeaders IN THE S/MIME SECTION AROUND LINE 260 needs to be cast to lowercase, instead of using whatever the MUA on the device uses (e.g. "From" with CAPITAL F in case of iOS 7+, thus failing the comparison). I don't think that list() can do that, so perhaps it needs to be a manual thing?

[fmbiete]

umm...

setFromHeaderValue() already check for the From/from header

private function setFromHeaderValue(&$headers) {
        $from = $this->getDefaultFromValue();

        // If the message is not s/mime
        if (isset($headers["from"])) {
            ZLog::Write(LOGLEVEL_DEBUG, sprintf("BackendIMAP->getFromHeaderValue(): from defined: %s", $headers["from"]));
            if (strlen(IMAP_DEFAULTFROM) > 0) {
                ZLog::Write(LOGLEVEL_DEBUG, sprintf("BackendIMAP->getFromHeaderValue(): Overwriting From: %s", $from));
                $headers["from"] = $from;
            }
        }
        elseif (isset($headers["From"])) {
            // if the message is s/mime
            ZLog::Write(LOGLEVEL_DEBUG, sprintf("BackendIMAP->getFromHeaderValue(): From defined: %s", $headers["From"]));
            if (strlen(IMAP_DEFAULTFROM) > 0) {
                ZLog::Write(LOGLEVEL_DEBUG, sprintf("BackendIMAP->getFromHeaderValue(): Overwriting From: %s", $from));
                $headers["From"] = $from;
            }
        }
        else {
            // not From header found
            ZLog::Write(LOGLEVEL_DEBUG, sprintf("BackendIMAP->getFromHeaderValue(): No From address defined, we try for a default one"));
            $headers["from"] = $from;
        }
    }

and setReturnPathValue() only will set the new header if the Return-Path/return-path pair doesn't exist

private function setReturnPathValue(&$headers, $fromaddr) {
        if (!(isset($headers["return-path"]) || isset($headers["Return-Path"]))) {
            ZLog::Write(LOGLEVEL_DEBUG, sprintf("BackendIMAP->setReturnPathValue(): No Return-Path address defined, we use From"));
            $headers["return-path"] = $fromaddr;
        }
    }

so, for S/MIME it should be ok, and the last fix should also make it ok for non S/MIME

[fortiko]

I am sorry, I had not seen that fix for #78 that you refer to which fixed it: thanks!

[fmbiete]

No problem!!