Summary: | app-misc/beep-1.3 - Could not open /dev/tty0 or /dev/vc/0 for writing | ||
---|---|---|---|
Product: | Gentoo Linux | Reporter: | Martin von Gagern <Martin.vGagern> |
Component: | Current packages | Assignee: | Gentoo Shell Tools project <shell-tools> |
Status: | RESOLVED FIXED | ||
Severity: | trivial | CC: | bugs, ibormuth, main.haarp, please.no.spam.here |
Priority: | High | Keywords: | PATCH |
Version: | 2006.1 | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://www.johnath.com/beep/README | ||
Whiteboard: | |||
Package list: | Runtime testing required: | --- | |
Attachments: |
Introduce suid USE flag, restricting access to audio group
Proposed new nosuid patch Introduce suid USE flag, restricting access to audio group |
Description
Martin von Gagern
2007-02-24 01:31:43 UTC
*** Bug 168205 has been marked as a duplicate of this bug. *** Paste from Bug 168205, comment #0 The patch beep-1.2.2-nosuid.patch which is applied for app-misc/beep-1.2.2-r1 has two problems: close(console_fd); does not check console_fd is valid. This causes close to result in EBADF for users without access to /dev/console. Should be no problem, but it's the same situation as with the ioctl for stopping the beep. In both cases the call will fail with EBADF if executed, unchecked, and in both cases a check before the execution is cleaner. The stop ioctl does check, so I believe close should be preceded by an "if (console_fd >= 0)" as well. bell printed to stdout, not stderr. This can cause problems in scripts that use bell, expecting no sideeffects, and redirecting their standard output. Or put differently, using stderr instead would allow the fallback to work even when stdout is redirected. The upstream release has the same problem here, I'll report this upstream as well. (In reply to comment #1) I'm not sure why Jakub duped this here. bug #168205 was a set of minor problems with the patch, with little real world impact. This here was intended as a feature request for making beep suid root, depending on USE flag configuration. This has little to do with the patch except that the patch is not and can never be a full replacement of a suid root binary. What is the purpose of the patch Gentoo adds to upstream beep? It only makes things worse. From beep-1.2.2 to beep-1.2.2-r1, there is a regression. The patch removes the checks of ioctl. In my system, when running the program with no special privileges (nor sudo nor setuid): 1) The upstream program beeps and tells me that "ioctl: Operation not permitted" 2) The patched program does not beep or warn me. Probably because the program successfully opens /dev/console and then gets "permission denied" when trying to execute ioctl. How about the "suid" USE flag making the binary mode 4750 owned by group "audio"? It is, after all, a program for playing audio. This seems to me to be the most sane policy: anyone who can make the sound card make noise can also make the PC speaker make noise, which is _not_ necessarily everyone (especially in a situation where you have a lot of SSHers). Created attachment 175585 [details, diff] Introduce suid USE flag, restricting access to audio group (In reply to comment #5) > How about the "suid" USE flag making the binary mode 4750 owned by group > "audio"? Sounds good to me. To get some traction here, I attach a patch to the ebuild to implement this. I would think this ready for inclusion right now, as the default behaviour is USE=-suid, in which case nothing changes. I think the nosuid patch should be applied in either case; it certainly doesn't hurt anything and I agree with the principle of the patch (though, as Jakub Moc points out, the implementation is not yet perfect). I think the name choice is rather poor though; it doesn't really have anything to do with SUID as such. It's more about making the repeat count and the intertone delay apply when permissions aren't available _for whatever reason_. That said, the rest of Martin's patch looks good to me. (In reply to comment #7) > I think the nosuid patch should be applied in either case; it certainly > doesn't hurt anything The patch prevents diagnostic output which could be useful at times, as Jorge pointed out. In a non-suid setup, the most likely reason for the program to fail would be lack of privileges, in which case the diagnostics might be considered ugly (though I'd disagree even there). In a suid setup, that shouldn't be the case, so something else is causing trouble, and I'd want detailed information about this trouble without jumoing through hoops. Users not in the audio group wouldn't be granted permission to execute the binary in the first place and thus receive an error mesage from their command interpreter. > (though, as Jakub Moc points out, the implementation is not yet perfect). He was quoting my initial comment from another bug he duped here. Looking at the patch again, I found a third problem with it: When open succeeds but the ioctl fails, the original code would fallback to a bell and print diagnostic information, while the patched version does neither. > It's more about making the repeat count and the intertone delay apply when > permissions aren't available _for whatever reason_. That's one part of the patch, and I would even agree you are right there. The other part, however, is dropping lots of diagnostic output. That part is what I'd not like to loose when installing it suid. Creating a new patch to include the diagnostics and still do the repeats seems a bit overkill to me, though. And honouring the repeats without control over the lengths might be a bad idea as well, as you might end up with considerably longer run time than originally intended. Created attachment 175910 [details, diff]
Proposed new nosuid patch
So you mean more something along the lines of this patch? (still dumps diagnostic output; writes bell char to stderr instead of stdout; guards all accesses to console_fd)
(In reply to comment #9) > So you mean more something along the lines of this patch? That's more like it, yes. (In reply to comment #10) > (In reply to comment #9) > > So you mean more something along the lines of this patch? > > That's more like it, yes. > So, what is the consensus here? IUSE=suid, with suid patch??? I'm slightly lost on the specifics, can someone present a final ebuild patch for me? Christopher Head's solution in comment #5 sounds the best to me. Although I'm no dev, I'd go with that one :) I would vote for Martin's patch from #6 to just get this off the ground. The "nosuid" patch *should* have absolutely no effect when beep is SUID; whether that patch should be applied never, conditionally, or always seems a little less clear-cut (people seem to disagree on its costs and benefits and on whether the implementation is fully correct; however, since the patch is *already* being applied, it doesn't really matter for the sake of just getting the suid USE flag into the ebuild). It would be nice to see the USE flag pushed out, and then further discussion of the "nosuid" patch could happen separately. It's been over two years. Is someone planning to apply this patch? beep-1.3 has been around for a while, and it doesn't apply the “nosuid” patch. But neither does it install the binary suid root. So an unprivileged user on a graphical desktop will see this error message: Could not open /dev/tty0 or /dev/vc/0 for writing open: No such file or directory The second line likely refers to /dev/vc/0, while /dev/tty0 has mode 0620 and owner root:tty. Normal users doesn't have access to this, which I think makes sense; we shouldn't start adding users to the tty group. The beep README does discuss the possibility of installing it suid root, and perhaps restricting its access to a suitable group. Created attachment 410544 [details, diff]
Introduce suid USE flag, restricting access to audio group
Since 1.3 doesn't do that “nosuid” patching any more, the discussion about when to apply that patch seems obsolete, as do the corresponding parts in my patch. For the rest I think we had a consensus: introduce a USE=suid flag, and change the group to audio if it is set. This patch to the ebuild reflects that. It's really simple.
* commit 88699eb | Author: Patrice Clement <monsieurp@gentoo.org> | Date: Fri Aug 28 23:28:57 2015 +0200 | | app-misc/beep: Add USE=suid flag courtesy of Martin von Gagern <Martin.vGagern@gmx.net>. Fixes bug 168201. | | Package-Manager: portage-2.2.20.1 | Signed-off-by: Patrice Clement <monsieurp@gentoo.org> | | create mode 100644 app-misc/beep/beep-1.3-r1.ebuild Thanks Martin. Wow! After all this time, now so quickly after my patch resolved! That's great! |