Re: [Xen-devel] Redundant lstats in libxl_pvusb.c

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 4/4/2016 at 11:07 PM, in message
<22274.33583.712655.413448@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson
<Ian.Jackson@xxxxxxxxxxxxx> wrote: 
> In libxl_usb.c, usbintf_get_drvpath calls stat(2) on the driver sysfs 
> path, and then realpath on the same path. 

It's true. This could be done by calling realpath only. Will correct.

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

It's not true. It calls stat(2) on a file in driver path (driver/interface),
and open(2) on another file in that driver path (driver/bind).

Chunyan
>  
> 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