Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.

Bug 576431

Summary: Package Review: skipfish - Web application security scanner
Product: [Fedora] Fedora Reporter: Michal Ambroz <rebus>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: 12CC: fedora-package-review, jakub, martin.gieseking, notting, pahan, rebus, susi.lehtola, tmraz
Target Milestone: ---Flags: tmraz: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
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 17:09:19 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:
Bug Depends On:    
Bug Blocks: 563471    
Attachments:
Description Flags
Proposed patch
none
Patch with fixed ADD_STR_DATA
none
Proposed patch none

Description Michal Ambroz 2010-03-24 01:58:21 UTC
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 07:52:00 UTC
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 13:36:44 UTC
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 17:50:31 UTC
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 09:16:54 UTC
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 12:24:35 UTC
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 20:13:20 UTC
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 12:30:45 UTC
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-18 02:08:54 UTC
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 06:56:21 UTC
(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 07:00:40 UTC
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 07:00:53 UTC
(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 07:10:45 UTC
(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 07:20:54 UTC
(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 10:32:01 UTC
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 10:53:35 UTC
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 10:11:33 UTC
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 13:53:04 UTC
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 14:03:35 UTC
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-21 00:44:11 UTC
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 12:35:24 UTC
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 18:27:29 UTC
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 08:13:58 UTC
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-23 02:48:25 UTC
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 16:30:15 UTC
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 16:39:03 UTC
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 17:01:44 UTC
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 20:48:55 UTC
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 20:50:10 UTC
I forgot to add that the patch is untested.

Comment 32 Michal Ambroz 2010-04-27 03:22:43 UTC
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-27 03:42:45 UTC
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 09:32:33 UTC
You're right that the ADD_STR_DATA is not correct. I'll look at this.

Comment 35 Tomas Mraz 2010-04-27 09:56:32 UTC
Created attachment 409422 [details]
Patch with fixed ADD_STR_DATA

Comment 36 Michal Ambroz 2010-04-27 16:37:09 UTC
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 17:58:29 UTC
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 23:00:47 UTC
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 11:54:49 UTC
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 18:02:02 UTC
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-04 02:46:26 UTC
CVS done (by process-cvs-requests.py).

Comment 42 Fedora Update System 2010-05-08 23:55:32 UTC
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 23:55:40 UTC
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 23:55:45 UTC
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 23:55:50 UTC
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 17:09:13 UTC
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 23:52:49 UTC
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 15:46:27 UTC
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 14:23:27 UTC
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 15:17:14 UTC
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 15:17:21 UTC
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 15:17:27 UTC
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-24 01:16:45 UTC
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 21:05:33 UTC
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 23:02:05 UTC
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-22 03:21:35 UTC
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-22 03:31:37 UTC
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-22 03:43:37 UTC
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 20:26:35 UTC
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 17:54:39 UTC
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 17:55:46 UTC
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.