[Xen-devel] Redundant lstats in libxl_pvusb.c

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

In libxl_usb.c, usbintf_get_drvpath calls stat(2) on the driver sysfs
path, and then realpath on the same path.

And bind_usbintf calls stat(2) on the driver directory path, and then
open(2) on a file in that directory.

It seems to be that in both cases, libxl could simply directly access
the target object.  Ie, it could always call realpath, and always call
open.  Appropriate error handling would deal with the cases currently
dealt with by the stat.

Am I wrong about this ?

I'm prompted to look at this by Coverity, Coverity thinks that this
stat-then-realpath, or stat-then-open, might be a TOCTOU security
problem.  I think it's wrong, but it would be nice to tidy up the code
and eliminate these complaints.

If I am right, I'd appreciate patch(es).  They should mention
CID: 1358112
CID: 1358111
for these two functions, respectively.

Thanks,
Ian.

> *** CID 1358112:  Security best practices violations  (TOCTOU)
> /tools/libxl/libxl_pvusb.c: 995 in usbintf_get_drvpath()
> 989         spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf);
> 990     
> 991         r = lstat(spath, &st);
> 992         if (r == 0) {
> 993             /* Find the canonical path to the driver. */
> 994             dp = libxl__zalloc(gc, PATH_MAX);
> >>>     CID 1358112:  Security best practices violations  (TOCTOU)
> >>>     Calling function "realpath" that uses "spath" after a check function. 
> >>> This can cause a time-of-check, time-of-use race condition.
> 995             dp = realpath(spath, dp);
> 996             if (!dp) {
> 997                 LOGE(ERROR, "get realpath failed: '%s'", spath);
> 998                 return ERROR_FAIL;
> 999             }
> 1000         } else if (errno == ENOENT) {

> *** CID 1358111:  Security best practices violations  (TOCTOU)
> /tools/libxl/libxl_pvusb.c: 1061 in bind_usbintf()
> 1055             return 0;
> 1056         if (r < 0 && errno != ENOENT)
> 1057             return ERROR_FAIL;
> 1058     
> 1059         path = GCSPRINTF("%s/bind", drvpath);
> 1060     
> >>>     CID 1358111:  Security best practices violations  (TOCTOU)
> >>>     Calling function "open" that uses "path" after a check function. This 
> >>> can cause a time-of-check, time-of-use race condition.
> 1061         fd = open(path, O_WRONLY);
> 1062         if (fd < 0) {
> 1063             LOGE(ERROR, "open file failed: '%s'", path);
> 1064             rc = ERROR_FAIL;
> 1065             goto out;
> 1066         }

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel