Bug 317101 - Review Request: rats - Rough Auditing Tool for Security
Review Request: rats - Rough Auditing Tool for Security
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-03 13:01 EDT by Scott J Henson
Modified: 2014-07-03 07:53 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-29 12:36:35 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Scott J Henson 2007-10-03 13:01:41 EDT
Spec URL: http://sjhserv.net/~sjh/rats.spec
SRPM URL: http://sjhserv.net/~sjh/rats-2.1-1.fc8.src.rpm
Description: 
RATS scans through code, finding potentially dangerous function calls.
The goal of this tool is not to definitively find bugs (yet). The 
current goal is to provide a reasonable starting point for performing 
manual security audits.

The initial vulnerability database is taken directly from things that
could be easily found when starting with the forthcoming book, 
"Building Secure Software" by Viega and McGraw.  

RATS is released under version 2 of the GNU Public License (GPL).
Comment 1 Lubomir Kundrak 2007-10-03 13:10:59 EDT
Taking this for review.
Comment 2 Lubomir Kundrak 2007-10-08 09:31:40 EDT
Scott: thanks for the package, it looks very well. Here are some random things
that would be nice to be looked at before this this gets approval:

 25 RATS is released under version 2 of the GNU Public License (GPL).

This is not necessary, License: tag is for this.

 30 ./configure  --prefix=/usr \
 31             --datadir=%{_datadir}/rats
 32 
 33 %build
 34 CFLAGS="$RPM_OPT_FLAGS"; export CFLAGS

Why not use the %configure macro? It would set the CFLAGS for you, and possibly
correct libdir on 64 bit architectures, etc.

 38 %install
 39 [ "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT

Do not conditionally remove the build root. Just delete it, it is never root.

 45 # rm -rf $RPM_BUILD_ROOT
 46 # mkdir -p $RPM_BUILD_ROOT/usr
 47 # make install prefix=$RPM_BUILD_ROOT
 48 # mv bin share $RPM_BUILD_ROOT/usr/

Why is this commented? Why do you install stuff by hand instead of make install?
It does not have to be wrong, just please always write comments in SPEC in case
you do something nonstandard.

 53 #make clean

Also, never make useless comments like this. Just remove it. Also applies to
what is below the %files section.

 77 * Wed Sep 26 2007 Scott Henson <shenson@redhat.com>
 78  - 2.1-1: Initial packaged version

The correct format of the changelog entry would be:

 77 * Wed Sep 26 2007 Scott Henson <shenson@redhat.com> - 2.1-1
 78 - Initial packaged version
Comment 3 Lubomir Kundrak 2007-10-08 09:36:09 EDT
RPMLint:

> $ rpmlint /home/lkundrak/rpmbuild/SRPMS/rats-2.1-1.fc8.src.rpm
> rats.src:32: E: configure-without-libdir-spec

Just do a
$ expand rats.spec >x; mv x rats.spec

> rats.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 11)

This would be solved using the %configure macro, as I wrote in comment #2

> $ rpmlint /home/lkundrak/rpmbuild/RPMS/i386/rats-2.1-1.fc8.i386.rpm
> $ rpmlint /home/lkundrak/rpmbuild/RPMS/i386/rats-debuginfo-2.1-1.fc8.i386.rpm
> $ 

Fine
Comment 4 Lubomir Kundrak 2007-10-08 09:49:57 EDT
I can not compare the upstream version with the packaged at the time as the
upstream web is not responding at the time.

The patch rats-2.1-linux.diff doesn't seem right to me;
1.) files rats-2.1/lex.yy*.c are autogenerated. Why don't you regenerate them
instead of patching?
2.) You seem to be patching unrelated issues in this one patch. Please eplit it
into multiple patches.

When I was talking about libdir in comment #2 and comment #3, I did not notice
that the package doesn't make use of libdir. Please ingore it.

Another bad thing in .SPEC:

  8 URL:            http://www.securesw.com/rats/rats-%{version}.tar.gz
  9 Source0:        rats-2.1.tar.gz

The URL tag is for the web page of the software's author, Source0 is for URIs of
the files. It should look like this instead:

  8 URL:            http://www.securesw.com/rats/
  9 Source0:        http://www.securesw.com/rats/rats-%{version}.tar.gz

Apart from these issues, the package meets the Packing Guidelines, and I will
approve it when the issues are resolved.
Comment 5 Lubomir Kundrak 2007-10-08 10:30:50 EDT
Builds fine in mock for fedora-devel-i386
Comment 6 Scott J Henson 2007-10-08 15:15:28 EDT
Sorry I haven't responded, I didn't see the update.  

As for the upstream, securesw was bought out by fortify.  I was contacting them
with the Debian maintainer while I put the package together so that never got
updated.  The next version will have the corrected URL.

I will take care of the other problems now as well.  Thank you.
Comment 7 Scott J Henson 2007-10-08 15:33:55 EDT
A new verion is now avaliable at http://sjhserv.net/~sjh/Rats/.  I believe all
the above problems should be fixed.  Thank you.
Comment 8 Lubomir Kundrak 2007-10-09 05:47:43 EDT
Just a note, so this doesn't look stalled; we discussed the patch split on IRC
and are waiting for a response from Debian patch maintainer. Also, Scott:
Mentioning that the patch comes from Debian should be appropriate in patch's header.
Comment 9 Scott J Henson 2007-10-09 15:31:45 EDT
New version avaliable.
SRPM: http://sjhserv.net/~sjh/Rats/rats-2.1-3.fc8.src.rpm
SPEC: http://sjhserv.net/~sjh/Rats/rats.spec

* Tue Oct 9 2007 Scott Henson <shenson@redhat.com> - 2.1-3
 - Break the monolithic patch into pieces
 - Build Clean, contains build cleanups and spelling corrections
 - Php, adds support for php3 and php4 files
 - Report, adds some html output cleanup
 - Lex, some lex bug fixes
 - GTK-Vuln, adds some gtk vulnerabilities
 - Also generate lex output files on each build.

As for the Debian maintainer, the lex-yy*.c stuff just slipped in as a result of
the Debian build process.  I missed him calling make lex in the debian/rules
file.  I now call this in the spec, so its now building the lex files every time.  
Comment 10 Scott J Henson 2007-10-09 15:53:12 EDT
The latest version doesn't work because the configure macro sets datadir to
/usr/share and rats wants it at /usr/share/rats.  I can do a configure myself,
but then I get the following error.

rats.src:40: E: configure-without-libdir-spec

It appears above as well and I think you said I could ignore that since rats
doesn't use lib.  Is that right? Or shold I do something to take care of this? 
If I can just ignore it, new version:

SRPM: http://sjhserv.net/~sjh/Rats/rats-2.1-3.fc8.src.rpm
SPEC: http://sjhserv.net/~sjh/Rats/rats.spec
Comment 11 Scott J Henson 2007-10-09 21:46:12 EDT
New version with fixes for what was discussed in irc.

SRPM: http://sjhserv.net/~sjh/Rats/rats-2.1-5.fc8.src.rpm
SPEC: http://sjhserv.net/~sjh/Rats/rats.spec

* Tue Oct 9 2007 Scott Henson <shenson@redhat.com> - 2.1-5
 - Change the Makefile.in so we can use the configure macro
 - Rename all patches to .patch to be more standard
Comment 12 Lubomir Kundrak 2007-10-10 03:17:04 EDT
Tadaa, the package is beuatiful; ready to go to CVS.

APPROVED
Comment 13 Scott J Henson 2007-10-11 16:06:42 EDT
New Package CVS Request
=======================
Package Name: rats
Short Description: Rough Auditing Tool for Security
Owners: shenson
Branches: F-7
InitialCC: 
Cvsextras Commits: no
Comment 14 Kevin Fenzi 2007-10-12 13:44:25 EDT
cvs done
Comment 15 Christopher Meng 2014-07-03 02:47:27 EDT
Package Change Request
======================
Package Name: rats
New Branches: f20
Owners: cicku
Comment 16 Jon Ciesla 2014-07-03 07:51:41 EDT
Branch exists, ownership is handled in pkgdb.

Note You need to log in before you can comment on or make changes to this bug.