CVEs request: Incorrect temporary file handling && silent code execution

[jaromil]

Interesting analysis of vulnerability here http://www.openwall.com/lists/oss-security/2014/10/20/2

to be taken definitely in consideration and solved for the upcoming release.

I thought in the latest git HEAD the filename wasn't predictable, will look into this and eventually get rid of all tempfiles alltogether.

[jaromil]

The analysis raises two issues.

The second is incorrect, since the analyst has not read the actual tomb manpage: there is a notice about exec-hooks and the flag to deactivate them is -n. This flag is implemented since the very feature of hooks exist.

       -n     Skip processing of post-hooks and bind-hooks if found inside the
              tomb.  See the HOOKS section in this manual  for  more  informa‐
              tion.

The first needs further investigation, yet the current refactoring goes exactly into the direction of eliminating tempfiles. I foresee that only in case of the setkey command it might be impossible to work around the way cryptsetup takes arguments for the operation and may need our own C program for that. Yet I believe it is not a hot issue since it is not so easy to guess the result of $RANDOM in zsh, to make sure I will double the $RANDOM and remove the $pid. Also a notice about setkey being a delicate operation to be done on a trusted system with noone else logged in should be printed to users.

[hellekin]

What about making the temporary file relative to the logged-in user, and ensure that it does not exist prior to creating it? The touch is not necessary, especially as you can use umask 077 before mktemp. That would be better before switching to mkfifo.

[boyska]

can't tomb just use mktemp ? reinventing the wheel is often a bad idea...

[hellekin]

hmmm, right. I thought it was already. +1

[jaromil]

The reason for mktemp not being in use is again readability of the process. GNU coreutils has made an horrible job through the years changing flags and deprecating them to the point I've been always confused, to not even mention the incompatibility with busybox mktemp. Since a correct use of umask and touch sorts the same effect, adds less dependencies and produces more readable code, I prefer staying with that. If there is some magic that only mktemp can do then point it out, I'm not aware of any.

[boyska]

what do you mean by "readability"? I think that a simple, well-known, easy to understand command like mktemp is readable in code.

coreutils shouldn't even be considered a dependency, as it's present in 99.9% of linux boxes.

And of course there is not any magic in mktemp: see mktemp.c and its most important library tempname.c; it just makes some attempts to properly create a file with the desired flags. But it is well-tested, audited since ages. It does one thing and does it well, in UNIX spirit.

If you want to avoid the busybox madness, just check with mktemp -V | grep -q coreutils that the right version is in place.

[jaromil]

No, I want Tomb to work also in case mktemp and other coreutils functions are provided by busybox. And for that we can simply implement our own mktemp to avoid the problems I've mentioned. There is no advantage in using mktemp. By readability I mean that one can read what it does and is not so long to do so. The advantage of not using mktemp are clear (again, see above).

You can insist and I'll think over it, but please list the advantages of using mktemp then.

[boyska]

I think I've already listed. Specialized tools that do one thing well, that's the unix approach to both simplicity, readability and security. The approach of "implementing our own *" is not just counterintuitive (which is a subjective statement): it is bugged (unless you are really, really, serious at it and write code with lot of attention, write lot of unit tests and simulate all possible edge cases to prove that there is no issue; but I don't see anything like that). This is true for any piece of software, but should be especially true for security-related ones. Anyway, it's not a dispute, and I'm not active as a tomb developer anymore, so I won't insist too much: you must be comfortable with the code you write, in first place!

About busybox mktemp: on my archlinux box, using busyboxv1.21.1 and coreutils 8.23 mktemp -d -t tomb.XXXXX works in a compatible way (at a quick test, at least). The -p option is also compatible. The only thing to keep in mind is that option parsing is more rigid (bsd-style); but that behaviour is compatible with gnu coreutils anyway.

[jaromil]

I don't see anything particularly secure being done here http://code.metager.de/source/xref/gnu/coreutils/src/mktemp.c which transcends a correct sequence of checking the file existance, setting the umask and going on. Rather I see that a Zsh version of this function would be more readable, would be doing less string handling in C and most importantly wouldn't deliver to a small and external binary program a delicate function that has the password data passing in it. I'm not sure what is you threat model here, but mine includes substituting that mktemp binary file whose autenticity can be audited only having hashes at hand.

[boyska]

What does "anything particularly secure" means? It is correct code, tested and audited. There's no magic of course.

Also, please explain your threat model better. It is a common assumption that your system cannot be compromised; which means that executabels in /usr/bin must be authenticated. If they have been replaced by an attacker, then even tomb itself, or zsh, or cryptsetup could have been replaced. If you assume that your system is compromised, there's no possible security.

[jaromil]

If you have no tripwire in place and you are offline and you presume you might have been compromised you have a last chance by relying on less binary elements and them being readable as a "manual" proof of integrity against tampering. Of course this doesn't solves it for every component that can be tampered with, but it makes it closer to a possibility. The fact that the temporary file creation is a very delicate part for attacks makes mktemp an important component to be evaluated in these scenarios.

Thanks for your evaluation. I'm convinced I'll keep (and slightly fix) the shell implementation of mktemp.

[jaromil]

BTW one doesn't "stops being an active developer" or so. we are BROS, remember =^) somehow the software in which we write always ends up populating a good piece of our memory li software so' pure bei ricordi be capisse quando te va de fatte un giro sei benvenuto mica rompi il cazzo :^) e non e' tempo sprecato parla de ste cose. io cmq de quello che ho scritto so proprio convinto.

[boyska]

Well, it's been a long time since my last commit :)

[jaromil]

plz stay around to have a look at @hellekin next commit he is prepping a biggie and we agreed on code guidelines. hoping he doesn't do stuff like the last piece of "poetry" that I deleted man it was BORKED.

[jaromil]

This https://github.com/dyne/Tomb/tree/cleanup

Looks good to me, cleaning up a lot of code making it style consistent and readable. From there on we'll make sure the issues raised by this CVE are even less likely to occur, or not occurring at all. Plz have a review in your spare time if you can.

[jaromil]

The cleanup branch is merged now. Points raised by the CVE are addressed and solved.