reopen 302454
thanks, but the fix doesn't guard against races; see below
Ari Pollak wrote:
> I just uploaded trackballs 1.0.0-10 to unstable, which fixes bug
> #302454, a severity:important and minor security issue, so I'd like to
> get this into sarge. An interdiff between revisions -9 and -10 is attached.
> --- trackballs-1.0.0/config.guess
> +++ trackballs-1.0.0/config.guess
> --- trackballs-1.0.0/config.sub
> +++ trackballs-1.0.0/config.sub
Hurrah for pointless config.* updates in security updates.
> diff -u trackballs-1.0.0/share/icons/Makefile.in trackballs-1.0.0/share/icons/Makefile.in
> --- trackballs-1.0.0/share/icons/Makefile.in
> +++ trackballs-1.0.0/share/icons/Makefile.in
> @@ -195,8 +195,8 @@
> maintainer-clean-generic clean mostlyclean distclean maintainer-clean
>
>
> -install-pkgdataDATA:
> - ./installIcons $(bindir) ${DESTDIR}
> +#install-pkgdataDATA:
> +# ./installIcons $(bindir) ${DESTDIR}
>
> # Tell versions [3.59,3.63) of GNU make to not export all variables.
> # Otherwise a system limit (for SysV at least) may be exceeded.
> diff -u trackballs-1.0.0/share/icons/trackballs.desktop trackballs-1.0.0/share/icons/trackballs.desktop
> --- trackballs-1.0.0/share/icons/trackballs.desktop
> +++ trackballs-1.0.0/share/icons/trackballs.desktop
> @@ -9,14 +8,0 @@
> -Exec=/usr/local/bin/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> -Exec=/usr/games/trackballs
> + * Don't bother running the script to install a GNOME .desktop file
> + since it doesn't work anyway
Doesn't explain why you modified the .desktop file, although I think I
see why -- the script was appending to it on each build, right?
Anyway, I ended up wasting at least 10 minutes reading the code and
doing my own test build on a system with /usr/share/applications present
to satisfy myself that installIcons never manages to do anything and
that this change is indeed a no-op. If you want quick service for
security fixes, please do not conflate them with unrelated changes like
this one.
> diff -u trackballs-1.0.0/debian/changelog trackballs-1.0.0/debian/changelog
> --- trackballs-1.0.0/debian/changelog
> +++ trackballs-1.0.0/debian/changelog
> @@ -1,3 +1,11 @@
> +trackballs (1.0.0-10) unstable; urgency=low
> +
> + * Backport symlink checking code from upstream CVS (Closes: #302454)
> +
> + -- Ari Pollak <ari@debian.org> Sun, 8 May 2005 18:49:27 -0400
> +
> trackballs (1.0.0-9) unstable; urgency=low
>
> * Somehow I uploaded -8 without the fix. Really do that this time.
> only in patch2:
> unchanged:
> --- trackballs-1.0.0.orig/src/general.cc
> +++ trackballs-1.0.0/src/general.cc
> @@ -25,6 +25,8 @@
> #include "font.h"
> #include "glHelp.h"
> #include "SDL/SDL_image.h"
> +#include <sys/types.h>
> +#include <sys/stat.h>
>
> using namespace std;
>
> @@ -58,3 +60,10 @@
> if(fp) { fclose(fp); return 1; }
> return 0;
> }
> +
> +int pathIsLink(char *path) {
> + struct stat m;
> + if(lstat(path,&m)) return 0;
> + if(S_ISLNK(m.st_mode)) return 1;
> + return 0;
> +}
This patch immediatly raises a red flag here, since this kind of check
cannot guard against races and is thus useless.
> --- trackballs-1.0.0.orig/src/settings.cc
> +++ trackballs-1.0.0/src/settings.cc
> @@ -140,9 +140,17 @@
> int version=4;
>
> snprintf(str,sizeof(str)-1,"%s/.trackballs",getenv("HOME"));
> + if(pathIsLink(str)) {
> + fprintf(stderr,"Error, %s is a symbolic link. Cannot save settings\n",str);
> + return;
> + }
>
> mkdir(str,S_IXUSR|S_IRUSR|S_IWUSR|S_IXGRP|S_IRGRP|S_IWGRP);
> snprintf(str,sizeof(str)-1,"%s/.trackballs/settings",getenv("HOME"));
> + if(pathIsLink(str)) {
> + fprintf(stderr,"Error, %s is a symbolic link. Cannot save settings\n",str);
> + return;
> + }
>
> /* TODO. Save all settings here */
> FILE *fp = fopen(str,"wb");
And sure enough, the way this patch uses pathIsLink does not guard
against an attacker racing trackballs and replacing a valid path with a
symlink bewteen the lstat and the fopen. This fails to fix the security
hole.
A correct fix would be to drop permissions before writing to files under
the user's control.
So sorry, I cannot accept this to testing. If you re-do the patch to
really be secure, I'd appreciate it if the new version doesn't include
the gnome desktop file change -- that would help speed its review.
--
see shy jo
Attachment:
signature.asc
Description: Digital signature