Fix DKIM-Signature header flowing, it is not valid to arbitrarily sli…

[jensbjorgensen]

Hello, another user reported that the package sometimes create invalid DKIM-Signature headers due to how it is flowing lines. As you noted in reply, the way flowing is done presently in the code is not correct--in particular the header value cannot be split arbitrarily at any point, because when the header is processed at the receiver, the linebreaks remain whitespace and inserting whitespace into a value can only be done in base64 values without corrupting the data. I've written a robust and generalized solution that fixes this and I would humbly present it to you for you acceptance.

[jensbjorgensen]

Hmm ok tests failed, sorry let me run those on my end. Also I added another commit, there was another small problem.

[emersion]

Thanks for the PR! I'm sorry about this, but I'm not sure we'll want to maintain that much code though.

What about using https://godoc.org/github.com/emersion/go-message/textproto#WriteHeader instead?

[emersion]

I take it this PR fixes #18 right?

[jensbjorgensen]

Yes, exactly that. And I don't think that a general "rfc822 header flow" solution will work, because the thing is the spec says that you /can/ split the base64 fields (bh, b), but you can't split other things without breaking the signature unless by luck. So actually if you're library solution doesn't pay attention to the specific header content and the semantics of the values then unfortunately it can't operate correctly.

On 5/7/20 8:59 PM, Simon Ser wrote: I take it this PR fixes #18 (comment) <#18 (comment)> right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3HANJKN3MIZLDJU3TTRQKWDNANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[foxcpp]

RFC seems to allow interesting arbitrary whitespace in base64 data. Is it possible to make textproto.WriteHeader folding algorithm produce valid output by inserting e.g. spaces in base64 as a hint?

[foxcpp]

That would require go-message to expose folding algorithm independently in API. I think that's formatHeaderField function in textproto.go. @jensbjorgensen, mind taking a look if you can reuse it?

[foxcpp]

If possible, we can try to improve it. Because you know, maintaining one implementation is enough fun already, let alone two.

[jensbjorgensen]

It sure is. You already have 2 implementations, and one of them is broken ;-). I'll take a look at the function you mentioned.

On 5/8/20 5:51 PM, Max Mazurov wrote: If possible, we can try to improve it. Because you know, maintaining one implementation is enough fun already, let alone two. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3ALHRC7ND7T55BMZJTRQPI2VANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[jensbjorgensen]

Doesn't look like it can do the job. It's looking at the line as a whole. The thing is you need to know the specific tag-value in question. How would the hinting work? Have to pass in the regions of the line that are base64? I admire the fact that you are trying to avoid duplicate code, but the thing is, it's not actually duplicate. It'd be great if there was some uniform set of rules around formatting email header lines, but aside from rules about flowing/unflowing, there aren't. Best you could do is just follow the rule on never breaking except on whitespace. Here's what the existing code generated from an actual email it sent along with some headers inserted by the receiving server. Note especially the "dkim=permerror" in Authentication-Results: Authentication-Results: inbound6.ore.mailhop.org; spf=pass smtp.mailfrom=chat.yoodychat.com; dkim=permerror header.d=chat.yoodychat.com header.s=vrfy0 header.a=rsa-sha256 header.b=R41Rfy0u; dmarc=pass header.from=chat.yoodychat.com; X-HalonDuo-Scores: {"rpd":"unknown", "sa":0.6, "sa_scores":{"HTML_MESSAGE":0.001, "HTML_TAG_BALANCE_BODY":0.1, "INVALID_DATE":0.432, "MIME_HTML_ONLY":0.1}} Envelope-Sender: [email protected] X-HalonDuo-ID: 8dcf73ca-8f50-11ea-b273-99dddcba7cd5 Received: from chat.yoodychat.com (chat.yoodychat.com [13.228.210.44]) by inbound6.ore.mailhop.org (Halon) with ESMTPS id 8dcf73ca-8f50-11ea-b273-99dddcba7cd5; Wed, 06 May 2020 04:17:54 +0000 (UTC) Received: (qmail 15989 invoked from network); 6 May 2020 03:51:11 +0000 Received: from chat (HELO localhost) (127.0.0.1) by chat.yoodychat.com with SMTP; 6 May 2020 03:51:11 +0000 DKIM-Signature: a=rsa-sha256; bh=7Xgui0yFAxLMluvjaRLRKJPgrOpPtHSIYy/BndZ2zL g=; c=simple/simple; d=chat.yoodychat.com; h=To:From:Subject:Date:Message-I D:MIME-Version:Content-Type:Content-Transfer-Encoding; s=vrfy0; t=158873707 1; v=1; x=1588744271; b=R41Rfy0ueHrSXNLcxjio8q9dPnLgNV0Cs4+eMyXVIC1HHw4X04V Zl7H3dU8LpbbfOttFYL0UNN5HX1uK971nBI1Bi1QEFiTxaeU4V55Fl1dZpPBfUeMuC4KdoHyqv8 Dg3nVp4PRAaE93GfgLmFsRIM9O8gNMe31++LUaskDt788ayjQsfuxIakf3Z1HJtWn7vizCjVeF9 oVB0b784jpg9Qp956mmS85uFpptqX5M1Mn9Q7eYHLqcJuMcbcTP4nLOYUep+9h09Lz8c0A+MF/9 NQ5aAxhVAU085FeuFFaW8iAusCpgflQn1J9/mjrRGlpgbcFIUvfAmPvGEB95cTPk0A==; It split apart the h= tag, mangling Message-ID, also the t= mangling the message timestamp. So that is definitely broken. I don't see how the formatHeaderField, which just sees the value as one big string, has a chance to know that after b= the data base Base64 and can be split. I hope you don't take my comments as rude, but that logic is just too generalized. I think you'd be better off with a more complicated model that can reduce to the simple case which formatHeaderField is doing. With a few tweaks the code I wrote to fix dkim could work as a drop-in replacement for formatHeaderField while also supporting more complex cases such as DKIM by exposing that level as well.

On 5/8/20 5:50 PM, Max Mazurov wrote: That would require go-message to expose folding algorithm independently in API. I think that's formatHeaderField function in textproto.go. @jensbjorgensen <https://github.com/jensbjorgensen>, mind taking a look if you can reuse it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3ABSEEN6ACAT6NX5ITRQPIXFANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[emersion]

If go-message doesn't help, there's still no need to have all of this complexity. We could just keep the params map we had, and put each parameter on a single line, inserting "\r\n " in the value if necessary.

[ludusrusso]

I notice the same issue! It's a problem when you send email to "@hotmail.xx" or any MS receiver that did not pass dkim verification due to this issue!

[jensbjorgensen]

Yeah, it's a significant problem that will in many cases end up with non-delivery. It's fixed in the fork on my github, it looks like my patch is not going to be merged so I'm not sure when it'll be updated in the the author's repo. Feel free to pull from my fork in the meanwhile.

On 5/20/20 11:28 PM, Ludovico Russo wrote: I notice the same issue! It's a problem when you send email to ***@***.***" or any MS receiver that did not pass dkim verification due to this issue! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3DQ65YLPNXUB2U2A2LRSPZJJANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[emersion]

@jensbjorgensen Do you have any plans to update this PR to address the feedback you've received?

[jensbjorgensen]

Ahhh, mmm, well I haven't decided to /not/ do anything about it ;-), just really busy with some other stuff at the moment, since the patch I offered works correctly it hasn't been high priority.

On 5/21/20 4:40 PM, Simon Ser wrote: @jensbjorgensen <https://github.com/jensbjorgensen> Do you have any plans to update this PR to address the feedback you've received? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3D546HPOTGIANQJWPTRSTSHDANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[ludusrusso]

I can work on it if it's ok for you!

[jensbjorgensen]

Hehe sure, why not! As the suggestion made before, the simplest way to go would be to just flow the lines on the nearest semi-colon. Even with RSA 4096-bit keys each part should be below the max 998 chars hard limit.

On 5/22/20 7:08 PM, Ludovico Russo wrote: I can work on it if it's ok for you! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3EP4FSF4PG6DTFWPELRSZMK5ANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[jensbjorgensen]

In dkim/sign.go there's a function formatSignature which in turn calls foldHeaderField, that function is arbitrarily wrapping at 75-char boundaries. If you changed it to instead fold right after ';' where the rest of the line was longer than 78 chars that would do the trick.

On 5/22/20 7:08 PM, Ludovico Russo wrote: I can work on it if it's ok for you! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3EP4FSF4PG6DTFWPELRSZMK5ANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[ludusrusso]

In dkim/sign.go there's a function formatSignature which in turn calls foldHeaderField, that function is arbitrarily wrapping at 75-char boundaries. If you changed it to instead fold right after ';' where the rest of the line was longer than 78 chars that would do the trick.
On 5/22/20 7:08 PM, Ludovico Russo wrote: I can work on it if it's ok for you! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNVZ3EP4FSF4PG6DTFWPELRSZMK5ANCNFSM4M3I7PZA.
-- Jens B. Jorgensen [email protected]

Uhm, that would not solve completely the issue if the headers string is longer then 75 chars. What about to completelly rewrite the formatHeaderParams in order to generate directly a well folded header? It should be simpler and also this should avoid folding issues like #23

[jensbjorgensen]

Well yes, that's the crux of the problem, but also this is not quite true. RFC 2822 (Section 2.1.1) says:

There are two limits that this standard places on the number of characters in a line. Each line of characters MUST be no more than 998 characters, and SHOULD be no more than 78 characters, excluding the CRLF.

So to interpret strictly we can get away with lines up to 998. The longest (unless you had some massive list of headers you were hashing over) will be the 'b' attribute containing the RSA signature, which has the same size as the key modulus so if you have a 2048-bit key then you end up with 2048/8 * 4 / 3 (after base64 encoding) so 342 bytes of data. Still comfortably under the 998 chars, even if you move up to 4096-bit keys. So I think that's Simon' and Max's point: if you just chop the line after each key-value you should end up with something that fulfills the looser requirement. Considering your suggestion, this is exactly what I did. However the code is 232 lines to do that. The original author, Simon, preferred to not have code specialized to flowing DKIM headers, resulting in multiple line-flowing implementations, a motivation I completely understand. My implementation could be used instead for all cases with a slight adjustment but this apparently was not desirable so I haven't writing that code change. So you've ended up in the same quandry as I did. It is simply not possible to write general-purpose (which is to say, code that could be used to flow /any/ headers) which will correctly and reliably flow header lines down to 78 chars. It is not possible because in the general case you can only split on whitespace. There are other places you can split (for DKIM in the middle of base64 data anywhere you like, or on the colons that separate the list of headers that are hashed) but only if you know the semantic content of the headers. Where to go from there? Well, if you want your DKIM headers flowed correctly and optimally, you can just use my fork. I have been thinking to write the code Simon suggested that would at least generate correct DKIM headers but this is low-priority for me as I have a working solution I'm happy with.

On 5/23/20 2:45 AM, Ludovico Russo wrote: formatSignature Uhm, that would not solve completely the issue if the headers string is longer then 75 chars. What about to completelly rewrite the |formatHeaderParams| in order to generate directly a well folded header? It should be simpler and also this should avoid folding issues like #23 <#23> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3A6NPKHILXBM7WISQTRS3B45ANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[foxcpp]

Is there a test case that reproduces issue with folding?

[foxcpp]

unless you had some massive list of headers you were hashing over

This can be the case, take a look at the list of headers signed by maddy by default.
https://github.com/foxcpp/maddy/blob/b54c705e2d3f415daa853a2e146322f4f29a8dca/internal/modify/dkim/dkim.go#L29

P.S. All field names from the first list are included twice in DKIM-Signature ("oversign").

[jensbjorgensen]

[TLDR: the existing test cases already fail if you fix the verifier to parse DKIM header per-RFC] [https://github.com/jensbjorgensen/go-msgauth/tree/jens_dkim_test] Looking at the existing test cases, the rsa one in dkim/sign_test.go and dkim/sign_ed25519_test.go both of those tests have DKIM signatures that should be failing. In sign_test.go the selector value is split (s=br" + "\r\r" + " "isbane...). A receiver would end up with a selector value of "br isbane" which of course is invalid because DNS name components cannot have space characters but in any case such lookup would fail. It works in the test because it processes the 's' parameter with a function that cleans out /all/ space characters (stripWhitespace in dkim/verify.go). This is not standards-compliant, note from RFC 6376: [from section 3.2, my italics]

Note that WSP is allowed anywhere around tags. In particular, any WSP after the "=" and any WSP before the terminating ";" is not part of the value; however, /WSP inside the value is significant/.

And for further qualification: [from section 3.5]

s= The selector subdividing the namespace for the "d=" (domain) tag (plain-text; REQUIRED).

I noted this because if the value is specific as dkim-quoted-printable then there's processing which removes embedded whitespace, but the selector is not specified as such. In the test TestSignAndVerifyEd25519 similarly the 'h' value is also split like h=From:To:Subject:Date:Mes\r\n sage-ID The parameter is defined as: [also from section 3.5]

h= Signed header fields (plain-text, but see description; REQUIRED). A colon-separated list of header field names that identify the header fields presented to the signing algorithm.

(The ABNF for hdr-name references RFC 5322) So while I think it's great that the code can be flexible to verify DKIM signatures that aren't correctly constructed experience indicates that lots of verifiers out there are not so forgiving.

On 5/24/20 8:10 PM, Max Mazurov wrote: Is there a test case that reproduces issue with folding? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3FCOY2BP53MAMJQBEDRTEFDXANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[emersion]

Regarding h=:

Folding whitespace (FWS) MAY be included on either side of the colon separator

[jensbjorgensen]

Yes, correct, but not in the middle of the header name: Mes\r\n sage-ID

On 5/25/20 4:18 PM, Simon Ser wrote: Regarding |h=|: Folding whitespace (FWS) MAY be included on either side of the colon separator — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNVZ3HFPDWKIEUAJW3TYWDRTISXDANCNFSM4M3I7PZA>.

-- Jens B. Jorgensen [email protected]

[emersion]

Superseded by #29