Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jaromil opened this issue Oct 20, 2014 · 16 comments

Comments

@jaromil
Copy link
Member

jaromil commented Oct 20, 2014

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 jaromil changed the title CVEs request: Incorrect temporary file handling && silent code execution in Tomb, a commandline tool to easily operate encryption of secret data CVEs request: Incorrect temporary file handling && silent code execution Oct 20, 2014
@jaromil

This comment has been minimized.

Copy link
Member Author

jaromil commented Oct 20, 2014

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

This comment has been minimized.

Copy link
Contributor

hellekin commented Oct 20, 2014

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

This comment has been minimized.

Copy link
Member

boyska commented Oct 20, 2014

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

@hellekin

This comment has been minimized.

Copy link
Contributor

hellekin commented Oct 20, 2014

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

@jaromil

This comment has been minimized.

Copy link
Member Author

jaromil commented Oct 21, 2014

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

This comment has been minimized.

Copy link
Member

boyska commented Oct 21, 2014

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

This comment has been minimized.

Copy link
Member Author

jaromil commented Oct 21, 2014

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

This comment has been minimized.

Copy link
Member

boyska commented Oct 21, 2014

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

This comment has been minimized.

Copy link
Member Author

jaromil commented Oct 22, 2014

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

This comment has been minimized.

Copy link
Member

boyska commented Oct 22, 2014

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

This comment has been minimized.

Copy link
Member Author

jaromil commented Oct 22, 2014

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

This comment has been minimized.

Copy link
Member Author

jaromil commented Oct 22, 2014

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

This comment has been minimized.

Copy link
Member

boyska commented Oct 22, 2014

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

@jaromil

This comment has been minimized.

Copy link
Member Author

jaromil commented Oct 22, 2014

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 jaromil added this to the 2.0 milestone Oct 23, 2014
@jaromil jaromil added the in progress label Oct 23, 2014
@jaromil

This comment has been minimized.

Copy link
Member Author

jaromil commented Oct 26, 2014

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

This comment has been minimized.

Copy link
Member Author

jaromil commented Nov 14, 2014

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

@jaromil jaromil closed this Nov 14, 2014
@jaromil jaromil removed the in progress label Nov 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.