Bug 456182

I'm not sure I'll be able to finish this review in a timely fashion so I'd
prefer for someone else to own the ticket; however, I'm happy to help start the
process. Here is my partial review according to revision 0.27 of
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines:

- MUST: rpmlint must be run on every package. The output should be posted in
  the review.

  $ ls /var/lib/mock/fedora-9-i386/result/
  build.log  root.log  rssh-2.3.2-1.fc9.i386.rpm  rssh-2.3.2-1.fc9.src.rpm
  rssh-debuginfo-2.3.2-1.fc9.i386.rpm  state.log

  $ (cd /var/lib/mock/fedora-9-i386/result/; rpmlint *.rpm)
  rssh.i386: E: setuid-binary /usr/libexec/rssh_chroot_helper root 04755
  rssh.i386: E: non-standard-executable-perm /usr/libexec/rssh_chroot_helper 04755
  3 packages and 0 specfiles checked; 2 errors, 0 warnings.

- MUST: The package must be named according to the Package Naming Guidelines .

  Good.

- MUST: The spec file name must match the base package %{name}, in the format
  %{name}.spec unless your package has an exemption on Package Naming
  %Guidelines .

  Good.

- MUST: The package must meet the Packaging Guidelines.
  
  - libexecdir note: "Packagers are highly encouraged to store libexecdir files
    in a package-specific subdirectory of %{_libexecdir}, such as
    %{_libexecdir}/%{name}." 

  - Are conf_convert.sh and mkchroot.sh really documentation?

  - Static linking note: "The packager must provide rationale for linking
    statically, including precedences where available, to FESCO for approval."

    The autotools scripts provided for compiling rssh presently detect a
    version of openssh >= 3.5 and decide to dynamically link rssh; however,
    packagers should remain aware of this sensitivity.

- MUST: The package must be licensed with a Fedora approved license and meet
  the Licensing Guidelines.

  Good.

- MUST: The License field in the package spec file must match the actual
  license.

  Good.

- MUST: If (and only if) the source package includes the text of the license(s)
  in its own file, then that file, containing the text of the license(s) for
  the package must be included in %doc.

  Good.

- MUST: The spec file must be written in American English.

  Good.

- MUST: The spec file for the package MUST be legible. If the reviewer is
  unable to read the spec file, it will be impossible to perform a review.
  Fedora is not the place for entries into the Obfuscated Code Contest
  (http://www.ioccc.org/).

  Good.

- MUST: The sources used to build the package must match the upstream source,
  as provided in the spec URL. Reviewers should use md5sum for this task. If no
  upstream URL can be specified for this package, please see the Source URL
  Guidelines for how to deal with this.

  $ wget http://downloads.sourceforge.net/rssh/rssh-2.3.2.tar.gz
  $ md5sum rssh-2.3.2.tar.gz 
  65712f2c06ff5fc6fc783bc8c2e4e1ba  rssh-2.3.2.tar.gz

  $ rpmdev-extract rssh-2.3.2-1.fc9.src.rpm
  $ md5sum rssh-2.3.2-1.fc9/rssh-2.3.2.tar.gz
  65712f2c06ff5fc6fc783bc8c2e4e1ba  rssh-2.3.2-1.fc9/rssh-2.3.2.tar.gz

- MUST: The package must successfully compile and build into binary rpms on at
  least one supported architecture.

  The package built successfully under the fedora-9-i386 mock configuration on
  an i386 machine.

- MUST: If the package does not successfully compile, build or work on an
  architecture, then those architectures should be listed in the spec in
  ExcludeArch.

  NOT CHECKED.

- MUST: All build dependencies must be listed in BuildRequires, except for any
  that are listed in the [wiki:Self:Packaging/Guidelines#Exceptions exceptions
  section of Packaging Guidelines] ; inclusion of those as BuildRequires is
  optional. Apply common sense.

  NOT CHECKED.

- MUST: The spec file MUST handle locales properly. This is done by using the
  %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.

  N/A.

- MUST: Every binary RPM package which stores shared library files (not just
  symlinks) in any of the dynamic linker's default paths, must call ldconfig in
  %post and %postun. If the package has multiple subpackages with libraries,
  each subpackage should also have a %post/%postun section that calls
  /sbin/ldconfig. An example of the correct syntax for this is:

  Good.

- MUST: If the package is designed to be relocatable, the packager must state
  this fact in the request for review, along with the rationalization for
  relocation of that specific package. Without this, use of Prefix: /usr is
  considered a blocker.

  Good.

- MUST: A package must own all directories that it creates. If it does not
  create a directory that it uses, then it should require a package which does
  create that directory. Refer to the Guidelines for examples.

  Good.

- MUST: A package must not contain any duplicate files in the %files listing.

  Good.

- MUST: Permissions on files must be set properly. Executables should be set
  with executable permissions, for example. Every %files section must include a
  %defattr(...) line.

  - Do setuid-root executables require any special comment?

- MUST: Each package must have a %clean section, which contains rm -rf
  %{buildroot} ([wiki:Self:Packaging/Guidelines#UsingBuildRootOptFlags or
  %$RPM_BUILD_ROOT] ).

  Good.

- MUST: Each package must consistently use macros, as described in the
  [wiki:Self:Packaging/Guidelines#macros macros section of Packaging
  Guidelines] .

  Good.

- MUST: The package must contain code, or permissable content. This is
  described in detail in the [wiki:Self:Packaging/Guidelines#CodeVsContent code
  vs. content section of Packaging Guidelines] .

  Good.

- MUST: Large documentation files should go in a -doc subpackage. (The
  definition of large is left up to the packager's best judgement, but is not
  restricted to size. Large can refer to either size or quantity)

  Good.

- MUST: If a package includes something as %doc, it must not affect the runtime
  of the application. To summarize: If it is in %doc, the program must run
  properly if it is not present.

  Good. (on surface inspection)

- MUST: Header files must be in a -devel package.

  N/A.

- MUST: Static libraries must be in a -static package.

  N/A.

- MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
  (for directory ownership and usability).

  N/A.

- MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1),
  then library files that end in .so (without suffix) must go in a -devel
  package.

  N/A. 

- MUST: In the vast majority of cases, devel packages must require the base
  package using a fully versioned dependency: Requires: %{name} =
  %{version}-%{release}

  N/A.

- MUST: Packages must NOT contain any .la libtool archives, these should be
  removed in the spec.

  Good.

- MUST: Packages containing GUI applications must include a %{name}.desktop
  file, and that file must be properly installed with desktop-file-install in
  the %install section. This is described in detail in the
  [wiki:Self:Packaging/Guidelines#desktop desktop files section of Packaging
  Guidelines] . If you feel that your packaged GUI application does not need a
  .desktop file, you must put a comment in the spec file with your explanation.

  Good.

- MUST: Packages must not own files or directories already owned by other
  packages. The rule of thumb here is that the first package to be installed
  should own the files or directories that other packages may rely upon. This
  means, for example, that no package in Fedora should ever share ownership
  with any of the files or directories owned by the filesystem or man package.
  If you feel that you have a good reason to own a file or directory that
  another package owns, then please present that at package review time.

  Good.

- MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}
  ([wiki:Self:Packaging/Guidelines#UsingBuildRootOptFlags or $RPM_BUILD_ROOT]
  ). See [wiki:Self:Packaging/Guidelines#PreppingBuildRootForInstall Prepping
  BuildRoot For %install] for details.

  Good.

- MUST: All filenames in rpm packages must be valid UTF-8.

  Good.

SHOULD Items:

- SHOULD: If the source package does not include license text(s) as a separate
  file from upstream, the packager SHOULD query upstream to include it.

  N/A.

- SHOULD: The description and summary sections in the package spec file should
  contain translations for supported Non-English languages, if available.

  N/A.

- SHOULD: The reviewer should test that the package builds in mock. See
  [wiki:Self:PackageMaintainers/MockTricks MockTricks] for details on how to do
  this.

  Good. (Tested fedora-9-i386 on i386.)

- SHOULD: The package should compile and build into binary rpms on all
  supported architectures.

  NOT CHECKED.

- SHOULD: The reviewer should test that the package functions as described. A
  package should not segfault instead of running, for example.

  NOT CHECKED.

- SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague,
  and left up to the reviewers judgement to determine sanity.

  N/A.

- SHOULD: Usually, subpackages other than devel should require the base package
  using a fully versioned dependency.

  N/A.

- SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and
  this is usually for development purposes, so should be placed in a -devel
  pkg. A reasonable exception is that the main pkg itself is a devel tool not
  installed in a user runtime, e.g. gcc or gdb.

  N/A.

- SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
  /usr/bin, or /usr/sbin consider requiring the package which provides the file
  instead of the file itself. Please see File Dependencies in the Guidelines
  for further information. 

  N/A.

MUST Items: 

xx - rpmlint is unclean on RPM (can be ignored)
    + [rishi@ginger x86_64]$ rpmlint rssh-2.3.2-1.fc8.x86_64.rpm
      rssh.x86_64: E: setuid-binary /usr/libexec/rssh_chroot_helper root 04755
      rssh.x86_64: E: non-standard-executable-perm
/usr/libexec/rssh_chroot_helper 04755
      [rishi@ginger x86_64]$ 

OK - follows Naming Guidelines
OK - spec file is named as %{name}.spec

OK - package does not meet Packaging Guidelines
    + To preserve timestamps you could consider using:
      make install INSTALL="%{__install} -p" DESTDIR=$RPM_BUILD_ROOT
    + https://fedoraproject.org/wiki/Packaging/Guidelines#Libexecdir suggests
that files be put into package-specific subdirectories. Can this be done?

OK - license meets Licensing Guidelines
OK - License field meets actual license
OK - upstream license file included in %doc
OK - spec file uses American English
OK - spec file is legible
OK - sources match upstream sources
OK - package builds successfully
OK - ExcludeArch not needed

OK - build dependencies correctly listed
    + It might be a good idea to add cvs, rdist and rsync to BuildRequires,
because the configure script hard-codes their path to /usr/bin/cvs,
/usr/bin/rdist, and /usr/bin/rsync, when they are absent.

OK - no locales
OK - no shared libraries
OK - package is not relocatable
OK - file and directory ownership
OK - no duplicates in %file

xx - file permissions set properly
    + The preferred attribute definition is: %defattr(-,root,root,-). If you use
it, the  %attr(755, root, root) and %attr(4755, root, root) become redundant.
Since the example scripts will be retaining their executable bits, they can be
turned off somewhere in the spec (maybe the %setup stanza).
    + The rssh(1) manual says:
      Additionally,  create  a group, for example "rsshuser", for rssh users.
      Put all your users who will be restricted by rssh in that  group.   Set
      the  ownership  and  permissions on rssh and rssh_chroot_helper so that
      only those users can  execute  them.   The  following  commands  should
      illustrate:
      # groupadd rsshuser
      # chown root:rsshuser rssh rssh_chroot_helper
      # chmod 550 rssh
      # chmod 4550 rssh_chroot_helper
      Fedora's packaging guidelines for users and groups
(https://fedoraproject.org/wiki/Packaging/UsersAndGroups) might then come into
the picture.

OK - %clean present

OK - macros used consistently
OK - contains code and permissable content
OK - -doc is not needed
OK - contents of %doc does not affect the runtime
OK - no header files
OK - no static libraries
OK - no pkgconfig files
OK - no library files
OK - -devel is not needed
OK - no libtool archives
OK - %{name}.desktop file not needed
OK - does not own files or directories owned by other packages
OK - buildroot correctly prepped
OK - all file names valid UTF-8

SHOULD Items:

OK - upstream provides license text
xx - no translations for description and summary
OK - package builds in mock successfully
OK - package builds on all supported architectures
OK - package functions as expected
OK - scriptlets are sane
OK - subpackages are not needed
OK - no pkgconfig files
OK - no file dependencies

I was wondering whether rssh should add an entry in /etc/shells or not?

http://sundaram.fedorapeople.org/packages/rssh.spec
http://sundaram.fedorapeople.org/packages/rssh-2.3.2-2.fc9.src.rpm

(In reply to comment #4)

+ You are not removing rssh from /etc/shell on removal of the package. It should be removed in %postun.

Here is how bash does it:

%post

HASBASH=""
HASSH=""

if [ ! -f /etc/shells ]; then
  > /etc/shells
fi

(while read line ; do
  if [ "$line" = "/bin/bash" ]; then
    HASBASH=1
  elif [ "$line" = "/bin/sh" ]; then
    HASSH=1
  fi
 done

 if [ -z "$HASBASH" ]; then
  echo "/bin/bash" >> /etc/shells
 fi
 if [ -z "$HASSH" ]; then
  echo "/bin/sh" >> /etc/shells
fi) < /etc/shells

%postun
if [ "$1" = 0 ]; then
    /bin/grep -v '^/bin/bash$' < /etc/shells | \
      /bin/grep -v '^/bin/sh$' > /etc/shells.new
    /bin/mv /etc/shells.new /etc/shells
fi

Here is how tcsh does it:

%post
if [ ! -f /etc/shells ]; then
	echo "%{_bindir}/tcsh" >> /etc/shells
	echo "%{_bindir}/csh"  >> /etc/shells
else
	grep -q '^%{_bindir}/tcsh$' /etc/shells || \
	echo "%{_bindir}/tcsh" >> /etc/shells
	grep -q '^%{_bindir}/csh$'  /etc/shells || \
	echo "%{_bindir}/csh"  >> /etc/shells
fi

%postun
if [ ! -x %{_bindir}/tcsh ]; then
	grep -v '^%{_bindir}/tcsh$'  /etc/shells | \
	grep -v '^%{_bindir}/csh$' > /etc/shells.rpm
	cat /etc/shells.rpm > /etc/shells && rm /etc/shells.rpm
fi

Use whichever one you like. Personally, I like the latter. Looks more readable. :-)

Let me know if this looks good

http://sundaram.fedorapeople.org/packages/rssh.spec
http://sundaram.fedorapeople.org/packages/rssh-2.3.2-3.fc10.src.rpm

Created attachment 313897 [details]
Patch to incorporate Spec file fixes.

(In reply to comment #6)

> http://sundaram.fedorapeople.org/packages/rssh.spec
> http://sundaram.fedorapeople.org/packages/rssh-2.3.2-3.fc10.src.rpm

+ The example scripts provided as documentation should not have their executable bits set.

+ https://fedoraproject.org/wiki/Packaging/Guidelines#Libexecdir suggests that files be put into package-specific subdirectories. Can this be done? 

+ The %pre scriptlet does not follow the guidelines for users and groups (https://fedoraproject.org/wiki/Packaging/UsersAndGroups).

You need to add 'Requires(pre): shadow-utils' and the scriptlet needs to end with an 'exit 0'. I think '|| :' also has the same effect as 'exit 0', but you might want to be pedantic and be safe.

+ You have mistakenly put fish instead of rssh in the Spec comments.

+ You might want to split the %doc in multiple lines to achieve the 72/80 character rule. But it is a matter of style and upto you.

+ As I had mentioned earlier, the rssh(1) manual recommends:
      # chown root:rsshuser rssh rssh_chroot_helper
      # chmod 550 rssh
      # chmod 4550 rssh_chroot_helper

Please find attached a patch which incorporates these changes. I have deliberately not bumped the release and added a %changelog. It is your package update them as you deem fit.

These changes lead to the following rpmlint issues (which can be ignored):
    $ rpmlint rssh
    rssh.x86_64: E: non-standard-gid /usr/bin/rssh rsshusers
    rssh.x86_64: E: non-readable /usr/bin/rssh 0750
    rssh.x86_64: E: non-standard-executable-perm /usr/bin/rssh 0750
    rssh.x86_64: E: non-standard-gid /usr/libexec/rssh_chroot_helper rsshusers
    rssh.x86_64: E: setuid-binary /usr/libexec/rssh_chroot_helper root 04750
    rssh.x86_64: E: non-standard-executable-perm /usr/libexec/rssh_chroot_helper 04750
    rssh.x86_64: E: non-standard-executable-perm /usr/libexec/rssh_chroot_helper 04750
    rssh.x86_64: E: no-binary
    rssh.x86_64: W: dangerous-command-in-%postun mv
    $

However using -i reveals some interesting avenues:

+ You might want to add /usr/bin/rssh to the list of files which are not readable by everyone in Fedora.

    rssh.x86_64: E: non-readable /usr/bin/rssh 0750
    The file can't be read by everybody. If this is expected (for security
    reasons), contact your rpmlint distributor to get it added to the list of
    exceptions for your distro (or add it to your local configuration if you
    installed rpmlint from the source tarball).

+ Can we have rssusers as a standard group in Fedora?

    rssh.x86_64: E: non-standard-gid /usr/bin/rssh rsshusers
    A file in this package is owned by a non standard group.
    Standard groups are:
    root, bin, daemon, sys, adm, tty, disk, lp, mem, kmem, wheel, mail,
    news, uucp, man, games, gopher, dip, ftp, lock, nobody, users

    rssh.x86_64: E: non-standard-gid /usr/libexec/rssh_chroot_helper rsshusers
    A file in this package is owned by a non standard group.
    Standard groups are:
    root, bin, daemon, sys, adm, tty, disk, lp, mem, kmem, wheel, mail,
    news, uucp, man, games, gopher, dip, ftp, lock, nobody, users

[rpmbuild@rocky ~]$ rp rssh-2.3.2-3.fc10.src.rpm 
 rssh.src: W: strange-permission rssh-2.3.2.tar.gz 0755
 rssh.src: W: strange-permission rssh.spec 0755
 rssh.src: W: strange-permission rssh-2.3.2-makefile.patch 0755

 755 is bad

 664 will be okay.

Applied patch

http://sundaram.fedorapeople.org/packages/rssh.spec
http://sundaram.fedorapeople.org/packages/rssh-2.3.2-4.fc10.src.rpm

The permissions and group is as suggested by upstream and inherited from the upstream spec file. Those are retained as it is for security reasons. I think the definition of a "standard" group doesn't include rssh but only the historically core components of a what is a POSIX system.

+---------------------------------+
| This package is APPROVED by me. |
+---------------------------------+

Ping?

Would be nice if you could file a request for CVS so that we can have rssh in Fedora.

New Package CVS Request
=======================
Package Name:rssh
Short Description: Restricted shell for use with OpenSSH, allowing only scp and/or sftp
Owners:sundaram
Branches:F-8 F-9
InitialCC:cvsextras

cvs done. 

If it works there, consider branching and maintaining this for EPEL as well?

I am considering letting it cook in rawhide for a while. There is a rsync v3 compatibility patch that OLPC folks want as well. So that gets to go in before I branch for EPEL or older releases.

Is there anything which is keeping us from closing this?

Yes, I am waiting to add someone from OLPC as a co-maintainer and do additional branches. I have only built for rawhide now. Doing other branches will let Bodhi close the review request eventually.

Actually, rssh should *absolutely* *not* be added to /etc/shells.  This file lists shells which should be considered valid login shells.  rssh is not, nor is it intended to be, a valid login shell... it's a specialized shell intended to provide extremely restricted access.

One consequence of adding rssh to /etc/shells is that if the machine in question has an FTP server configured, then depending on which server and how it is configured, any user whose shell is rssh might be granted access to the FTP service unintentionally.  This would defeat the purpose of configuring the user's shell to be rssh! Other installed programs may also check /etc/shells for a similar purpose.

Please remove that functionality from the post and postun portions of the spec file.

[I am the original author of RSSH.]

Some additional examples of badness that can occur if rssh is listed in /etc/shells:

A malicious user could walk up to someone's terminal while they are away (or even not looking), quickly run chsh (setting it to rssh), and log the user out, effectively denying them login access to the machine.

GDM will populate the user browser with an entry for that user, despite the fact that they will be unable to log in.

Sendmail may allow users to execute arbitrary programs via .forward if their shell is rssh and it is listed in /etc/shells.

getusershell() will return incorrect information about which shells are valid login shells.

> Actually, rssh should *absolutely* *not* be added to /etc/shells.  This file
> lists shells which should be considered valid login shells.  rssh is not, nor
> is it intended to be, a valid login shell... it's a specialized shell intended
> to provide extremely restricted access.

Thanks Derek for that feedback!

> Some additional examples of badness that can occur if rssh is listed in
> /etc/shells:
>
> A malicious user could walk up to someone's terminal while they are away (or
> even not looking), quickly run chsh (setting it to rssh), and log the user out,
> effectively denying them login access to the machine.
>
> GDM will populate the user browser with an entry for that user, despite the
> fact that they will be unable to log in.
>
> Sendmail may allow users to execute arbitrary programs via .forward if their
> shell is rssh and it is listed in /etc/shells.
>
> getusershell() will return incorrect information about which shells are valid
> login shells.

Well, /etc/shells also has /sbin/nologin. Won't that cause some of the above problems too?

(In reply to comment #19)

> Well, /etc/shells also has /sbin/nologin. Won't that cause some of the above
> problems too?

Indeed it would...  All of the above would apply to nologin as well, though in some cases (e.g. the ftp server case and the sendmail .forward case) the exact configuration of the system can come into play. 

I'm not really at liberty to test any of this at the moment, but I think the chsh DoS example should be easy to reproduce...  If you can change your shell to nologin, you can lock yourself out of the system (and so can an opportunistic malicious user), requiring root intervention.

Bear in mind that the list of examples I provided is by no means exhaustive...

Actually I just tried the chsh thing on an old red hat 9 system I had laying around...  Smartly, chsh asks for the user's password before changing the shell.  However it does otherwise work... so if someone changed the PAM settings for chsh it could still be an issue. The other things I mentioned also would be an issue.

So does this make the presence of /sbin/nologin in /etc/shells a problem?

Rahul, can you please revert the change to add rssh to /etc/shells?

Also note that it's less bad for nologin, since nologin doesn't allow anyone to copy files (i.e. via SSH) to, say, the user's .forward file.  Whereas rssh is expressly designed to allow this. :)  One might make a case that nologin should be in /etc/shells so that system users can have working .forward files...  I would counter that a better way to handle that would be via some other mechanism (e.g. /etc/aliases can send mail to a program for a specified user).

Yeah, I think it's still not the right thing...

(In reply to comment #22)
> So does this make the presence of /sbin/nologin in /etc/shells a problem?
> 
> Rahul, can you please revert the change to add rssh to /etc/shells?

Rahul asked fedora-devel-list if someone could do this for him and I went ahead.

http://koji.fedoraproject.org/koji/taskinfo?taskID=913061

Is there anything which is keeping us from closing this? It has been more than 2 months now.

As per https://bugzilla.redhat.com/show_bug.cgi?id=573618, I'd like to request EPEL branches for rssh.

Package Change Request
======================
Package Name: rssh
New Branches: el5 el6
Owners: xavierb sundaram

Git done (by process-git-requests).