Hello,
Peter Maydell, le jeu. 23 oct. 2025 10:58:10 +0100, a ecrit:
> Coverity worries here about a possible time-of-check-time-of-use
> bug (CID 1641394). This is a heuristic that tends to fire even
> when there's no interesting attack possible, but I don't
> know what this code is doing so I raise it here:
>
> > +#if !defined(WIN32) && SLIRP_CHECK_VERSION(4, 7, 0)
> > + struct stat st;
> > + if (stat(buf, &st) == 0) {
>
> Coverity notes that we do a check on the filename here
> with stat()...
>
> > + if (!S_ISSOCK(st.st_mode)) {
> > + fail_reason = "file exists and it's not unix socket";
> > + goto fail_syntax;
> > + }
> > +
> > + if (unlink(buf) < 0) {
>
> ...and then later we do an unlink() if it's a unix socket.
> But Coverity points out that an attacker could change what
> the filename points to between the stat and the unlink,
> causing us to unlink some non-socket file.
>
> Do we care ?
It is true that an "attacker" could be pointing qemu to a symlink to
a socket, and stat() will follow it, and they can change the symlink
into something else (a symlink to something else, or another type of
file), and we'll unlink() that (which will not follow any symlink, so
just unlink the given path).
I don't see which harmful scenario we could have here. Either the
attacker has control over the given path, and we'll just unlink it, too
bad for them, or they don't have control over the given path, and they
won't be able to change it to their liking between stat() and unlink().
Once thing we could do is to use fstatat to pass AT_SYMLINK_NOFOLLOW,
(when fstatat is available), but I'm not really seeing it worth the
effort.
Samuel
