Bug 576431 - (skipfish) Package Review: skipfish - Web application security scanner
Package Review: skipfish - Web application security scanner
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
12
All Linux
low Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-SECLAB
  Show dependency treegraph
 
Reported: 2010-03-23 21:58 EDT by Michal Ambroz
Modified: 2011-02-07 12:55 EST (History)
8 users (show)

See Also:
Fixed In Version: skipfish-1.84-0.1.b.el5
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-05-10 13:09:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tmraz: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Proposed patch (16.61 KB, patch)
2010-04-26 16:48 EDT, Tomas Mraz
no flags Details | Diff
Patch with fixed ADD_STR_DATA (17.12 KB, patch)
2010-04-27 05:56 EDT, Tomas Mraz
no flags Details | Diff
Proposed patch (17.34 KB, patch)
2010-04-27 13:58 EDT, Tomas Mraz
no flags Details | Diff

  None (edit)
Description Michal Ambroz 2010-03-23 21:58:21 EDT
SPEC Url: http://rebus.webz.cz/d/skipfish.SPEC
SRPM Url: http://rebus.webz.cz/d/skipfish-1.16b-1.fc12.src.rpm

A fully automated, active web application security reconnaissance tool 
released recently by Google.

Key features:
High speed: pure C code, highly optimized HTTP handling, minimal CPU
footprint - easily achieving 2000 requests per second with responsive targets.

Ease of use: heuristics to support a variety of quirky web frameworks
and mixed-technology sites, with automatic learning capabilities, on-the-fly

word-list creation, and form auto-completion.
Cutting-edge security logic: high quality, low false positive, differential
security checks, capable of spotting a range of subtle flaws, including blind
injection vectors.

---
Hello.
Please can anyone review skipfish package - new security scanning tool from WebShere?

Note that current release is beta and there are some known bugs - for example the issues with D_FORTIFY_SOURCE.

Best regards
Michal Ambroz
Comment 1 Michal Ambroz 2010-03-25 03:52:00 EDT
Updated to version 1.25b
SPEC URL: http://rebus.webz.cz/d/skipfish.SPEC
SRPM URL: http://rebus.webz.cz/d/skipfish-1.25b-1.fc12.src.rpm
Comment 2 Martin Gieseking 2010-03-27 09:36:44 EDT
Just a couple of quick comments:

- if the final upstream release will be 1.25 (i.e. drops the trailing "b"), change the version number to 1.25 and the release tag to 0.1.b%{dist} (see http://fedoraproject.org/wiki/PackageNamingGuidelines#Pre-Release_packages)

- the package doesn't build in mock because of a missing BR: libidn-devel

- in the %files section, replace %{_datadir}/* 
  with %{_datadir}/%{name}/

- also in the %files section, replace %attr(755,root,root) /usr/bin/skipfish 
  with %{_bindir}/%{name}
Comment 3 Michal Ambroz 2010-03-28 13:50:31 EDT
Updated to version 1.26b
Incorporated changes to the specfile as suggested by Martin Gieseking

SPEC URL: http://rebus.webz.cz/d/skipfish.SPEC
SRPM URL: http://rebus.webz.cz/d/skipfish-1.26-0.1.b.fc12.src.rpm
Comment 5 Martin Gieseking 2010-03-29 05:16:54 EDT
A couple of further remarks:

- you can drop "%attr(755,root,root)" from the %files section because the permissions are set automatically

- Directory /usr/share/skipfish/assets contains a file COPYING with the GPL 3.0 license text. You should ask upstream whether the images are actually intended to be licensed under GPLv3. If so, change the License tag to "ASL 2.0 and GPLv3".

- Please also upload the SPEC file to your server. The above SPEC URLs don't work (404).
Comment 6 Michal Ambroz 2010-03-29 08:24:35 EDT
SPEC URL: http://rebus.webz.cz/d/skipfish.spec
SRPM URL: http://rebus.webz.cz/d/skipfish-1.26-0.2.b.fc12.src.rpm    

Hello Martin,
thank you for the review.

>- you can drop "%attr(755,root,root)" from the %files section because the
>permissions are set automatically
Done

>- Directory /usr/share/skipfish/assets contains a file COPYING with the GPL 3.0
>license text. 
Thanks for noticing that. License is actually LGPLv3. 

>You should ask upstream whether the images are actually intended
>to be licensed under GPLv3.
>If so, change the License tag to "ASL 2.0 and GPLv3".
Icons are comming from different project so they need to keep different license.
Following the licensing guideline I have split skipfish to 2 packages with different licenses (http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios)

>- Please also upload the SPEC file to your server. 
> The above SPEC URLs don't work (404).    
I a sorry for the typo ... it should have been .spec not .SPEC

Best regards
Michal Ambroz
Comment 7 Michal Ambroz 2010-04-02 16:13:20 EDT
SPEC URL: http://rebus.webz.cz/d/skipfish.spec
SRPM URL: http://rebus.webz.cz/d/skipfish-1.29-0.1.b.fc12.src.rpm   

Updated to version 1.29b
Comment 10 Martin Gieseking 2010-04-17 08:30:45 EDT
Hi Michal,

two additional remarks:

- The header file string-inl.h is licensed under BSD (3 clause variant). Thus, you must add BSD to the License field.

- You can drop the redundant %dir lines in the %files section.
Comment 11 Michal Ambroz 2010-04-17 22:08:54 EDT
Hello Martin,
thank you for review - here is the updated package.

SPEC URL: http://rebus.fedorapeople.org/fedora/12/SPECS/skipfish.spec
SRPM URL: 
http://rebus.fedorapeople.org/fedora/12/SRPMS/skipfish-1.31-0.2.b.fc12.src.rpm

Please could you specify what dir lines you find redundant?
Do I understand it right that instead of :
%dir %{_datadir}/%{name}/dictionaries
%{_datadir}/%{name}/*


You would like to see:
%{_datadir}/%{name}/dictionaries
Comment 12 Martin Gieseking 2010-04-18 02:56:21 EDT
(In reply to comment #11)
> Please could you specify what dir lines you find redundant?

Hi Michal,

you can omit all %dir lines in your spec file. These lines simply add empty directories to the binary rpm and would only be necessary if no files were supposed to be added to these directories. But since the %file section contains lines that place files in there, rpm creates the required directories automatically:

%{_datadir}/%{name}/assets/index.html creates %{_datadir}/%{name} and %{_datadir}/%{name}/assets, and finally puts index.html in "assets". Thus, there's no need for 
%dir %{_datadir}/%{name} and
%dir %{_datadir}/%{name}/assets.

Please also append a slash to "dictionaries" in order to make clear a folder (including all containing files) is added: 
%{_datadir}/%{name}/dictionaries/
It's not required but this way it's easier to notice "dictionaries" is a folder and not a file.
Comment 13 Martin Gieseking 2010-04-18 03:00:40 EDT
To be a bit more clearer, I suggest to make the %files section look like this:

%files
%defattr(-,root,root,-)
%doc COPYING ChangeLog README
%{_bindir}/%{name}
%{_datadir}/%{name}/assets/index.html
%{_datadir}/%{name}/dictionaries/
Comment 14 Susi Lehtola 2010-04-18 03:00:53 EDT
(In reply to comment #12)
> %{_datadir}/%{name}/assets/index.html creates %{_datadir}/%{name} and
> %{_datadir}/%{name}/assets, and finally puts index.html in "assets". Thus,
> there's no need for 
> %dir %{_datadir}/%{name} and
> %dir %{_datadir}/%{name}/assets.

Incorrect. See review guidelines:

"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."

If you drop the declarations %dir %{_datadir}/%{name} and %dir  {_datadir}/%{name}/assets, these will stick around after uninstall.
Comment 15 Susi Lehtola 2010-04-18 03:10:45 EDT
(In reply to comment #13)
> To be a bit more clearer, I suggest to make the %files section look like this:
> 
> %files
> %defattr(-,root,root,-)
> %doc COPYING ChangeLog README
> %{_bindir}/%{name}
> %{_datadir}/%{name}/assets/index.html
> %{_datadir}/%{name}/dictionaries/    

and it has to own these, too

%dir %{_datadir}/%{name}
%dir %{_datadir}/%{name}/assets

since otherwise these wouldn't be owned by anything.
Comment 16 Martin Gieseking 2010-04-18 03:20:54 EDT
(In reply to comment #15)
> and it has to own these, too
> 
> %dir %{_datadir}/%{name}
> %dir %{_datadir}/%{name}/assets
> 
> since otherwise these wouldn't be owned by anything.    

Oh, sorry for the confusion. I thought these kinds of ownership are detected automatically too. I obviously still have to dig into the details. Sorry again.
Comment 17 Michal Ambroz 2010-04-18 06:32:01 EDT
Hello guys,
yes I actually just came to the same conclusion, when removing all the %dir from the spec file.
$ sudo rpm -Uhv skipfish-1.31-0.3.b.fc12.i686.rpm skipfish-icons-1.31-0.3.b.fc12.noarch.rpm
Preparing...                ########################################### [100%]
   1:skipfish               ########################################### [ 50%]
   2:skipfish-icons         ########################################### [100%]
$ rpm -qf /usr/share/skipfish
file /usr/share/skipfish is not owned by any package
$ rpm -qf /usr/share/skipfish/dictionaries
file /usr/share/skipfish/dictionaries is not owned by any package
$ rpm -qf /usr/share/skipfish/assets
file /usr/share/skipfish/assets is not owned by any package

So ... I will revert the changes.

Michal
Comment 18 Michal Ambroz 2010-04-18 06:53:35 EDT
SPEC URL: http://rebus.fedorapeople.org/fedora/12/SPECS/skipfish.spec
SRPM URL: 
http://rebus.fedorapeople.org/fedora/12/SRPMS/skipfish-1.31-0.3.b.fc12.src.rpm

Hello .. here is the update.I have returned dir macros and set rigid version for the dependency check between icons and base package. 

Michal
Comment 19 Tomas Mraz 2010-04-20 06:11:33 EDT
I do not think it makes much sense to split out icons in an extra subpackage just for the licensing reason. Especially when the ASL-2.0 and LGPLv3 licenses are compatible. Just include both licenses properly in the License field and in the %doc. The split would make sense for example in case of library/tools with different licenses split or in similar cases where the split out files can be reasonably useful alone.
Comment 20 Michal Ambroz 2010-04-20 09:53:04 EDT
Hello Tomasi,
I was following the recommendation from the licensing guidelines about using AND in the license field:
"Fedora maintainers are highly encouraged to avoid this scenario whenever reasonably possible, by dividing files into subpackages (subpackages can each have their own License: field)."

Statement seems pretty strong and in this case it was low effort to separate subpackage. Files are not linked together into one binary.
Of course if you really do want to see this packaged under single package I will respect your opinion.

>The split would make sense for example in case of library/tools with
>different licenses split or in similar cases where the split out files can be
>reasonably useful alone.
I was rather thinking about possibility that someone can opt for installing different set of icons to use with skipfish. (not that such set would be available now)

Best regards
Michal Ambroz
Comment 21 Tomas Mraz 2010-04-20 10:03:35 EDT
I'm sorry but I really dislike this split and I have to say that I really dislike the wording of the guideline too. Artificially inflating the number of packages just for the different licensing of the files when we can not reasonably fix all the packages this way anyway does not seem like a way to improve Fedora to me.

I am not sure how much realistic scenario is it to have different set of icons for such kind of tool.
Comment 22 Michal Ambroz 2010-04-20 20:44:11 EDT
SPEC URL: http://rebus.fedorapeople.org/fedora/12/SPECS/skipfish.spec
SRPM URL: 
http://rebus.fedorapeople.org/fedora/12/SRPMS/skipfish-1.32-0.1.b.fc12.src.rpm

- package merged again
- update to version 1.32b
Comment 23 Tomas Mraz 2010-04-21 08:35:24 EDT
A few more notes:

I am not sure that including the string-inl.h as %doc is correct as the guideline says "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. If the source package does not include the text of the license(s), the packager should contact upstream and encourage them to correct this mistake." The license is not in its own file. You have it also twice in the %files.

I see you workaround some problem with FORTIFY_SOURCE, is that really needed? I do not see any warning. It would be much better to fix the problematic code if possible.
Comment 24 Michal Ambroz 2010-04-21 14:27:29 EDT
Hello Tomas,
regarding the FORIFY_SOURCE - here are the links to the skipfish bugzilla:
http://code.google.com/p/skipfish/issues/detail?id=34
http://code.google.com/p/skipfish/issues/detail?id=1


Result is that this won't be fixed in skipfish and is issue of the fortify code in gcc. Skipfish upstream distributes with makefile containing -DFORTIFY_SOURCE=0.

Issue which I have analyzed was working like that:
1) ask malloc to allocate buffer of size let say 97 bytes
2) malloc allocates 100 (according to malloc_usable_size)
3) to clean any leftovers memset(buffer, 0, 100) is called
4) this triggers coredump as FORTIFY code believes 97 bytes was requested, no more should be accessed. 

Situation gets even more complicated with realloc.

In my opinion malloc_usable_size should be "FORTIFIED" (return only number of requested and not allocated bytes) or FORTIFY_SOURCE should use malloc_usable_size as the cap and not the requested bytes.

From security point of view - if I got from kernel more space than requested it is OK to clean it all from some lef-overs which have been there before to avoid some posibility of injecting some unwanted data.


Best regards
Michal Ambroz
Comment 25 Tomas Mraz 2010-04-22 04:13:58 EDT
Sorry but I just consulted this with Jakub Jelinek and this is clearly bug in the source code of skipfish. The malloc_usable_size() does not allow you to memset over the end of the length passed to malloc(). There might be very well some internal data of the allocator. This call just tells you that if you realloc the allocated memory it will not have to move the block if the newly requested size is up to the malloc_usable_size() length.

So please
1. report this to the upstream.
2. patch the memset calls so they clear just the allocated memory.
3. change the spec so it properly uses the optflags from rpm including the FORTIFY_SOURCE=2.
Comment 26 Michal Ambroz 2010-04-22 22:48:25 EDT
Hello Thomas,
Thank you for the review and hints from Jakub Jelinek.
Here are the requested modifications to enable FORTIFY_SOURCES.

SPEC URL: http://rebus.fedorapeople.org/fedora/12/SPECS/skipfish.spec
SRPM URL: http://rebus.fedorapeople.org/fedora/12/SRPMS/skipfish-1.32-0.2.b.fc12.src.rpm

Best regards 
Michal Ambroz
Comment 27 Tomas Mraz 2010-04-23 12:30:15 EDT
I do not understand why you patched it this way. It does not make sense to me to overallocate the memory to always occupy the malloc_usable_size(). Why do not just drop the malloc_usable_size() calls altogether and memset just the allocated memory. You cannot use the overallocated memory in the callers anyway.
Comment 28 Michal Ambroz 2010-04-23 12:39:03 EDT
I patched it this way because skipfish is using the memory this way.

If you do not like this way, please could you propose some better way. Do you know about better patch which would not need to rewrite most of the realloc calls in skipfish?

Michal Ambroz
Comment 29 Tomas Mraz 2010-04-23 13:01:44 EDT
You're right, that the ck_realloc would have to be changed to have possibly 2 more parameters - curr_nmemb, add_nmemb, size - but the rewrite would be pretty straightforward. I'll make the patch later.
Comment 30 Tomas Mraz 2010-04-26 16:48:55 EDT
Created attachment 409289 [details]
Proposed patch

Here is the proposed patch that rewrites all ck_realloc calls appropriately.
Comment 31 Tomas Mraz 2010-04-26 16:50:10 EDT
I forgot to add that the patch is untested.
Comment 32 Michal Ambroz 2010-04-26 23:22:43 EDT
Hello Tomas,
thank you for the patch. 

Here is the package rebuild using this patch:
SPEC URL: http://rebus.fedorapeople.org/fedora/13/SPECS/skipfish.spec
SRPM URL:
http://rebus.fedorapeople.org/fedora/13/SRPMS/skipfish-1.32-0.3.b.fc13.src.rpm

Koji FC12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2139805
Koji FC13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2139819

Best regards
Michal Ambroz
Comment 33 Michal Ambroz 2010-04-26 23:42:45 EDT
Please Tomas, 
are you sure about the ADD_STR_DATA macro?
It is still using malloc_usable_size, is it OK? 
I have got some sucpicion, that it can still copy some data past canary.
Michal Ambroz
Comment 34 Tomas Mraz 2010-04-27 05:32:33 EDT
You're right that the ADD_STR_DATA is not correct. I'll look at this.
Comment 35 Tomas Mraz 2010-04-27 05:56:32 EDT
Created attachment 409422 [details]
Patch with fixed ADD_STR_DATA
Comment 36 Michal Ambroz 2010-04-27 12:37:09 EDT
Hello Tomas,
Something is wrong. 
If I compile the package with fortify it segfaults on:
skipfish -o a http://a

Best regards
Michal Ambroz
Comment 37 Tomas Mraz 2010-04-27 13:58:29 EDT
Created attachment 409537 [details]
Proposed patch

Heh, stupid mistake of mine.

This time I've at least trivially tested it.
Comment 38 Michal Ambroz 2010-04-28 19:00:47 EDT
Hello Tomas,
thanks for the fix of patch. It seems to work now.

SPEC URL: http://rebus.fedorapeople.org/12/SPECS/skipfish.spec
SRPM URL: http://rebus.fedorapeople.org/12/SRPMS/skipfish-1.32-0.4.b.fc12.src.rpm

Koji FC12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2144434
Koji FC13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2144431

Output from rpmlint:
$ rpmlint skipfish-1.32-0.4.b.fc12.i686.rpm skipfish-debuginfo-1.32-0.4.b.fc12.i686.rpm skipfish-1.32-0.4.b.fc12.src.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.


Best regards
Michal Ambroz
Comment 39 Tomas Mraz 2010-04-30 07:54:49 EDT
Rpmlint is silent:
rpmlint -v skipfish-1.32-0.4.b.fc12.src.rpm skipfish-1.32-0.4.b.fc12.x86_64.rpm skipfish-debuginfo-1.32-0.4.b.fc12.x86_64.rpm
skipfish.src: I: checking
skipfish.x86_64: I: checking
skipfish-debuginfo.x86_64: I: checking
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

According to my review the package conforms to the Fedora packaging guidelines.
APPROVED
Comment 40 Michal Ambroz 2010-05-01 14:02:02 EDT
New Package CVS Request
=======================
Package Name: skipfish
Short Description: Web application security scanner
Owners: rebus
Branches: F-12 F-13 EL-4 EL-5 devel
InitialCC: tmraz

Thank you
Michal Ambroz
Comment 41 Kevin Fenzi 2010-05-03 22:46:26 EDT
CVS done (by process-cvs-requests.py).
Comment 42 Fedora Update System 2010-05-08 19:55:32 EDT
skipfish-1.32-0.4.b.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/skipfish-1.32-0.4.b.fc13
Comment 43 Fedora Update System 2010-05-08 19:55:40 EDT
skipfish-1.32-0.4.b.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/skipfish-1.32-0.4.b.fc12
Comment 44 Fedora Update System 2010-05-08 19:55:45 EDT
skipfish-1.32-0.4.b.el4 has been submitted as an update for Fedora EPEL 4.
http://admin.fedoraproject.org/updates/skipfish-1.32-0.4.b.el4
Comment 45 Fedora Update System 2010-05-08 19:55:50 EDT
skipfish-1.32-0.4.b.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/skipfish-1.32-0.4.b.el5
Comment 46 Fedora Update System 2010-05-10 13:09:13 EDT
skipfish-1.32-0.4.b.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 47 Fedora Update System 2010-05-10 19:52:49 EDT
skipfish-1.32-0.4.b.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 48 Fedora Update System 2010-06-09 11:46:27 EDT
skipfish-1.32-0.4.b.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 49 Fedora Update System 2010-06-30 10:23:27 EDT
skipfish-1.32-0.4.b.el4 has been pushed to the Fedora EPEL 4 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 50 Fedora Update System 2010-08-08 11:17:14 EDT
skipfish-1.54-0.1.b.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/skipfish-1.54-0.1.b.el5
Comment 51 Fedora Update System 2010-08-08 11:17:21 EDT
skipfish-1.54-0.1.b.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/skipfish-1.54-0.1.b.fc13
Comment 52 Fedora Update System 2010-08-08 11:17:27 EDT
skipfish-1.54-0.1.b.fc14 has been submitted as an update for Fedora 14.
http://admin.fedoraproject.org/updates/skipfish-1.54-0.1.b.fc14
Comment 53 Fedora Update System 2010-08-23 21:16:45 EDT
skipfish-1.54-0.1.b.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 54 Fedora Update System 2010-08-24 17:05:33 EDT
skipfish-1.54-0.1.b.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 55 Fedora Update System 2010-08-24 19:02:05 EDT
skipfish-1.54-0.1.b.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 56 Fedora Update System 2011-01-21 22:21:35 EST
skipfish-1.84-0.1.b.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/skipfish-1.84-0.1.b.fc14
Comment 57 Fedora Update System 2011-01-21 22:31:37 EST
skipfish-1.84-0.1.b.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/skipfish-1.84-0.1.b.el5
Comment 58 Fedora Update System 2011-01-21 22:43:37 EST
skipfish-1.84-0.1.b.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/skipfish-1.84-0.1.b.el6
Comment 59 Fedora Update System 2011-01-23 15:26:35 EST
skipfish-1.84-0.1.b.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 60 Fedora Update System 2011-02-07 12:54:39 EST
skipfish-1.84-0.1.b.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 61 Fedora Update System 2011-02-07 12:55:46 EST
skipfish-1.84-0.1.b.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

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