[Patch] libytnef buffer overflow

[Nikolaus Filus]

related to bug #608085 (even if expired!)

libytnef has several problems and makes ytnef as well as the evolution plugins crash. Found a working patch at redhat bugzilla #375822. Please include in ubuntu

[Paul (paul-redhat-bugs)]

Description of problem:
There are a number of issues with the code of ytnef.c

Version-Release number of selected component (if applicable):
1.5.5

How reproducible:
By using libytnef.

Steps to Reproduce:
1. Run the code (eg, run evolution with the experimental TNEF Attachment Decoder plugin).
2.
3.

Actual results:
Leaks. Invalid reads.

Expected results:
None of the above.

Additional info:
I'll try to attach a patch shortly.

[Paul (paul-redhat-bugs)]

Created attachment 375822
Fix several issues in ytnef.c

0) This patch is the result of a discussion started at a evolution bugreport: https://bugzilla.gnome.org/show_bug.cgi?id=602177

1) The patch tries to fix these issues:
- SwapWord(), SwapDWord() and SwapDDWord() can return a pointer to a local value (in big endian mode). They can also be simplified (though some might argue with my idea of simple here);
- TNEFRendData() reads a 14 byte (packed) value in a TNEF stream as if it were a 16 byte (unpacked) struct;
- leaks related to MAPIProps->properties->propnames;
- buffer overflow in comp_Prebuf.data (in DecompressRTF()), fixed by a minor code reorganization;
- one "if branch" in DecompressRTF() should have a return (though I have no way to test what that return should be).

2) Note that the code (even with this patch) generates a lot of compiler warnings. I haven't really looked at those.

3) Upstream looks dead to me. If (something like) the patch is accepted will the packager contact other packagers (as the patch does fix a buffer overflow)? Or should (something like) the patch just be dropped as upstream's page (at sf.net)?

[Paul (paul-redhat-bugs)]

(In reply to comment #1)
> 1) The patch tries to fix these issues:
> - SwapWord(), SwapDWord() and SwapDDWord() can return a pointer to a local
> value (in big endian mode). They can also be simplified (though some might
> argue with my idea of simple here);
> - TNEFRendData() reads a 14 byte (packed) value in a TNEF stream as if it were
> a 16 byte (unpacked) struct;
> - leaks related to MAPIProps->properties->propnames;
> - buffer overflow in comp_Prebuf.data (in DecompressRTF()), fixed by a minor
> code reorganization;
> - one "if branch" in DecompressRTF() should have a return (though I have no way
> to test what that return should be).

Furthermore:
- TNEFPriority() accesses a 2 byte WORD value as if it were a 4 byte DWORD value.

[Paul (paul-redhat-bugs)]

Created attachment 376060
[RFC] Address all current gcc (default) warnings

(In reply to comment #1)
> 2) Note that the code (even with this patch) generates a lot of compiler
> warnings. I haven't really looked at those.

- gcc will compile ytnef.c without any warnings with this patch (that should be applied on top of my previous patch).
- Most of the fixes are entirely trivial: one wonders why those changes have not been done earlier.
- I had to resort to casting a few times; as far as I know all casts should be safe (but a review whether that is actually correct would be much appreciated).
- I changed the definition of to_utf8(), which, sadly, is included in <ytneh.h>. Still seemed the best thing to do.
- Please note the few remarks I added about the current interface.

[Milan (milan-redhat-bugs)]

These are warnings I see on my 64bit machine.
> ytnef.c: In function 'TNEFFillMapi':
> ytnef.c:474: warning: format '%i' expects type 'int', but argument 2 has type
> 'long int'
> ytnef.c:475: warning: format '%i' expects type 'int', but argument 2 has type
> 'long int'
> ytnef.c:480: warning: format '%i' expects type 'int', but argument 2 has type
> 'long int'
> ytnef.c:481: warning: format '%i' expects type 'int', but argument 2 has type
> 'long int'

There is a format for them, like G_GUINT32_FORMAT, which might help here. I tested both patches and it otherwise looks good. I would recommend to use them personally.

[Paul (paul-redhat-bugs)]

(In reply to comment #4)

> There is a format for them, like G_GUINT32_FORMAT, which might help here. I
> tested both patches and it otherwise looks good. I would recommend to use them
> personally.

Maybe something like:

    #include <inttypes.h>
    printf("%" PRIi32 " bytes missing\n", size - (d-data));

(I can't remember ever having used this.)

[Milan (milan-redhat-bugs)]

whatever you prefer. Yet another possibility might be to use "%d" instead of "%i" and cast the value to "(int) (size - (d-data))". I do not suppose there will be too large files, and, more than that, it's for debug only anyway.

[Bug (bug-redhat-bugs)]

This bug appears to have been reported against 'rawhide' during the Fedora 13 development cycle.
Changing version to '13'.

More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

[Bug (bug-redhat-bugs)]

This message is a reminder that Fedora 13 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 13. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as WONTFIX if it remains open with a Fedora
'version' of '13'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version'
to a later Fedora version prior to Fedora 13's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that
we may not be able to fix it before Fedora 13 is end of life. If you
would still like to see this bug fixed and are able to reproduce it
against a later version of Fedora please change the 'version' of this
bug to the applicable version. If you are unable to change the version,
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

The process we are following is described here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

[Bug (bug-redhat-bugs)]

Fedora 13 changed to end-of-life (EOL) status on 2011-06-25. Fedora 13 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version.

Thank you for reporting this bug and we are sorry it could not be fixed.

[Nikolaus Filus (nfilus)]

sorry, the redhat bugzilla number ist not correct in the previous text (this was the attachment number).
https://bugzilla.redhat.com/show_bug.cgi?format=multiple&id=543965