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

[Chun Yan Liu]

>>> On 4/7/2016 at 06:43 PM, in message
<22278.14817.248393.423...@mariner.uk.xensource.com>, Ian Jackson
 wrote: 
> Chun Yan Liu writes ("Re: Redundant lstats in libxl_pvusb.c"): 
> > <22274.33583.712655.413...@mariner.uk.xensource.com>, Ian Jackson 
> >  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. 
>  
> Thanks. 
>  
> > > 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). 
>  
> I have read the function again and you are right. 
>  
> Coverity said: 
>  
> > > > >>> 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. 
>  
> But it seems that it is confused by the reuse of the path variable. 
> I think this is arguably a bug in Coverity. 
>  
> But, evidently, the same reuse confused me too.  Maybe we should turn 
> `path' into two variables, `intf_path' and `bind_path' ?  What do you 
> think ? 

Yeah, maybe it's better to change into 'intf_path' and 'bind_path', I'll update.
But it's unavoidable that some temp variable will be reused for many
times.

Chunyan

>  
> Thanks, 
> Ian. 
>  
>  



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

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

[Ian Jackson]

Chun Yan Liu writes ("Re: Redundant lstats in libxl_pvusb.c"):
> <22274.33583.712655.413...@mariner.uk.xensource.com>, Ian Jackson
>  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.

Thanks.

> > 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).

I have read the function again and you are right.

Coverity said:

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

But it seems that it is confused by the reuse of the path variable.
I think this is arguably a bug in Coverity.

But, evidently, the same reuse confused me too.  Maybe we should turn
`path' into two variables, `intf_path' and `bind_path' ?  What do you
think ?

Thanks,
Ian.

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

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

[Chun Yan Liu]

>>> On 4/4/2016 at 11:07 PM, in message
<22274.33583.712655.413...@mariner.uk.xensource.com>, Ian Jackson
 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, ); 
> > 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@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] Redundant lstats in libxl_pvusb.c

[Ian Jackson]

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, );
> 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@lists.xen.org
http://lists.xen.org/xen-devel