deanproxy / eMail

Command line SMTP client
https://www.deanproxy.com/code
GNU General Public License v2.0
135 stars 46 forks source link

email corrupts mail bodies #9

Closed mined closed 6 years ago

mined commented 10 years ago

[I had reported this before @ http://www.cleancode.org/projects/email/contact but received no response so far]

One of my mails recently was cut off after a line containing a non-ASCII character and a final dot ("."). No idea how many other emails had been corrupted before as this happened silently. Since this is a serious reliability issue I would ask for quick response and fix, please.

Sample mail body: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.

xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx. xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.

xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxxx xxxxxxxxxxxxxxxxx.

xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.

xxxxxxxxxxxxxxxxxxxxxxxxx

xxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

xxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxx.x.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxäxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

xxxxxxxxxxx xxxäxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxx xxxxxxxxäxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxäxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

xxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

xxxxxxxxxxxxxxxxxxxxxxxxxxxxäxxxxxxxxxxxx.

xxxxxxxxxxxxxxxxx xxxxxxxxxxxx

xxxxxxxx

xxxxxxxxxxxx xxxxxxxxxxx

mined commented 10 years ago

I did some tracing and found that the affected line wraps like this, mime-encoded: xxxxxxxxxxxxxxxxxxxxxxxxx=C3=A4xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx= . where the single dot obviously terminates the process at the smtp server. It's time to provide some feedback here, and to fix this most serious bug! This is really not "clean code"!

deanproxy commented 10 years ago

This project receives very little attention anymore, since I have very little time given I have a family to attend to now. I can get to it when I have a moment, or you can feel free to dig into the code yourself and create a pull request with any changes you may have.

mined commented 10 years ago

I fully understand your priorities in general; however, this is a critical bug as people can get their outgoing mails corrupted without even noticing. Anyway, after some more analysis I found a fix for most part; the core problem is that eMail does not handle leading dots at all. Because a single dot line terminates SMTP transfer, however, every leading dot on a line must be duplicated. The problem occurs both with Unicode text and ASCII text, in different branches of the sources.

Places to fix for the UTF-8 case: dstrbuf.c (dlib): dsbPrintf: check leading '.'; duplicate!

and for the ASCII case: message.c: makeMessage calling: dstrbuf.c (dlib): dsbCat ...

I'll provide my fixes which do not cover the signature and pgp handling where in theory the same issue could arise.

Hmm, why can't I add my fixes here as an attachment...

deanproxy commented 10 years ago

Fork the project, apply your fixes, commit them and submit a pull request. On May 23, 2014 4:14 PM, "towo-net" notifications@github.com wrote:

I fully understand your priorities in general; however, this is a critical bug as people can get their outgoing mails corrupted without even noticing. Anyway, after some more analysis I found a fix for most part; the core problem is that eMail does not handle leading dots at all. Because a single dot line terminates SMTP transfer, however, every leading dot on a line must be duplicated. The problem occurs both with Unicode text and ASCII text, in different branches of the sources.

Places to fix for the UTF-8 case: dstrbuf.c (dlib): dsbPrintf: check leading '.'; duplicate!

  • or when calling, in particular: mimeutils.c: mimeQpEncodeString or qpStdout sig_file.c: appendFortune, appendSig (dsbCatChar!) ... message.c: createGpgEmail (gpgDate!)

and for the ASCII case: message.c: makeMessage calling: dstrbuf.c (dlib): dsbCat ...

I'll provide my fixes which do not cover the signature and pgp handling where in theory the same issue could arise.

Hmm, why can't I add my fixes here as an attachment...

— Reply to this email directly or view it on GitHubhttps://github.com/deanproxy/eMail/issues/9#issuecomment-44055347 .

mined commented 10 years ago

I submitted patches (2 files). Please commit the fixes and notify me. I'd like to prompt cygwin to make an update to fix the issue there.

mined commented 10 years ago

I understood your patch-comments so that you wouldn't wish to accept them. What I did is find a quick-and-easy fix for the issue without analysing all your software design first. While I am aware that this is a hot fix, and fixing a similar thing in different software layers isn't quite "clean code", the existing software architecture did not offer much other possibilities to insert an easy fix (i.e. without restructuring). However, it is a fix and as such certainly better than the current status of 'email' which, with the possible unnoticed corruption of outgoing mail, isn't really "clean code" either. Please consider this and that a fix is urgently needed. You may clean up the code later ("refactoring" which is considered a clean code practice anyway).

deanproxy commented 10 years ago

I added a fix yesterday and committed it. Please fill free to test it for me and let me know if it fixes your issues.

On Wed, May 28, 2014 at 10:16 AM, towo-net notifications@github.com wrote:

I understood your patch-comments so that you wouldn't wish to accept them. What I did is find a quick-and-easy fix for the issue without analysing all your software design first. While I am aware that this is a hot fix, and fixing a similar thing in different software layers isn't quite "clean code", the existing software architecture did not offer much other possibilities to insert an easy fix (i.e. without restructuring). However, it is a fix and as such certainly better than the current status of 'email' which, with the possible unnoticed corruption of outgoing mail, isn't really "clean code" either. Please consider this and that a fix is urgently needed. You may clean up the code later ("refactoring" which is considered a clean code practice anyway).

— Reply to this email directly or view it on GitHubhttps://github.com/deanproxy/eMail/issues/9#issuecomment-44411871 .

deanproxy commented 9 years ago

I'm going to assume this is fixed to your satisfaction. If the problem persists, please reopen this bug.

mined commented 9 years ago

It's not fixed. Email is still utterly broken as I had described. Note the dot checking does not need to be applied to the input contents but to the transmitted data stream (which may be the same for ASCII-only files with short lines...). I've retested with 3.2.1-git and the above test case (which you could have tested yourself, actually) and the mail is being corrupted. Also I'd like to remind that I provided a working patch already. Not liking the lack of design-integration of the patch is not really a good reason to let email continue to corrupt e-mails for more than a year.

mined commented 9 years ago

"reopen the bug": no option for this in github

mined commented 9 years ago

This bug has actually 3 incarnations, as can be seen in this shorter test case: abc .def ghi . jkl xxxxxxxxxxxxxxxxxxxxxxxxx¢xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx. xyz

The second line is shortened to "def" (dot missing). The dot only line as well as the line containing "¢" truncate the mail body. Of these two critical errors you only fixed the first.

deanproxy commented 9 years ago

I have been testing it and I cannot reproduce the problem. I do, however, have limited test resources. I've sent to a QMail SMTP server as well as Gmail's SMTP servers. Both seem to handle your test case above with no problems. As a matter of fact, with your test case above I found that 3 dots are added to the single dot test case ('.' only on a line by itself) but no truncating happens.

I'll keep testing and try to find some different SMTP servers to test with.

mined commented 9 years ago

Maybe I should have pointed out more clearly that the file is UTF-8-encoded. If that's the problem to reproduce the issue, I apologize. I would have attached it as a binary if this !#% github system allowed for it.

deanproxy commented 9 years ago

I think you were fairly clear once I read back. I'm just now getting caught back up with everything so I'm forgetting some of the details... I'll generate some UTF-8 stuff.

deanproxy commented 9 years ago

Okay, I believe I have a fix situated now.

Here is the original message:

abc .def ghi. . jkl ö.öö. xyz

Here is the result that's sent to the server:

Subject: Testing From: "Dean Jones" dean@li67-28 To: dean@test Date: Wed, 14 Jan 2015 15:08:41 -0500 Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline X-Mailer: Cleancode.email v

abc ..def ghi. .. jkl =C3=B6.=C3=B6=C3=B6. xyz

Tests confirm I'm receiving the e-mail message as the original text. Can you please confirm?

deanproxy commented 9 years ago

I think there may be one more test case I'm forgetting, which is if the QP content wraps on a dot... I'll figure that out tonight.

mined commented 9 years ago

The test case I provided wraps (in quoted-printable, i.e.) just before the dot. ö.öö. is not long enough, it does not wrap

deanproxy commented 9 years ago

Added another update to handle wrapped quoted printable stuff. Give it a try when you get a chance.

mined commented 9 years ago

(skip this comment, see next) ---Unfortunately, all 3 test cases fail again (including the one that was fixed before); the leading dot (.def) is stripped, and both other dots would terminate the mail contents. How can I check what is sent to the server (as mentioned above)? Wait! Somehow I managed to compile a new but call an old version... ; Can't test the new version yet as it complains email: FATAL: Can't open address book: '/usr/local/etc/email/email.address.template' email: FATAL: You must specify at least one recipient! Apparently, there's some configuration needed... (anyway I think a missing address book should not stop it from sending a mail with full email address actually specified - the second message line is wrong - but that's another issue).

mined commented 9 years ago

OK, after uncommenting ADDRESS_BOOK in .email.conf (and considering that email does not use $HOME/.email.conf), testing succeeded and all 3 situations are OK now. Thank you.

I did not check whether the fixes really catch all cases or maybe in situations like other encodings, encrypted mails, signatures, whatever, the dot problem would bypass the current fix.

deanproxy commented 9 years ago

I'll run a few more test cases before I close out the bug to make sure it passes all possible cases. Thanks for helping out with this one.