fix strdup() on possibly unterminated string

[pauldreik]

A buffer read overflow may happen at file.c line 236 where strdup() is invoked. strdup() will just search until it finds a null terminator. If there was none, it will continue past the heap allocated memory. (Update 20191110: there is another one in mapi_attr.c, see https://github.com/verdammelt/tnef/pull/40/commits/3ae8b93746aa6403420b9907886993ceaa6705e3)

The effect of this read overflow is that either the application crashes from a segfault, or "random" data is being duplicated, the effect of which I do not know. Writing a file with garbage suffixed to the name, perhaps, but there seem to be some kind of sanitation in path.c preventing that.

Update: there is another similar situation in mapi_attr.c, also fixed in this pull request.

An example input that triggers the behaviour is base64 encoded here:

base64 <mini
eJ8+IjAwATAwMDAEAAAAAAABAAEAATAwMDAIAAAA5AQAAAAAAADoAAEwMDAwGAAAAElQTS5NaWNy
b3NvZnQgTWFpbC5Ob3RlADEIATAwMDAhAAAANDAwMTdGQ0ZEMDgxRDMxMUE3QTUwMDA4QzcxQkNB
OEQAJAcBMDAwMBgAAABJUE0uTWljcm9zb2Z0IE1haWwuTm90ZQAxCAEwMDAwDgAAAM8HCgANABYA
MwAzAAMAbAEBMDAwMA4AAADPBwoADQAWADEACQADAEABATAwMDAKAAAAdHdvIGZpbGVzAI0DATAw
MDACAAAAAgACAAEwMDAwuAUAADgAAAADAP0/5AQAAEAAOQCAgBGw7hW/AR4AMUABAAAAFgAAAHNp
bXBzb25Ad29ybGQuc3RkLmNvbQAAAAMAGkAAAAEAHgAwQAEAAAAWAAAAc2ltcHNvbkB3b3JsZC5z
dGQuY29tAAAAAwAZQAAAAQADAN4/r28AAB4AcAABAAAACgAAAHR3byBmaWxlcwAAAAIBcQABAAAA
FgAAAAG/Fe8OHc9/AUGB0BHTp6UACMcbyo0AAB4A/lcBAAAAFQAAAE5BSVNDQU5ORURQT1NUT0ZG
SUNFAAAAAAsA8hABAAAAAgHzPwEAAAAAAAAAAgH0PwEAAAAAAAAAAgE/AAEAAABRAAAAAAAAANyn
QMjAQhAatLkIACsv4YIBAAAAAAAAAC9PPUNPTVBVV0FSRS9PVT1OVU1FR0EgTEFCL0NOPVJFQ0lQ
SUVOVFMvQ049TVNJTVBTT04AAAAAHgB1AAEAAAAFAAAAU01UUAAAAAAeAHYAAQAAABgAAABtYXJr
LnNpbXBzb25AbnVtZWdhLmNvbQAeAEAAAQAAAA4AAABTaW1wc29uLCBNYXJrAAAAHgA0QAEAAAAJ
AAAATVNJTVBTT04AAAAAAgFRAAEAAAA4AAAARVg6L089Q09NUFVXQVJFL09VPU5VTUVHQSBMQUIv
Q049UkVDSVBJRU5UUy9DTj1NU0lNUFNPTgADABtAAAAAAAIBQwABAAAAUQAAAAAAAADcp0DIwEIQ
GrS5CAArL+GCAQAAAAAAAAAvTz1DT01QVVdBUkUvT1U9TlVNRUdBIExBQi9DTj1SRUNJUElFTlRT
L0NOPU1TSU1QU09OAAAAAB4AdwABAAAABQAAAFNNVFAAAAAAHgB4AAEAAAAYAAAAbWFyay5zaW1w
c29uQG51bWVnYS5jb20AHgBEAAEAAAAOAAAAU2ltcHNvbiwgTWFyawAAAB4ANUABAAAACQAAAE1T
SU1QU09OAAAAAAIBUgABAAAAOAAAAEVYOi9PPUNPTVBVV0FSRS9PVT1OVU1FR0EgTEFCL0NOPVJF
Q0lQSUVOVFMvQ049TVNJTVBTT04AAwAcQAAAAAALAFcAAQAAAAsAWAAAAAAACwBZAAEAAAACAUcA
AQAAAAAAAAACAfk/AQAAAEAAAAAAAAAAgSsfpL6jEBmdbgDdAQ9UAgAAAQBNYXJrIFNpbXBzb24A
U01UUABzaW1wc29uQHdvcmxkLnN0ZC5jb20AHgD4PwEAAAANAAAATWFyayBTaW1wc29uAAAAAB4A
OEABAAAAFgAAAHNpbXBzb25Ad29ybGQuc3RkLmNvbQAAAAIB+z8BAAAAUQAAAAAAAADcp0DIwEIQ
GrS5CAArL+GCAQAAAAAAAAAvTz1DT01QVVdBUkUvT1U9TlVNRUdBIExBQi9DTj1SRUNJUElFTlRT
L0NOPU1TSU1QU09OAAAAAB4A+j8BAAAADgAAAFNpbXBzb24sIE1hcmsAAAAeADlAAQAAAAkAAABN
U0lNUFNPTgAAAABAAAcwuPYdDu8VvwFAAAgwmMHyEO8VvwEeAD0AAQAAAAEAAAAAAAAAHgAdDgEA
AAAKAAAAdHdvIGZpbGVzAAAAAgHUPwEAAAAAAAAAHgA1EAEAAAAyAAAAPDE0MzQxLjE3NTczLjU2
MDc2MS4zNjg1MTJAbG9jYWxob3N0LmxvY2FsZG9tYWluPgAAAB4AORABAAAAAQAAAAAAAAAeADYQ
AQAAAAEAAAAAAAAAAgFoQAEAAAAAAAAAAgFpQAEAAAAAAAAAAwA2AAAAAAALACkAAAAAAAsAIwAA
AAAAAwAGEAAAAAADAAcQAAAAAAMAEBAAAAAAAwAREAAAAAAeAAgQAQAAAAEAAAAAAAAAAgF/AAEA
AAAyAAAAPDE0MzQxLjE3NTczLjU2MDc2MS4zNjg1MTJAbG9jYWxob3N0LmxvY2FsZG9tYWluPgAA
AFUdAgKQMDAOAAAAAQD/////IAAgAAAAAAA9BAIwMDAwDgAAAM8HCgANABYAMwAuAAMAZwECMDAw
MA4AAADPBwoADQAWADMALgADAGcBAjAwMDAIAAAAQVVUSE9SUwAmAgIQgDAw9AAAAAogICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICBBdXRob3JzIG9mIHRuZWYKICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgPT09PT09PT09PT09PT09CiAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAKKiBNYXJrIFNpbXBzb24gICAgICAgICAgICBkYW1uZWRAd29ybGQuc3RkLmNvbQoK
TWFueSB0aGFuayBnbyB0byB0aGUgb3JpZ2luYWwgYXV0aG9yOiBUaG9tYXMgQm9sbCAodGJAYm9s
bC5jaCkuCgrTOQ==

To prove the bug, run

base64 -d >mini # paste input from above
valgrind src/tnef mini

and get (source lines are not accurate, I ran it on the patched version, with the fix disabled)

paul@tonfisk:~/code/delaktig/tnef$ valgrind src/tnef  mini
==506== Memcheck, a memory error detector
==506== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==506== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==506== Command: src/tnef mini
==506== 
==506== Invalid read of size 1
==506==    at 0x4C32D04: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==506==    by 0x4ED99AD: strdup (strdup.c:41)
==506==    by 0x10B058: file_add_attr (file.c:236)
==506==    by 0x10CD5E: parse_file (tnef.c:334)
==506==    by 0x10990C: main (main.c:380)
==506==  Address 0x522f4a4 is 0 bytes after a block of size 244 alloc'd
==506==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==506==    by 0x10A6A3: attr_read (attr.c:260)
==506==    by 0x10CD28: read_object (tnef.c:70)
==506==    by 0x10CD28: parse_file (tnef.c:277)
==506==    by 0x10990C: main (main.c:380)
==506== 
==506== Invalid read of size 1
==506==    at 0x4C36788: memmove (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==506==    by 0x10B058: file_add_attr (file.c:236)
==506==    by 0x10CD5E: parse_file (tnef.c:334)
==506==    by 0x10990C: main (main.c:380)
==506==  Address 0x522f4a4 is 0 bytes after a block of size 244 alloc'd
==506==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==506==    by 0x10A6A3: attr_read (attr.c:260)
==506==    by 0x10CD28: read_object (tnef.c:70)
==506==    by 0x10CD28: parse_file (tnef.c:277)
==506==    by 0x10990C: main (main.c:380)
==506== 
%0A                              Authors of tnef%0A                              ===============%0A                                     %0A%2A Mark Simpson            damned@world.std.com%0A%0AMany thank go to the original author%3A Thomas Boll (tb@boll.ch).%0A%0A: File name too long
==506== 
==506== HEAP SUMMARY:
==506==     in use at exit: 1,395 bytes in 6 blocks
==506==   total heap usage: 42 allocs, 36 frees, 10,971 bytes allocated
==506== 
==506== LEAK SUMMARY:
==506==    definitely lost: 0 bytes in 0 blocks
==506==    indirectly lost: 0 bytes in 0 blocks
==506==      possibly lost: 0 bytes in 0 blocks
==506==    still reachable: 1,395 bytes in 6 blocks
==506==         suppressed: 0 bytes in 0 blocks
==506== Rerun with --leak-check=full to see details of leaked memory
==506== 
==506== For counts of detected and suppressed errors, rerun with: -v
==506== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

[verdammelt]

@pauldreik Thanks for this patch. I'm away on vacation at the moment so I'm not going to have a chance to look into this more deeply until next week. Can you check into why the build is failing? Looks like one of the automated tests is failing. Thanks.

[pauldreik]

It was the memory allocation test which failed, because it compared the output to the last time. I reworked it a bit, now it passes.

[pauldreik]

Hi @verdammelt , have you had a chance to look at this yet? Thanks, Paul

[verdammelt]

Instead of changing the interface of xcalloc/xmalloc can we just have that the extra ADDNULL macros which +1 to the provided length - or in the two places called simply +1 there? I'd rather just do that.

[pauldreik]

I started with the +1 approach, but that is dangerous since it creates a possibility for integer overflow on the call site. See here: https://github.com/verdammelt/tnef/pull/40/commits/7109b9ec57c9f41362220a78ad488d92af61b987

Also, it will make the tests break since they check the debug output compared to the checked in output from an earlier version.

Then I moved the "+1 functionality" inside the xcalloc call: https://github.com/verdammelt/tnef/pull/40/commits/375d20fe753e27c6798f5d73bab647f2b27e3bdc The functionality for safely checking for integer overflow is inside that function.

I understand you do not like it, it is pretty ugly, but at least it is correct.

You can implement this however you prefer, I am happy as long as the end result is free of undefined behaviour, buffer overflows and integer overflows!

[verdammelt]

ah, I think I see your point. I'll merge.

[verdammelt]

Also @pauldreik thanks for the PRs...

[pauldreik]

My pleasure! Thanks for maintaining this useful tool.

[pauldreik]

@verdammelt can you comment on what the consequence could be of this bug? I would say opening a crafted file could lead to a file with a weird filename may be created on the system, but could that weird file name end up being in another directory (say ../../.ssh/authorized_keys if the heap could be prepared)?

[verdammelt]

Yes, it seems perhaps with a specially constructed file you could get files written to unexpected locations or with contents not as expected.

I don't know how easy it would be to do either - but it seems that this bug would make that possible.

[pauldreik]

This has been assigned CVE-2019-18849