New ZIP walk leaks content of nested ZIP files to /tmp

[robert-scheck]

With 7d018ef, the new ZIP walk was introduced. Unfortunately it also leaks the content of nested ZIP files to /tmp directory – world readable by default. Reproducer:

1. cd MacroMilter/test_mails/
2. zip test1.zip zipwithinfectedandnotinfectedword.zip
3. zip test2.zip test1.zip
4. zip test3.zip test2.zip
5. zip test4.zip test3.zip
6. zip test5.zip test4.zip
7. zip test6.zip test5.zip
8. Try to mail test6.zip (fails as expected)
9. ls -l /tmp/*.zip

$ ls -l /tmp/*.zip
-rw-r--r-- 1 macromilter macromilter 12970 31. Okt 03:13 /tmp/test1.zip
-rw-r--r-- 1 macromilter macromilter 13138 31. Okt 03:13 /tmp/test2.zip
-rw-r--r-- 1 macromilter macromilter 13306 31. Okt 03:13 /tmp/test3.zip
-rw-r--r-- 1 macromilter macromilter 13474 31. Okt 03:13 /tmp/test4.zip
-rw-r--r-- 1 macromilter macromilter 13642 31. Okt 03:13 /tmp/test5.zip
-rw-r--r-- 1 macromilter macromilter 12746 31. Okt 03:13 /tmp/zipwithinfectedandnotinfectedword.zip
$ 

From my point of view, MacroMilter should in any case clean up afterwards and it should not directly use /tmp, but a collision-free directory. Hardcoding e.g. /tmp/macromilter might lead to new issues (a local user could create that directory with modified permissions to gain the content), thus I would suggest something like either /var/lib/macromilter/tmp (similar like amavisd-new does) or the usage of something similar like mktemp (but in Python), while still applying more restrictive directory permissions.

A regular user might mitigate this issue by setting in /usr/lib/systemd/system/macromilter.service e.g.

[Service]
PrivateTmp=true

which however still keeps the extracted files until logrotate (while nested ZIP files are hopefully not that common).

[decalage2]

See the tempfile module to create temporary files more securely, and to clean up afterwards: https://docs.python.org/3/library/tempfile.html https://docs.python.org/2.7/library/tempfile.html

[sbidy]

Now the zipwalk removes the tmp files, also add a umask to create the tmp files only for the service user (0600). In the actual implementation it is not (easy) possible to use the tempflile lib.

I put a note to the backlog to re-implement this function in a later release. But the actual implementation works as expected (for zip archives). I'm open for some suggestions or implementation ideas 😉 .

[robert-scheck]

I am sorry, but the actual implementation is securitywise still bad, given the /tmp directory is directly consumed rather a safe temporary directory by using something like mktemp (whatever the Python replacement for that is) or an application specific private directory such as /var/lib/macromilter/tmp (similar like amavisd-new does). Leak and crash example based on the initial situation above (works also as unprivileged local user) applicable to latest commit fdd6828 in testing branch:

7b. touch /tmp/hardlink.zip 7c. chmod 666 /tmp/hardlink.zip 7d. ln /tmp/hardlink.zip /tmp/test2.zip

Content is leaked to world readable /tmp/hardlink.zip given a predictable filename.

[sbidy]

OK, I didn't noticed that problem. Maybe the mkstemp brings a better solution for that?

[sbidy]

I've updated the method with the mkstemp function. In my opinion it fixed the mentioned problem.

https://github.com/sbidy/MacroMilter/blob/828ee301fb3d5ee77c6c4fd889a11a5b7f6e0273/macromilter/macromilter.py#L377

[robert-scheck]

I'm not a python guy…how does this work logically now? Reading documentation tempfile.mkstemp() creates a temporary file while I would have assumed using tempfile.mkdtemp() to create a temporary working directory for unzipping into (and removing this temporary directory afterwards).

[sbidy]

I used the mkstemp to create new temp files to obfuscate the real filename and made it unpredictable. mkdtemp only creates a temp folder but doesn't change the real file name. I think booth are possible solution but the mkstemp seems to me more secure.

Now, the zipwalk method reads the OLE-files or archives and saves it to a random named temp file. It also reads only the ole file objects to a generator and after the walk finished returns it. After the generator was return, the files will be deleted in the /temp folder.

[sbidy]

implemented and released with 3.6