Bug 3457 - broken winmail.dat attachments cause crash
Summary: broken winmail.dat attachments cause crash
Status: RESOLVED FIXED
Alias: None
Product: Claws Mail
Classification: Unclassified
Component: Plugins/tnefParse (show other bugs)
Version: 3.11.1
Hardware: PC Linux
: P3 normal
Assignee: users
URL:
Depends on:
Blocks:
 
Reported: 2015-07-02 15:50 CEST by MD
Modified: 2015-07-15 16:49 CEST (History)
0 users

See Also:


Attachments
Two problem Email messages (16.36 KB, application/x-gzip)
2015-07-02 15:50 CEST, MD
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description MD 2015-07-02 15:50:54 CEST
Created attachment 1528 [details]
Two problem Email messages

We are starting to have issues with certain incoming Email messages causing Claws to freeze for several seconds and then crash.  Because the offending message remains in the user's incoming /var/spool/mail/$LOGNAME file, when they restart Claws, it will start processing the mailspool again (re-importing all the messages again) and then freeze and crash again when it hits the offending message.

This is a major problem because if a user gets such a message, they can no longer use Claws until a sysadmin goes in and manually parses out all the Email in their mailspool and tries to import each message, one-by-one, until the offending one can be identified.  This is very time consuming.

We are using Protocol: Local mbox file

I have two such messages that will crash Claws, even the most current version of Claws.  I sanitized them slightly by changing all from address domains to "nospamfrom" and the to address domains to "nospamto" and wiping phone numbers and such.  The two messages are not from the same person or same domain, and separated by more than a month.  The only thing they seem to have in common is that the mail agent is MS Outlook and they have a plaintext part and TNEF part.  But none of that is unusual, we get such Email all the time with no problems.

I am attaching the two problem Email messages to this bug report.  To use them, simply cat one of them into a test user's incoming local mbox file then click on "Get Mail" and Claws will then freeze and crash.
Comment 1 Paul 2015-07-02 15:58:42 CEST
This is a problem in the tnefParse plugin. There is no crash when this plugin is not loaded.
Comment 2 Paul 2015-07-02 16:01:17 CEST
> To use
> them, simply cat one of them into a test user's incoming local mbox file
> then click on "Get Mail" and Claws will then freeze and crash.

Just FYI, it can be simpler than that. You just need to save them to one of the MH mailbox folders, naming them sequentially. Then open the folder.
Comment 3 Paul 2015-07-02 16:28:00 CEST
These attachments have two problems:
1. they have spurious data appended and prepended to them
2. when the spurious data is removed it turns out that they are not tnef after all.

Claws should not crash in this case, but these mails are broken. ytnef also segfaults on them.
Comment 4 MD 2015-07-02 16:46:15 CEST
(In reply to comment #1)
> This is a problem in the tnefParse plugin. There is no crash when this
> plugin is not loaded.

I just confirmed that you are 100% correct.  If I unload the tnefParse plugin, it has no problems with that message being in the incoming mailspool and it doesn't crash. I can also view the message with no problem.

However, after the Email makes it into Claws, if I then turn tnefParse back on/load it, then the moment I select the offending message for viewing, Claws will crash.  And since it is new, it will auto-view when clicking on that folder (inbox).

In our case, we need that plugin on for users to be able to access TNEF, and I have recompiled Claws in a way that prevents users from changing most of their settings (so they don't screw up things).  So users can't toggle plugins on and off at will.  I don't what we are going to do right now.  Still thinking about it.

Not sure I understand how this is resolved, though.... if it causes Claws to crash, isn't that a Claws or plugin problem that needs fixing?

Anyway, thanks for the quick and accurate response!!
Comment 5 MD 2015-07-02 17:13:52 CEST
(In reply to comment #3)
> These attachments have two problems:
> 1. they have spurious data appended and prepended to them

You have access to cooler tools than I :)

> 2. when the spurious data is removed it turns out that they are not tnef
> after all.

That somehow doesn't surprise me.  I can't imagine how the spurious data got in there.
 
> Claws should not crash in this case, but these mails are broken. ytnef also
> segfaults on them.

I tried just a "cat winmail.dat | tnef -t" command on one of them and it doesn't segfault or crash, but it didn't return anything on stdout either like it is supposed to (unless I add the --debug).  So it is certainly munged.

I did some additional research, unfortunately we *HAVE* to use the tnef plugin, because outside people really do send us stuff in that horrible method (rather than just attaching files directly).  I am really caught between a rock and a hard place now.

Would this issue really only be with the tnef plugin or ALSO with claws?  Yes, the plugin depends on ytnef which is crashing, and that causes the plugin to fail... that is one bug (which I think is higher than "normal" priority).  But Claws should not crash in this case, wouldn't that be another bug?  Should I create a new one for that?
Comment 6 Paul 2015-07-02 17:16:06 CEST
I relented and re-opened the bug, so no need for you to create a new one.
Comment 7 Andrej Kacian 2015-07-04 10:39:32 CEST
I took a look at the TNEF parsing code, and unfortunately, the code we have from libytnef is apparently written by someone who does not believe in possibility of people sending malformed data.

I tried to add in some checks to prevent crashing, but after a bit of time, I realized I would have to overhaul the entire code, and I do not really have time nor motivation to do that.

At this point, I wonder if dropping the tnef_parse plugin wouldn't be a better idea, since I can't rule out a possibility of a security hole, via a specifically crafted attachment. Instead of a crash, your computer might get compromised.
Comment 8 MD 2015-07-04 14:12:12 CEST
(In reply to comment #7)
> I took a look at the TNEF parsing code, and unfortunately, the code we have
> from libytnef is apparently written by someone who does not believe in
> possibility of people sending malformed data.

Thanks for looking into it
 
> I tried to add in some checks to prevent crashing, but after a bit of time,
> I realized I would have to overhaul the entire code, and I do not really
> have time nor motivation to do that.

There was a comment from a Mageia mailing list about security when I was doing other research.  What they said was that the designer of the plugin integrated decade-old code from ytnef and it has never been updated.  I wonder if it is worth looking at the current version of ytnef and seeing if this is already addressed and could be integrated.

> At this point, I wonder if dropping the tnef_parse plugin wouldn't be a
> better idea, since I can't rule out a possibility of a security hole, via a
> specifically crafted attachment. Instead of a crash, your computer might get
> compromised.

I actually wrote up a large bug report about security and then decided not to send it because I thought bugzilla was not the proper conduit.  There was a critical vulnerability in ytnef that could allow just what you said.  They reported it upstream to Claws and the plugin was quickly patched in GIT.  The issue I have is that 3.11.1 has been out for over 8 months now and no new version has been released that contains the patch.  I actually applied the patch manually (it is just one line of code) in my copy of 3.11.1 but how many people downloading are going to know about this potential problem?

I can't stress enough how important the tnef plugin is to business users (such as us).  People can and do send attachments using it, and we have to access them.  We can't change what others do, but have to deal with their decisions and my users would have no clue how to deal with tnef without the plugin.... all they would see is "winmail.dat".  I have looked into finding some alternative and I can't find anything that will not create a nightmare.  I wish I were a programmer, because I would gladly try to assist with fixing whatever is an issue.

So now there are three problems, as I see it:

1) A plugin should not cause Claws to crash.  Main claws perhaps has a design problem with regards to plugins.  (And I still think that is probably worthy of an additional/different discussion).

2) The tnef plugin can crash.  There is a flaw in the plugin.  At a minimum it should completely skip parsing something it doesn't understand or something that generates an internal error, leaving just winmail.dat.

3) Claws needs to be updated to a new numbered version from GIT ASAP to present a security patch that is already there.

I want to thank all the developers and maintainers of Claws for making such a great Email client.  All these years and there is nothing that can match it in so many ways!
Comment 9 Michael Rasmussen 2015-07-04 15:05:56 CEST
(In reply to comment #8)
> 
> I can't stress enough how important the tnef plugin is to business users
> (such as us).  People can and do send attachments using it, and we have to
> access them.  We can't change what others do, but have to deal with their
> decisions and my users would have no clue how to deal with tnef without the
> plugin.... all they would see is "winmail.dat".  I have looked into finding
> some alternative and I can't find anything that will not create a nightmare.
> I wish I were a programmer, because I would gladly try to assist with fixing
> whatever is an issue.
> 
I have tried decoding your attached problem emails with this library: http://www.pldaniels.com/opentnef/

However quit old it does seem to run without crashing and outputting a correct error message:

Problem1:
$ ./opentnef -i /tmp/problem1 --debug
tnef.c:700:TNEF_main:DEBUG: Start, decoding /tmp/problem1
tnef.c:752:TNEF_main:DEBUG: Read 21447 bytes
tnef.c:628:TNEF_decode_tnef:DEBUG: Start. Size = 21447
tnef.c:642:TNEF_decode_tnef:WARNING: Bad TNEF signature, expecting 223e9f78 got 6d6f7246
tnef.c:649:TNEF_decode_tnef:DEBUG: TNEF Attach Key: 7320
tnef.c:658:TNEF_decode_tnef:DEBUG: TNEF - Commence reading attributes
tnef.c:661:TNEF_decode_tnef:DEBUG: Offset = 0x6
tnef.c:433:TNEF_read_attribute:DEBUG: Reading Attribute...
tnef.c:440:TNEF_read_attribute: Reading Size...
tnef.c:468:TNEF_read_attribute:DEBUG: Reading Checksum...(offset 6, bytes=778399338)
tnef.c:212:TNEF_read_16:ERROR: Attempting to read past end
tnef.c:472:TNEF_read_attribute:DEBUG: Decoding attribute 1785225586
tnef.c:676:TNEF_decode_tnef:DEBUG: Done.
tnef.c:774:TNEF_main:DEBUG: finished decoding.

Problem2:
$ ./opentnef -i /tmp/problem2 --debug
tnef.c:700:TNEF_main:DEBUG: Start, decoding /tmp/problem2
tnef.c:752:TNEF_main:DEBUG: Read 20233 bytes
tnef.c:628:TNEF_decode_tnef:DEBUG: Start. Size = 20233
tnef.c:642:TNEF_decode_tnef:WARNING: Bad TNEF signature, expecting 223e9f78 got 6d6f7246
tnef.c:649:TNEF_decode_tnef:DEBUG: TNEF Attach Key: 6d20
tnef.c:658:TNEF_decode_tnef:DEBUG: TNEF - Commence reading attributes
tnef.c:661:TNEF_decode_tnef:DEBUG: Offset = 0x6
tnef.c:433:TNEF_read_attribute:DEBUG: Reading Attribute...
tnef.c:440:TNEF_read_attribute: Reading Size...
tnef.c:468:TNEF_read_attribute:DEBUG: Reading Checksum...(offset 6, bytes=1081697653)
tnef.c:212:TNEF_read_16:ERROR: Attempting to read past end
tnef.c:472:TNEF_read_attribute:DEBUG: Decoding attribute 1767989806
tnef.c:676:TNEF_decode_tnef:DEBUG: Done.
tnef.c:774:TNEF_main:DEBUG: finished decoding.
Comment 10 Michael Rasmussen 2015-07-04 15:09:54 CEST
I can also be noted that the ytnef library in Debian Sid does not crash either so maybe the plugin could simply link against the ytnef library instead of using the current one included in the plugin?
Comment 11 Andrej Kacian 2015-07-04 15:49:10 CEST
(In reply to comment #10)
> I can also be noted that the ytnef library in Debian Sid does not crash
> either so maybe the plugin could simply link against the ytnef library
> instead of using the current one included in the plugin?

I tried using the source files from recent ytneflib from github, but it did not fix anything - the part of ytnef.c where the crash happens is still the same. I'm not sure if linking dynamically to the same code, instead of including it in the plugin, would make any difference.
Comment 12 Michael Rasmussen 2015-07-04 16:12:47 CEST
(In reply to comment #11)
> 
> I tried using the source files from recent ytneflib from github, but it did
> not fix anything - the part of ytnef.c where the crash happens is still the
> same. I'm not sure if linking dynamically to the same code, instead of
> including it in the plugin, would make any difference.

I have just tried with the plugin linked against the provided libytnef library in Debian Sid with the same result.

To solve this issue it seems with need to completely abandon libytnef and use another library or make a bug report upstream hoping for a fix?
Comment 13 Charles Curley 2015-07-04 16:43:49 CEST
(In reply to comment #8)
> I have looked into finding
> some alternative and I can't find anything that will not create a nightmare.

I can't help with the nightmares, sorry. Here is a list of Debian
packages with "tnef" in their descriptions. Some might be useful.

root@iorich:~# apt-cache search tnef
claws-mail-tnef-parser - TNEF attachment handler for Claws Mail
libktnef4 - library for handling TNEF data
libapache-poi-java - Apache POI - Java API for Microsoft Documents
libapache-poi-java-doc - Apache POI - Java API for Microsoft Documents (Documentation)
libconvert-tnef-perl - Perl module to read TNEF files
libytnef0 - improved decoder for application/ms-tnef attachments
libytnef0-dev - improved decoder for application/ms-tnef attachments
libqtmessaging1 - Qt Mobility Messaging module
tnef - Tool to unpack MIME application/ms-tnef attachments
root@iorich:~#
Comment 14 MD 2015-07-04 21:59:00 CEST
(In reply to comment #13)
> (In reply to comment #8)
> > I have looked into finding
> > some alternative and I can't find anything that will not create a nightmare.
> 
> I can't help with the nightmares, sorry. Here is a list of Debian
> packages with "tnef" in their descriptions. Some might be useful.

There are working alternatives for something outside of Claws, but the "nightmare" part is dealing with users, who will not understand that "winmail.dat" is actually some type of funky archive and they have to open it (using some type of external app-launching script I would presumably create).  (I have a hard enough time with users and plain .zip files).  It doesn't help that utilities that go by extension will not be impressed by ".dat" (geeze, couldn't they have at least used ".tnef" or something sane??)

The Claws TNEF plugin is really slick because it somehow decodes what is in the TNEF and presents it to the user as regular attachments.  It is very intuitive.

I should emphasize that although it is annoying that there are tnef attachments that will crash the plugin, my issue is Claws crashing with it and the subsequent inability to use Claws anymore without Admin assistance.  If "bad"/failed tnef messages were somehow left undecoded or skipped (imported/displayed as-is with winmail.dat left alone), that would be fine with me (since for now it seems like only a few of those cause this issue).
Comment 15 users 2015-07-14 20:58:02 CEST
Changes related to this bug have been committed.
Please check latest Git and update the bug accordingly.
You can also get the patch from:
http://git.claws-mail.org/

++ ChangeLog	2015-07-14 20:58:02.762986843 +0200
http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=ae01a131cecb486b95053df8cb2738404b518ae6
Merge: 9bd8db0 463abe4
Author: Colin Leroy <colin@colino.net>
Date:   Tue Jul 14 20:58:02 2015 +0200

    Merge branch 'master' of file:///home/git/claws

http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=463abe4effd5dedc7e41332a6f9cbe185f0bb738
Author: Michael Rasmussen <mir@datanom.net>
Date:   Tue Jul 14 20:57:00 2015 +0200

    Added FreeBSD patch, see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=167460. Fixes claws-mail bug #3457
Comment 16 MD 2015-07-14 23:27:22 CEST
(In reply to comment #15)
> Changes related to this bug have been committed.
> [...]
>     Added FreeBSD patch, see
> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=167460. Fixes claws-mail
> bug #3457

From the link: "devel/libytnef/ytnef.c has incomplete handling of the PT_CLSID type and will spin off and crash applications attempting to use it. Patch attached properly handles the GUID data structure via hardcoded magic values."

Wow- that is awesome!!  Thanks!

Two questions... has it been tested yet (against the samples)?  And does any of this still merit opening a new bug that a plugin can cause Claws to crash?  (I don't want to make waves if it is really not possible to prevent Claws from crashing when a plugin does... I don't understand the issues that might be involved so forgive me if my asking seems insane).
Comment 17 Michael Rasmussen 2015-07-15 08:39:39 CEST
(In reply to comment #16)
> Two questions... has it been tested yet (against the samples)?  And does any
> of this still merit opening a new bug that a plugin can cause Claws to
> crash?  (I don't want to make waves if it is really not possible to prevent
> Claws from crashing when a plugin does... I don't understand the issues that
> might be involved so forgive me if my asking seems insane).

copy/paste from my mail to dev-list:
I have just pushed patch. It seems to have fixed our bug #3457 since I
am now able to open the 2 problem mails from the bug report without
crashing claws. Apart from not crashing claws I am also able to read
the contents and have valid contents displayed:

tnef_parse.c:281:Tnef parser parsing part (12471).
tnef_parse.c:283:content: /home/mir/.claws-mail/mimetmp/claws.4JZ61X
Attempting to parse /home/mir/.claws-mail/mimetmp/claws.4JZ61X...
Aid Owner: [4] 
Request Response: [2] 
    message/rfc822 (offset:0 length:21447 encoding: 6)
        multipart/mixed (offset:3351 length:18096 encoding: 6)
            text/plain (offset:3505 length:817 encoding: 3)
            multipart/mixed (offset:0 length:12471 encoding: 2)
Comment 18 MD 2015-07-15 16:49:40 CEST
(In reply to comment #16)

> Two questions... has it been tested yet (against the samples)?

I manually patched my copy of 3.11.1 and it compiled OK.  So I, too, tested it with the sample tnef files and it works without crashing now.  When I select the bad message, the plugin just silently fails and so it doesn't show ANY tnef attachment or winmail.dat attachment at all (and doesn't crash).  Might be nice if it showed the winmail.dat instead of nothing, but the important part is the not crashing :)