[Bug 491430] Review Request: sslogger - A keystroke logging utility for privileged user escalation

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=491430





--- Comment #12 from Gratien D'haese <gratien.dhaese at gmail.com>  2009-04-10 04:11:50 EDT ---
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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.