* [RFC] killing DEVFS_FL_AUTO_OWNER @ 2002-10-06 18:55 Alexander Viro 2002-10-06 19:32 ` Richard Gooch 0 siblings, 1 reply; 4+ messages in thread From: Alexander Viro @ 2002-10-06 18:55 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, Richard Gooch Devfs supports an interesting "feature" (read: gaping security hole waiting to happen). Namely, there is one (1) driver that asks for the following behaviour: its nodes (/dev/video1394/*) are created with root.root rw-rw-rw- opening a node sets (with feel^Wraces) ownership to that of opening task and mode to rw-------. if you close it, wait for dentry to be evicted and open it again - you get root.root rw-rw-rw- and it will stay that way after open(). Now, I might try and guess WTF had been intended (reversion to permissions of the Beast upon final close() and subsequent open() acting as the first one), but even if it would behave that way... Just ask yourself what will happen to any program that relies on this behavior on a system where /dev is not on devfs. And that, BTW, is the setup suggested by vidoe1394 folks on their homepage. Now, I don't ask WTF had been smoked to produce that code - I don't want to know and it's probably illegal anywhere (if it isn't, it should be). The question is could we fscking please remove that idiocy? Unless somebody gives a good reason why behaviour of DEVFS_FL_AUTO_OWNER is _not_ a security hole - I'm submitting a patch that removes this crap in a couple of hours. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] killing DEVFS_FL_AUTO_OWNER 2002-10-06 18:55 [RFC] killing DEVFS_FL_AUTO_OWNER Alexander Viro @ 2002-10-06 19:32 ` Richard Gooch 2002-10-06 20:07 ` Alexander Viro 0 siblings, 1 reply; 4+ messages in thread From: Richard Gooch @ 2002-10-06 19:32 UTC (permalink / raw) To: Alexander Viro; +Cc: Linus Torvalds, linux-kernel Alexander Viro writes: > Devfs supports an interesting "feature" (read: gaping security > hole waiting to happen). Namely, there is one (1) driver that asks > for the following behaviour: > its nodes (/dev/video1394/*) are created with root.root rw-rw-rw- > opening a node sets (with feel^Wraces) ownership to that of opening > task and mode to rw-------. > if you close it, wait for dentry to be evicted and open it again - > you get root.root rw-rw-rw- and it will stay that way after open(). > > Now, I might try and guess WTF had been intended (reversion to > permissions of the Beast upon final close() and subsequent open() > acting as the first one), but even if it would behave that way... The orginal use for DEVFS_FL_AUTO_OWNER was for PTY slaves, particularly the BSD-style ones. It allows a non-suid root process to open an unused PTY and be given ownership of it. Good for non-suid xterms. Later, I created DEVFS_FL_CURRENT_OWNER, and used that (in combination with unregistering PTY slaves when the master is closed). That seemed a cleaner approach. > Just ask yourself what will happen to any program that relies on > this behavior on a system where /dev is not on devfs. And that, > BTW, is the setup suggested by vidoe1394 folks on their homepage. > > Now, I don't ask WTF had been smoked to produce that code - I don't > want to know and it's probably illegal anywhere (if it isn't, it > should be). The question is could we fscking please remove that > idiocy? Unless somebody gives a good reason why behaviour of > DEVFS_FL_AUTO_OWNER is _not_ a security hole - I'm submitting a > patch that removes this crap in a couple of hours. Well, I can't comment on the video1394 driver. I don't really know why they are using DEVFS_FL_AUTO_OWNER. If their device node is safe to have rw-rw-rw- (like with PTY slaves), then it's not a problem. However, if the driver allows you to do Bad Things[tm] if you can read or write to the device node, then the driver is buggy, and is abusing DEVFS_FL_AUTO_OWNER. So we should get input from the driver maintainer as to what the intent is. Regards, Richard.... Permanent: rgooch@atnf.csiro.au Current: rgooch@ras.ucalgary.ca ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] killing DEVFS_FL_AUTO_OWNER 2002-10-06 19:32 ` Richard Gooch @ 2002-10-06 20:07 ` Alexander Viro 2002-10-13 0:17 ` Richard Gooch 0 siblings, 1 reply; 4+ messages in thread From: Alexander Viro @ 2002-10-06 20:07 UTC (permalink / raw) To: Richard Gooch; +Cc: Linus Torvalds, linux-kernel On Sun, 6 Oct 2002, Richard Gooch wrote: > Well, I can't comment on the video1394 driver. I don't really know why > they are using DEVFS_FL_AUTO_OWNER. If their device node is safe to > have rw-rw-rw- (like with PTY slaves), then it's not a problem. > However, if the driver allows you to do Bad Things[tm] if you can read > or write to the device node, then the driver is buggy, and is abusing > DEVFS_FL_AUTO_OWNER. > > So we should get input from the driver maintainer as to what the > intent is. 1) current implementation does _not_ reset uid/gid after the first open(). It either stays as it was (until the d_delete) or gets reset to root.root.666 (after d_delete) and stays that way. Notice that your code that sets it is in the very end of devfs_open() - in the part that is run once. df->open is never reset, so... Oh, BTW - you have if (df->open) return 0; df->open = TRUE; with no locking whatsoever - you'd reduced BKL-covered area so that it doesn't cover that place. With obvious consequences... 2) either applications do care to do chmod/chown (and in that case DEVFS_FL_AUTO_OWNER is simply irrelevant), or they are * broken on non-devfs systems * broken on devfs systems due to (1) Having world-readable video camera is an obvious security problem, so applications _must_ deal with that, for non-devfs systems if nothing else. 3) this crap is the only thing that still uses DEVFS_FL_AUTO_OWNER. IOW, we should remove it and send heads-up to driver authors. End of story. Another thing: when you do devfs_register(), the thing creates intermediate directories. However, devfs_unregister() on the same node doesn't undo the effect of devfs_register() - directories stay around, even if nothing else holds them. Proposal: add a counter to devfs entries of directories so that * result of devfs_mk_dir would have it set to 1 * creation of child in a directory would increment it by 1 * removal of child would decrement it by 1 * when counter drops to 0 (which means that directory had been created implictly by devfs_register() and all children are gone) we unregister directory. That, in turn, can cause unregistering its parent, etc. That would cut down the amount of work done in drivers (esp. block device drivers) and allow to simplify the ad-hackery in partitions/check.c. It's trivial to implement, so if you have objections to that - tell it now. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] killing DEVFS_FL_AUTO_OWNER 2002-10-06 20:07 ` Alexander Viro @ 2002-10-13 0:17 ` Richard Gooch 0 siblings, 0 replies; 4+ messages in thread From: Richard Gooch @ 2002-10-13 0:17 UTC (permalink / raw) To: Alexander Viro; +Cc: Linus Torvalds, linux-kernel Alexander Viro writes: > On Sun, 6 Oct 2002, Richard Gooch wrote: > > Well, I can't comment on the video1394 driver. I don't really know why > > they are using DEVFS_FL_AUTO_OWNER. If their device node is safe to > > have rw-rw-rw- (like with PTY slaves), then it's not a problem. > > However, if the driver allows you to do Bad Things[tm] if you can read > > or write to the device node, then the driver is buggy, and is abusing > > DEVFS_FL_AUTO_OWNER. > > > > So we should get input from the driver maintainer as to what the > > intent is. Before you get upset about my reply, bear in mind that the following discussion is mostly irrelevant, since DEVFS_FL_AUTO_OWNER is no longer being used for BSD PTY slaves. > 1) current implementation does _not_ reset uid/gid after the first > open(). It either stays as it was (until the d_delete) or gets > reset to root.root.666 (after d_delete) and stays that way. The uid/gid isn't supposed to reset after the first open(). It's supposed to reset after the last close() (or soon after). The intended semantics of DEVFS_FL_AUTO_OWNER as quite simple: hand ownership of the node to the opening process, which will then do a chmod(2) to obtain the desired permissions (which are usually rw-------). The process will receive EPERM if someone else opens the same device at the same time and wins the race. Once the "owning" process closes the device, it will soon be restored to root.root rw-rw-rw-, allowing another process to open the device. > Notice that your code that sets it is in the very end of > devfs_open() - in the part that is run once. df->open is never > reset, so... Actually, it *is* reset, in devfs_d_delete(). > Oh, BTW - you have > > if (df->open) return 0; > df->open = TRUE; > > with no locking whatsoever - you'd reduced BKL-covered area so that > it doesn't cover that place. With obvious consequences... It doesn't really matter, because the worst that will happen is that a process open(2)s the node, but some other process manages to get ownership. For the intended use (allowing the user to set BSD PTY slave permissions to rw------- rather than the normal rw-rw-rw-) this is fine. The situation is no worse than a non-devfs system where a non-privileged process cannot ever lock down the permissions on its BSD PTY slave. With DEVFS_FL_AUTO_OWNER, said process can. > 2) either applications do care to do chmod/chown (and in that case > DEVFS_FL_AUTO_OWNER is simply irrelevant), or they are > * broken on non-devfs systems > * broken on devfs systems due to (1) I do: "chmod go= `tty`" in my login scripts, just to prevent funny buggers from sending crap to my login session. I don't always have permission to do this (depending on the system), but where I do, it's a plus. So the intended application doesn't depend on this, but takes advantage of it. I do agree that depending on it is non-portable, though. > Having world-readable video camera is an obvious security problem, > so applications _must_ deal with that, for non-devfs systems if > nothing else. Agreed, and I can't see why the driver author did this. > 3) this crap is the only thing that still uses DEVFS_FL_AUTO_OWNER. > > IOW, we should remove it and send heads-up to driver authors. End > of story. That's fine by me too. I might have coded up a patch for it by now, but my box has been busy the last few hours doing a rsync (scanning the ChangeSet information seems to be the main offender, even though I have a gig of RAM:-(). I'll start on this once my repository is unlocked again. > Another thing: when you do devfs_register(), the thing creates > intermediate directories. However, devfs_unregister() on the same > node doesn't undo the effect of devfs_register() - directories stay > around, even if nothing else holds them. > > Proposal: add a counter to devfs entries of directories so that > * result of devfs_mk_dir would have it set to 1 > * creation of child in a directory would increment it by 1 > * removal of child would decrement it by 1 > * when counter drops to 0 (which means that directory had > been created implictly by devfs_register() and all children are gone) > we unregister directory. That, in turn, can cause unregistering its > parent, etc. Hm. I'll have to think about this some more, but offhand I'm not happy about the potential interactions with devfs_get()/devfs_put(). If a driver grabs hold of an intermediate directory and removes the child, the directory it has a hold of is no longer part of the tree. This could cause problems for some people (I've always rejected a devfs_move(), but I know there are people who have implemented it themselves). A sequence like: devfs_get_handle (); devfs_unregister (); devfs_register (); would now fail. People would have to recode to: devfs_get_handle (); devfs_register (); devfs_unregister (); > That would cut down the amount of work done in drivers (esp. block > device drivers) and allow to simplify the ad-hackery in > partitions/check.c. What kind of reductions to driver code do you have in mind? Is it really a big deal? Regards, Richard.... Permanent: rgooch@atnf.csiro.au Current: rgooch@ras.ucalgary.ca ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2002-10-13 0:11 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2002-10-06 18:55 [RFC] killing DEVFS_FL_AUTO_OWNER Alexander Viro 2002-10-06 19:32 ` Richard Gooch 2002-10-06 20:07 ` Alexander Viro 2002-10-13 0:17 ` Richard Gooch
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).