Derek Fountain wrote:
The project over at Sourceforge (http://aa-project.sourceforge.net) appears to be dead, having had no update for 3 years. Yes, the project is dead. I e-mailed them about the same issue several months ago...
Your fix is pretty good, yet it still has a small security bug in it: The clear() procedure is used insecurely.
TDIR=${TMPDIR:-/tmp}/aview_$$ FIFO=$TDIR/aview$$.pgm
clear() { kill $! 2>/dev/null rm -f $FIFO 2>/dev/null rmdir $TDIR 2>/dev/null }
[...]
trap clear 0 (umask 077 && mkdir $TDIR) || { echo "Unable to create temp directory $TDIR" exit 1 } mkfifo $FIFO || { echo "Unable to create FIFO $FIFO" exit 1 }
The "trap clear 0" will call clear() when the program exits in order to clean up. An attacker can create a symlink /tmp/aview_$$ (he has to guess $$ of course). Mkdir will fail, your program exits and clear() is called. Clear() will follow the symlink! That means an attacker can delete every file called "aview$$.pgm" on the system. Not very likely that there is such a file, but still it is possible and may help with other exploits as you will see: You could create a symlink to /root/secretdir/../../tmp and place your own aview$$.pgm in /tmp. If it gets deleted you know that the symlink worked which means that /root/secretdir/ exists and is a directory. This will not neccessarily kill you, but it is an information leak. Solution: Place the "trap" after the "mkdir". Then you can be sure nobody is playing tricks on you when clear() is called. (umask 077 && mkdir $TDIR) || { echo "Unable to create temp directory $TDIR" exit 1 } trap clear 0 Regards nordi