Bug 491430
Summary: | Review Request: sslogger - A keystroke logging utility for privileged user escalation | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ed Brand <edbrand> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | fedora-package-review, gratien.dhaese, herrold, mtasaka, notting, tcallawa, vanhoof |
Target Milestone: | --- | Flags: | mtasaka:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-09-22 17:37:48 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Ed Brand
2009-03-20 22:44:12 UTC
*** Bug 491443 has been marked as a duplicate of this bug. *** *** Bug 491442 has been marked as a duplicate of this bug. *** *** Bug 491434 has been marked as a duplicate of this bug. *** *** Bug 491433 has been marked as a duplicate of this bug. *** *** Bug 491431 has been marked as a duplicate of this bug. *** *** Bug 491429 has been marked as a duplicate of this bug. *** I really don't understand why you submitted this package 6 more times total 7 times including this?? Make sure you always reports package review request of any package only once. *** Bug 491464 has been marked as a duplicate of this bug. *** (In reply to comment #7) > I really don't understand why you submitted this package 6 more times total 7 > times including this?? > > Make sure you always reports package review request of any package only once. Apologies, first time user of Bugzilla, I figured out what I was doing wrong Please start with reading: https://fedoraproject.org/wiki/PackageMaintainers/CreatingPackageHowTo or https://fedoraproject.org/wiki/Packaging/Guidelines for further details. It will help you to improve the spec file. Please use the "rpmlint" program to validate the spec/rpm/srpm package. Once you show the output of rpmlint without errors I'll digg into your package for further assistance. # rpmlint -vi sslogger.spec sslogger.spec:7: W: hardcoded-path-in-buildroot-tag /%{_tmppath}/%{name}-root A path is hardcoded in your Buildroot tag. It should be replaced by something like %{_tmppath}/%name-root. sslogger.spec:10: W: hardcoded-packager-tag Ed The Packager tag is hardcoded in your spec file. It should be removed, so as to use rebuilder's own defaults. sslogger.spec:27: W: rpm-buildroot-usage %prep if [ -n "%{buildroot}" ]; then $RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it will break short circuiting. sslogger.spec:28: W: rpm-buildroot-usage %prep if [ "%{buildroot}" != "/" ]; then $RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it will break short circuiting. sslogger.spec:29: W: rpm-buildroot-usage %prep echo removing %{buildroot} $RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it will break short circuiting. sslogger.spec:30: W: rpm-buildroot-usage %prep rm -rf %{buildroot} $RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it will break short circuiting. sslogger.spec: E: no-cleaning-of-buildroot %install You should clean $RPM_BUILD_ROOT in the %clean section and just after the beginning of %install section. Use "rm -Rf $RPM_BUILD_ROOT". sslogger.spec: E: no-cleaning-of-buildroot %clean You should clean $RPM_BUILD_ROOT in the %clean section and just after the beginning of %install section. Use "rm -Rf $RPM_BUILD_ROOT". 0 packages and 1 specfiles checked; 2 errors, 6 warnings. Updated soec file, please review Spec URL: http://sourceforge.net/project/showfiles.php?group_id=253689&package_id=310373&release_id=672270 SRPM URL: http://sourceforge.net/project/showfiles.php?group_id=253689&package_id=310373&release_id=672270 Some remarks: - The LICENSE file mentions "GNU GENERAL PUBLIC LICENSE Version 3, 29 June 2007" and the spec file GPL (W: invalid-license GPL). Change this in GPLv3 - The README file in the sources is just a text version of the main page? Would like to see a decent description and usage example of sslogger instead. - the sslogger.conf file should been explained in detail - The spec file: a/ W: non-standard-group User Interface: Group: User Interface => use one of the following: "User Interface/Desktops", "User Interface/X", "User Interface/X Hardware Support" b/ change License (see above) c/ W: incoherent-version-in-changelog 0.9-0.28 ['0.9-0.29', '0.9-0.29'] The last entry in %changelog contains a version identifier that is not coherent with the epoch:version-release tuple of the package. d/ W: no-url-tag The URL tag is missing. e/ W: conffile-without-noreplace-flag /etc/sslogger.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here f/ E: world-writable /usr/share/doc/sslogger/LICENSE 0666 A file or directory in the package is installed with world writable permissions, which is most likely a security issue. g/ W: non-standard-uid /usr/bin/sslogger slogger A file in this package is owned by a non standard user. Standard users are: root, bin, daemon, adm, lp, sync, shutdown, halt, mail, news, uucp, operator, games, gopher, ftp, nobody. => is there a specific reason to have a separate user/group for sslogger? h/ E: setuid-binary /usr/bin/sslogger slogger 06555 The file is setuid, this may be dangerous, especially if this file is setuid root. => explain/describe the reason in the README file for example. SYSAdmins should have a clear understanding of what kind of executable are installed on their systems. Any audit check will bring your executable up and a very good explanation should be given. So, help the poor sysadmin. i/ E: non-standard-dir-perm /var/log/sl 0750 A standard directory should have permission set to 0755. If you get this message, it means that you have wrong directory permissions in some dirs included in your package. %dir %attr(750,%{suser},%{sgroup}) /var/log/sl Why don't you make /var/log/sl mode 755 and create sub-dirs beneath with 750? Why don't you create a sub-dir per user account? However, it is still not clear why you cannot do it with root:root? Please explain. j/ sslogger.i386: W: log-files-without-logrotate /var/log/sl This package contains files in /var/log/ without adding logrotate configuration for them. k/ %defattr(-,root,root) => change this into: %defattr(-,root,root,-) l/ convince me of the need of %pre, %post, %prein. If you use the user root you can remove these. m/ Use %global instead of %define, unless you really need only locally defined submacros within other macro definitions (a very rare case): http://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_defined n/ I advise to read the wiki page "Frequently made mistakes while packaging RPMs by new packagers" as it will help you to improve your package - URL https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes OK, apologies for the delay. Had a series of personal issues to resolve. In reading through the comments, here is a summary of what we've got.. a/ no issue b/ no issue c/ no issue d/ no issue e/ no issue f/ no issue g/ * W: non-standard-uid /usr/bin/sslogger slogger h/ * E: setuid-binary /usr/bin/sslogger slogger 06555 i/ * E: non-standard-dir-perm /var/log/sl 0750 j/ in progress... k/ no issue l/ convince me of the need of %pre, %post, %prein m/ no issue n/ no issue README ... in progress So lets start a dialogue on the suid, user/group, and permission: Here is what I was looking to accomplish: - Create a binary which allows the user to gain access to another user's account, including root, whilst logging all keystrokes. - Create group of users "sloggers" whose members are permitted to review/audit other users activities. Hence the /i 750 Breaking it down, g/ I need a suid user/group, as I am reluctant to create a suid root binary. True the Makefile installed as suid root which the spec file later changes permission to the other "non standard" user/group in the %pre/%post. This is related to /k. h/ The log files are written at a lower privilege level and not as root. i/ The permission on the directory are set to exclude users that are not in the audit group "sloggers" l/ Needed to create a non-root logging audit directory. As stated, I am reluctant to crate a suid/sgid root binary. I am open to suggestions to resolve the above. Thanks -Ed Alright, looking forward on a new version of your spec file and source. To have an idea how others do it, have a look at for example: http://www.cora.nwra.com/~orion/fedora/amanda.spec I like the way they defined an user/group: %{!?amanda_user:%define amanda_user amandabackup} %{!?amanda_group:%define amanda_group disk} It's ok to have a suid binary if it is required. Thx By the way, would you post your new spec/srpm, Ed? SRPM and spec aval at: https://sourceforge.net/project/platformdownload.php?group_id=253689 -Ed Some pre-remarks: !!!! About license Well, actually almost all the files in this tarball are under GPLv3+, however sslogger.c is under "BSD with advertising", which conflicts with GPLv3+: https://fedoraproject.org/wiki/Licensing For this package, actually sl.c (under GPLv3+) actually calls execvp() for sslogger, which is written in sslogger.c (BSD with advertising), licenses really conflict. * Please consider to use %?dist macro: https://fedoraproject.org/wiki/Packaging/DistTag * BuildRoot does not honor Fedora's packaging guidelines: https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag * Source0 must be specified with full URL: https://fedoraproject.org/wiki/Packaging/SourceURL * Some needed Requires(pre) or so are missing: https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets * Fedora specific compilation flags are not correctly honored: https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags https://fedoraproject.org/wiki/Packaging/Debuginfo * Please use macros for standard directories. For example, you should use %{_localstatedir} for /var: https://fedoraproject.org/wiki/Packaging/RPMMacros * For binary names - In Fedora other rpm already uses the name "%{_bindir}/sl" - And IMO the name "%{_bindir}/replay" is too generic. - Also I recommend to change %log_dir to %_localstatedir/log/sslogger or so. * Fedora considers that deleting user/group automatically by rpm scriptlets is dangerous and this must be done manually by sysadmin -------------------------------------------------------------------- %post chown -R %{suser}.%{sgroup} %{log_dir} -------------------------------------------------------------------- * - is actually useless. This scriptlet is executed only when this rpm is installed or upgraded and does nothing when this rpm is actually used. * Fedora uses %{_defaultdocdir}/%{name}-%{version} as directory to install documents - By the way currently the directory %{_docdir}/sslogger/ itself is not owned by this package: https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership https://fedoraproject.org/wiki/Packaging/UnownedDirectories Setting FE-Legal I like the pre 11 way of License: distributable :) I don't claim to be a Licence/Leagal expert but does changing to below work? License: GPLv3 and BSD with advertising Once past the Licence stage, I'll address the remaining issues (In reply to comment #18) > I don't claim to be a Licence/Leagal expert but does changing to below work? > License: GPLv3 and BSD with advertising No. "GPLv3+" and "BSD with advertising" are incompatible. This means that you have to ask the upstream developer to change the license of the source (for this case the most possible way is that the upstream changes the license of sslogger.c to "BSD", not "BSD with advertising") * fully agree with the comments in #17 - thanks for the in-depth analysis Mamoru ==> the legal issue must be fixed * man page sslogger contains an error: FILES /etc/sloger.conf << /etc/sslogger.conf Configuration file * Concerning "replay" - I agree it would be better to rename it to e.g. slreplay $ replay /var/log/sl/2009/05/sl-fed-gdha-root-2009.05.27-09:24:31.log Sending output to: /dev/pts/4 /var/log/sl/2009/05/sl-fed-gdha-root-2009.05.27-09:24:31.log: Permission denied End replay ==> replay can only be used by people part of group sloggers. Why can a plain user not replay his own logs? ==> replay man page missing ==> some useful functions are still missing in replay such as find. I would rather see "/" as find symbol, rather then "f" (try to follow vi syntax) * It is rather confusing to see sometimes slogger and then sslogger and then sl e.g. in config.h: #define SLOGGER sslogger * The "-h" option is not respected: $ sl -h [sudo] password for gdha: $ sslogger -h Sloggerd started, file is /var/log/sl/2009/05/sl-fed-gdha-gdha-2009.05.27-10:04:37.log Reason for invoking thus interactive shell for gdha: * Found lots of spelling errors in source files and in the man page * why not combining the -u option in sslogger itself? Why using sl for that purpose? * The sl.log file contains not enough information where the log file itself is stored: # cat /var/log/sl/sl.log 2009-05-27 09:24:39 sslogger[30880]; user:gdha; as:root; invoked_shell:"/bin/bash"; logfile:sl-fed-gdha-root-2009.05.27-09:24:31.log; reason:test sslogger Actual location is: /var/log/sl/2009/05/sl-fed-gdha-root-2009.05.27-09:24:31.log To use replay (or cat) it would be better to log the full path of the log file. * why do I read in the log file Sloggerd started - it should read sslogger started # ps ax|grep ssl 31720 pts/2 S+ 0:00 sslogger 31721 pts/2 S+ 0:00 sslogger Because the original copyright holder on sslogger.c is the Regents of the University of California, this isn't a problem (because they have dropped the advertising clause on all code they are the copyright holder for). I've been meaning to write this up as a FAQ entry for some time now, and this was a good reminder: https://fedoraproject.org/wiki/Licensing:FAQ#BSD_with_advertising Please ask upstream to remove the advertising clause from that file. Lifting FE-Legal. (In reply to comment #21) > Because the original copyright holder on sslogger.c is the Regents of the > University of California, this isn't a problem (because they have dropped the > advertising clause on all code they are the copyright holder for). Ah, thank you for clarifying. To Ed: Now I will wait for you to update your srpm. Also would you address what Gratien pointed out? Awesome, Yes, expect an update.. Since /usr/bin/sl conflicts with the sl package (Steam Locomotive) Any one care to suggest a new name for sl before i pick one out of the hat -Ed (In reply to comment #25) > Any one care to suggest a new name for sl before i pick one out of the hat > > -Ed Maybe %_bindir/slog or so? New src rpm available at: https://sourceforge.net/projects/sslogger/files/sslogger-0.9/sslogger-0.9-43.fc10.src.rpm Addresses items Gratien pointed out: Changed /usr/bin/sl to /usr/bin/slog Changed replay to sreplay Added man pages for slog and sreplay Spec file fixes Added ability for users to review their own logs by modifying settings in sslogger.conf I cant yet add the -u option to sslogger, as slog relying on sudo for managing user access control. -Ed Srpm update, fix permission issue on log directory https://sourceforge.net/projects/sslogger/files/sslogger-0.9/sslogger-0.9-45.fc10.x86_64.rpm/download -Ed Please post the URL of your srpm instead of binary x86_64 rpm. Oops, Here you go: http://sourceforge.net/projects/sslogger/files/sslogger-0.9/sslogger-0.9-45.fc10.src.rpm/download Well, for 0.9-45: * Tarball - First of all, is the tarball used in the srpm formally released 0.9 tarball or 0.9 tarball is not yet released formally? * License - Now the license tag should be "GPLv3+" * %description ============================================================== %description A keystroke logging utility for privileged user escalation %{!?log_dir:%define log_dir /%{_localstatedir}/log/slog} %{!?sslogger_user:%define sslogger_user slogger} %{!?sslogger_group:%define sslogger_group sloggers} %if 0%{?rhel} Requires(pre): shadow-utils util-linux %else Requires(pre): shadow-utils util-linux-ng %endif ============================================================== %pre -------------------------------------------------------------- - These lines (before %pre like) are all in %description. Actually $ rpm -qi shows that "Requires(pre)" _comment_ is included in %description ! By the way on Fedora "util-linux-ng" provides "util-linux" so this conditional requires is not strictly needed. * Macros - Use %_sysconfdir instead of /etc. https://fedoraproject.org/wiki/Packaging/RPMMacros * Parallel make - Support parallel make if possible. If not possible, write some comments that parallel make is disabled on the spec file: https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make * Timestamps - When using "install" or "cp" commands, add "-p" option to keep timestamps on installed files: https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps * Permission - Please check the permissions of installed files and set them properly. * Usually normal files should have 0644 permission * From rpmlint: --------------------------------------------------------------- sslogger.i586: E: incoherent-logrotate-file /etc/logrotate.d/sslogger_rotate --------------------------------------------------------------- - It seems that this logrote file should be named as %_sysconfdir/logrotate.d/%{name} * %changelog format - It is useful in Fedora CVS system that one line is inserted between each %changelog entry like: --------------------------------------------------------------- %changelog * Sun Jul 05 2009 Ed Brand <edbrand> - 09-40 - Split man files into slog, sslogger and sreplay - Chmod 775 $log_dir to allow normal user access - Add check to slog to disallowe passing '-' when -c option is used * Tue May 18 2009 Ed Brand <edbrand> - 0.9-32 - removed $global * Sun Mar 29 2009 Ed Brand <edbrand> - 0.9-30 - Change Licence to GPLv3 - Misc. spec file fixes --------------------------------------------------------------- Updated: https://sourceforge.net/projects/sslogger/files/srpms/sslogger-0.9-48.fc10.src.rpm/download Would you answer this question? (In reply to comment #31) > Well, for 0.9-45: > > * Tarball > - First of all, is the tarball used in the srpm formally > released 0.9 tarball or 0.9 tarball is not yet released > formally? For -46: * Macros definition ------------------------------------------------------------ %{!?log_dir:%define log_dir /%{_localstatedir}/log/slog} %{!?sslogger_user:%define sslogger_user slogger} %{!?sslogger_group:%define sslogger_group sloggers} ------------------------------------------------------------ - Move these lines of defining macros to the top of the spec file. Defining macros between %description and %pre is error-prone. * Requires - By the way why is "Requires(pre): util-linux" needed? * Cosmetic issue - Well, for consistency please use %{_sysconfdir} instead of %_sysconfdir .... Then: ================================================================= NOTE: Before being sponsored: This package will be accepted with another few work. But before I accept this package, someone (I am a candidate) must sponsor you. Once you are sponsored, you have the right to review other submitters' review requests and approve the packages formally. For this reason, the person who want to be sponsored (like you) are required to "show that you have an understanding of the process and of the packaging guidelines" as is described on : http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored Usually there are two ways to show this. A. submit other review requests with enough quality. B. Do a "pre-review" of other person's review request (at the time you are not sponsored, you cannot do a formal review) When you have submitted a new review request or have pre-reviewed other person's review request, please write the bug number on this bug report so that I can check your comments or review request. Fedora package collection review requests which are waiting for someone to review can be checked on my wiki page: http://fedoraproject.org/wiki/User:Mtasaka#B._Review_request_tickets (Check "No one is reviewing") Review guidelines are described mainly on: http://fedoraproject.org/wiki/Packaging/ReviewGuidelines http://fedoraproject.org/wiki/Packaging/Guidelines http://fedoraproject.org/wiki/Packaging/ScriptletSnippets Thanks for the info, I will update this report with another review. In reply to #31, the tarball has since been published. Regarding the Requires(pre): util-linux The required file from that package is /sbin/nologin What is prefered, the package or the required file name? Updated spec, URL is: https://sourceforge.net/projects/sslogger/files/srpms/sslogger-0.9-49.fc10.src.rpm/download (In reply to comment #34) > In reply to #31, the tarball has since been published. - Thank you for reply. Actually the 0.9 tarball can be downloaded from the URL written in your spec file and it coincides with the one used in your srpm. > Regarding the Requires(pre): util-linux > The required file from that package is /sbin/nologin - Ah, fairly reasonable. Now I will wait for your another review request or your pre-review of other person's review request. ping? Pong. Please bear with me. My day job is sucking up way too much my time. I've have looked at your wiki/review requests. Expect an update shortly. -Ed ping again? I've started looking through new requested, and giving comments, beginng with Bug 517466 Well, okay. ----------------------------------------------------- This package (sslogger) is APPROVED by mtasaka ----------------------------------------------------- Please follow the procedure written on: http://fedoraproject.org/wiki/PackageMaintainers/Join from "Install the Client Tools (Koji)". Now I am sponsoring you. If you want to import this package into Fedora 10/11, you also have to look at http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT (after once you rebuilt this package on koji Fedora rebuilding system). If you have questions, please ask me. Removing NEEDSPONSOR. Cool, Thanks. ooking into importing into koji/bodhi -Ed First please write new package CVS request following http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure New Package CVS Request ======================= Package Name: sslogger Short Description: A keystroke logging utility for privileged user escalation Owners: ebrand Branches: F-10 F-11 EL-5 InitialCC: mtasaka Reading the doc, where is the fedora-cvs flag? It is in Bugzilla, look at the top of this page, you should see: Flags: (edit). cvs done. This seems NOT to be making it through to RawHide .. is there a blocker? [herrold@centos-5 ~]$ srcfind sslogger /home/herrold/.tmp/srcfind.cache.txt sslogger nil [herrold@centos-5 ~]$ Ed, would you actually build this package with koji? Ok, Following the steps on http://fedoraproject.org/wiki/PackageMaintainers/NewPackageProcess I imported into CVS, and ran make build for EL-5, FC-10&11 branches. [ebrand@f12 F-10]$ make build /usr/bin/koji build dist-f10-updates-candidate 'cvs://cvs.fedoraproject.org/cvs/pkgs?rpms/sslogger/F-10#sslogger-0_9-50_fc10' ... Question) In order to request the package be added to EL-5, F-10&11 I need to submit a bodhi request - make update? -Ed Well, - Please rebuild also for F-11 - For F-10/11/EL-5, please submit push requests on https://admin.fedoraproject.org/updates/ sslogger-0.9-50.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/sslogger-0.9-50.fc11 sslogger-0.9-50.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/sslogger-0.9-50.fc10 sslogger-0.9-50.el5 has been submitted as an update for Fedora EPEL 5. http://admin.fedoraproject.org/updates/sslogger-0.9-50.el5 Rebuilt for F-11, and requests submitted. Okay. Now closing. sslogger-0.9-50.el5 has been pushed to the Fedora EPEL 5 stable repository. If problems still persist, please make note of it in this bug report. sslogger-0.9-50.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report. sslogger-0.9-50.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report. |