1.4.13: Assertion `a->type == szMAPI_STRING' / `(idx+(a->names[i].len*2)) <= len' failed

[isoftjs]

Since Version 1.4.13 we get assertion failures while parsing a winmail.dat containing unicode strings.

Example winmail.dat with debug output from 1.4.12 and 1.4.13: https://gist.github.com/isoftjs/e7eb450b62abbd426b055a540b3685d2

---

Using 1.4.12:

./tnef-1.4.12/src/tnef --list --file winmail.dat
example.dat |   example.dat

Using 1.4.13:

./tnef-1.4.13/src/tnef --list --file winmail.dat
tnef: file.c:176: file_add_mapi_attrs: Assertion `a->type == szMAPI_STRING' failed.

When adding szMAPI_UNICODE_STRING to the assertion the file can be parsed:

./tnef-1.4.13/src/tnef --list --file winmail.dat
example.dat |   example.dat

With some debug printing after the changed assertion:

./tnef-1.4.13/src/tnef --list --file winmail.dat
file_add_mapi_attrs@178: file->name=example.dat, a->type=31
example.dat |   example.dat

---

At first the issue was occurring with an incoming mail (which contains some sensitive information) with another assertion failure:

Using 1.4.12:

./tnef-1.4.12/src/tnef --list --file winmail.dat
Filename Jan. 2017 - Kopie.pdf  |   Filename Jan. 2017 - Kopie.pdf
Filename Dez. 2016 - Kopie.pdf  |   Filename Dez. 2016 - Kopie.pdf

Using 1.4.13:

./tnef-1.4.13/src/tnef --list --file winmail.dat
tnef: mapi_attr.c:216: mapi_attr_read: Assertion `(idx+(a->names[i].len*2)) <= len' failed.

Then the assertion failure from the gist above will occur (file.c:176: file_add_mapi_attrs: Assertion a->type == szMAPI_STRING failed.)

With some debugging:

./tnef-1.4.13/src/tnef --list --file winmail.dat
mapi_attr_read@216: idx=8968, a->names[i].len=30, len=9096
mapi_attr_read@216: idx=9048, a->names[i].len=32, len=9096
file_add_mapi_attrs@178: file->name=Filename Jan. 2017 - Kopie.pdf, a->type=31
Filename Jan. 2017 - Kopie.pdf  |   Filename Jan. 2017 - Kopie.pdf
file_add_mapi_attrs@178: file->name=Filename Dez. 2016 - Kopie.pdf, a->type=31
Filename Dez. 2016 - Kopie.pdf  |   Filename Dez. 2016 - Kopie.pdf

[alteholz]

The same issues appeared in the Debian BTS at: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=857342

Do all those winmail.dat contain wrong data?

[verdammelt]

I'll need to investigate further. When originally developing TNEF I hit cases where the specification of MS-TNEF didn't match the reality of it. Perhaps this is another case, or one where the specification has changed and TNEF assumes older behavior.

The assertion was put in place in the interest of security, of not interpreting data incorrectly which may lead to data corruption or out-of-bounds reads etc. I don't want to relax the restriction without careful checking.

[verdammelt]

After some quick googling it appears that field type #31 maybe "unicode string" (type #30 is "string"). I'll check the data files next and if that appears to be the case we'll need to add this new type and handle it (probably in all places where string is currently handled).

@isoftjs & @alteholz can the attached data file here, or from the debian project be used by me as part of my test suite? The file would be committed to the code base as an example of this problem to ensure against regressions.

[verdammelt]

There are two problems happening here. The attached data file shows the problem file.c:176: file_add_mapi_attrs: Assertiona->type == szMAPI_STRING' failed`.

The datafile from the linked to Debian ticket is not the same failure.

[verdammelt]

The second problem was amusing. It appears that for 12+ years TNEF has had a useful function for reading in unicode strings which was not used in mapi_read_attrs (in the GUID section). That code did its own odd hacky thing which is where the problem lay.

Using the useful utility appears to be fixing the problem.

[isoftjs]

@isoftjs & @alteholz can the attached data file here, or from the debian project be used by me as part of my test suite? The file would be committed to the code base as an example of this problem to ensure against regressions.

The attachment in the winmail.dat from the gist contains just some data from /dev/urandom so i'm OK with including it in the test suite.

[alteholz]

@isoftjs & @alteholz can the attached data file here, or from the debian project be used by me as part of my test suite? The file would be committed to the code base as an example of this problem to ensure against regressions.

The original poster allowed to use the file from the Debian BTS in the test suite as well.

[verdammelt]

@alteholz excellent. I will hopefully have time this weekend to get those tests set up and a new release created.