Menu
▾
▴
[TiLP-devel] [ tilp-Bugs-1731776 ] Bad permissions check on /dev/ttySx
|
From: SourceForge.net <no...@so...> - 2007-10-14 02:57:36
|
Bugs item #1731776, was opened at 2007-06-06 03:50 Message generated for change (Comment added) made by kevinkofler You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=118378&aid=1731776&group_id=18378 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Closed Resolution: Fixed Priority: 5 Private: No Submitted By: Ross Kendall Axe (rossaxe) Assigned to: Romain Liévin (roms) Summary: Bad permissions check on /dev/ttySx Initial Comment: TiLP produces the following output in the terminal window when attempting to open a serial link: ticables-INFO: Check for tty usability: ticables-INFO: node /dev/ttyS0: exists ticables-INFO: permissions/user/group: -rw-rw---- root users ticables-INFO: is user can r/w on device: no ticables-INFO: are others can r/w on device: no ticables-INFO: is the user 'ross' in the group 'users': no ticables-INFO: => you should add your username at the group 'users' in '/etc/group' ticables-INFO: => you will have to restart you session, too However, 'id ross' clearly indicates that I'm in the users group, in case I didn't already know that ;-) $ id ross uid=1004(ross) gid=100(users) groups=100(users),11(floppy),17(audio),18(video),19(cdrom),93(scanner) Any attempt to use the link therefore fails since it never got initialised. As to why it failed to notice that I have access, well, I'm not sure. It could be that it's because it's my primary group, or because my passwd entry is accessed via nss_ldap. I imagine it would also fail if I was using ACLs. It seems to be that TiLP ought to simply use open("/dev/ttySx", O_RDWR) to check access rather than trying to be too clever and attempting to second guess the kernel's access rules. Meanwhile, 'chmod 0666 /dev/ttyS?' is a suitable workaround for this problem :-) ---------------------------------------------------------------------- Comment By: Kevin Kofler (kevinkofler) Date: 2007-10-14 04:57 Message: Logged In: YES user_id=573515 Originator: NO Oops, that if(access(pathname, R_OK | W_OK)) should have been if(!access(pathname, R_OK | W_OK)), I fixed that: http://svn.tilp.info/cgi-bin/viewcvs.cgi?rev=3905&view=rev ---------------------------------------------------------------------- Comment By: Romain Liévin (roms) Date: 2007-10-14 04:40 Message: Logged In: YES user_id=136160 Originator: NO Patch merged. Thanks, Kevin. ---------------------------------------------------------------------- Comment By: Kevin Kofler (kevinkofler) Date: 2007-10-13 23:28 Message: Logged In: YES user_id=573515 Originator: NO Fixed patch (no need to check for group membership if the user has access, we can go directly to the "unknown reason" conclusion): Index: src/linux/detect.c =================================================================== --- src/linux/detect.c (revision 3891) +++ src/linux/detect.c (working copy) @@ -3,6 +3,7 @@ /* libticables2 - link cable library, a part of the TiLP project * Copyright (C) 1999-2005 Romain Lievin + * Copyright (C) 2007 Kevin Kofler * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -217,12 +218,18 @@ } else { - ticables_info(_(" node %s: does not exists"), pathname); + ticables_info(_(" node %s: does not exist"), pathname); ticables_info(_(" => you will have to create the node.")); return -1; } + if(access(pathname, R_OK | W_OK)) + { + ticables_info(_(" node %s: accessible"), pathname); + return 0; + } + if(!stat(pathname, &st)) { ticables_info(_(" permissions/user/group:%s%s %s"), @@ -238,23 +245,24 @@ if(getuid() == st.st_uid) { - ticables_info(_(" is user can r/w on device: yes")); - return 0; + ticables_info(_(" user can r/w on device: yes")); + ticables_info(_(" => device is inaccessible for unknown reasons (SELinux?)")); + return -1; } else { - ticables_info(_(" is user can r/w on device: no")); + ticables_info(_(" user can r/w on device: no")); } if((st.st_mode & S_IROTH) && (st.st_mode & S_IWOTH)) { - ticables_info(_(" are others can r/w on device: yes")); + ticables_info(_(" others can r/w on device: yes")); } else { char *user, *group; - ticables_info(_(" are others can r/w on device: no")); + ticables_info(_(" others can r/w on device: no")); user = strdup(get_user_name(getuid())); group = strdup(get_group_name(st.st_gid)); @@ -268,7 +276,7 @@ { ticables_info(_(" is the user '%s' in the group '%s': no"), user, group); ticables_info(_(" => you should add your username at the group '%s' in '/etc/group'"), group); - ticables_info(_(" => you will have to restart you session, too"), group); + ticables_info(_(" => you will have to restart you session, too")); free(user); free(group); @@ -279,7 +287,8 @@ free(group); } - return 0; + ticables_info(_(" => device is inaccessible for unknown reasons (SELinux?)")); + return -1; } int linux_check_root(void) ---------------------------------------------------------------------- Comment By: Kevin Kofler (kevinkofler) Date: 2007-10-13 23:23 Message: Logged In: YES user_id=573515 Originator: NO > It does seem to me that if the program is not SUID, then no extra checks > are needed beyond what the kernel does anyway when calling open(), and if > it is then it should just drop privileges before it calls open(). Running TiLP SUID is no longer supported (there's better ways to get access rights to devices these days), but even when it was, the entire purpose of doing that was to be able to access more devices than otherwise. > But then again, I don't know what the reason for this extra check is so > perhaps this is too simple minded. It's to help debugging. I think the right solution is to: * use access() to check if the device can be accessed, * only if this fails, do the other checks to help debugging. I'd suggest this patch: Index: src/linux/detect.c =================================================================== --- src/linux/detect.c (revision 3891) +++ src/linux/detect.c (working copy) @@ -3,6 +3,7 @@ /* libticables2 - link cable library, a part of the TiLP project * Copyright (C) 1999-2005 Romain Lievin + * Copyright (C) 2007 Kevin Kofler * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -217,12 +218,18 @@ } else { - ticables_info(_(" node %s: does not exists"), pathname); + ticables_info(_(" node %s: does not exist"), pathname); ticables_info(_(" => you will have to create the node.")); return -1; } + if(access(pathname, R_OK | W_OK)) + { + ticables_info(_(" node %s: accessible"), pathname); + return 0; + } + if(!stat(pathname, &st)) { ticables_info(_(" permissions/user/group:%s%s %s"), @@ -238,23 +245,22 @@ if(getuid() == st.st_uid) { - ticables_info(_(" is user can r/w on device: yes")); - return 0; + ticables_info(_(" user can r/w on device: yes")); } else { - ticables_info(_(" is user can r/w on device: no")); + ticables_info(_(" user can r/w on device: no")); } if((st.st_mode & S_IROTH) && (st.st_mode & S_IWOTH)) { - ticables_info(_(" are others can r/w on device: yes")); + ticables_info(_(" others can r/w on device: yes")); } else { char *user, *group; - ticables_info(_(" are others can r/w on device: no")); + ticables_info(_(" others can r/w on device: no")); user = strdup(get_user_name(getuid())); group = strdup(get_group_name(st.st_gid)); @@ -268,7 +274,7 @@ { ticables_info(_(" is the user '%s' in the group '%s': no"), user, group); ticables_info(_(" => you should add your username at the group '%s' in '/etc/group'"), group); - ticables_info(_(" => you will have to restart you session, too"), group); + ticables_info(_(" => you will have to restart you session, too")); free(user); free(group); @@ -279,7 +285,8 @@ free(group); } - return 0; + ticables_info(_(" => device is inaccessible for unknown reasons (SELinux?)")); + return -1; } int linux_check_root(void) Romain, can I commit this one? ---------------------------------------------------------------------- Comment By: Ross Kendall Axe (rossaxe) Date: 2007-10-13 20:30 Message: Logged In: YES user_id=1206088 Originator: YES Well, I'm glad I'm not the only one who thinks so :-) It does seem to me that if the program is not SUID, then no extra checks are needed beyond what the kernel does anyway when calling open(), and if it is then it should just drop privileges before it calls open(). But then again, I don't know what the reason for this extra check is so perhaps this is too simple minded. nss_ldap, ACLs, maybe SELinux? Either these things (and god only knows what else) have to supported explicitly, or libticables should just trust that the kernel knows best? ---------------------------------------------------------------------- Comment By: Kevin Kofler (kevinkofler) Date: 2007-10-13 08:47 Message: Logged In: YES user_id=573515 Originator: NO How's that "fixed"? He just worked around it. We really really need to fix libticables2 not to do those weird access checks, also because newer Fedora (and probably many other distros soon, because granting access to console users is now going to be done with ConsoleKit and HAL rather than the RH-specific pam_console) is going to always use ACLs to grant access to devices, so your ACL-unaware hacks aren't going to work anymore. ---------------------------------------------------------------------- Comment By: Ross Kendall Axe (rossaxe) Date: 2007-07-09 15:10 Message: Logged In: YES user_id=1206088 Originator: YES Yes, it's OK now with the the modification to /etc/group. ---------------------------------------------------------------------- Comment By: Romain Liévin (roms) Date: 2007-07-05 08:50 Message: Logged In: YES user_id=136160 Originator: NO So, is TiLP working now? ---------------------------------------------------------------------- Comment By: Ross Kendall Axe (rossaxe) Date: 2007-07-05 00:46 Message: Logged In: YES user_id=1206088 Originator: YES Yeah, putting my username into /etc/group does also work around the problem. My username wasn't in 'group' on the LDAP server either though, only in 'passwd' (where my GID is set to the GID of the users group, of course). ---------------------------------------------------------------------- Comment By: Romain Liévin (roms) Date: 2007-07-04 14:58 Message: Logged In: YES user_id=136160 Originator: NO ticables2 library is parsing /etc/group for entries. Is your username in the same line as the 'users' line in this file? nss_ldap access may explain your problem... ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=118378&aid=1731776&group_id=18378 |
