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
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
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}
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
Koji build for FC12 http://koji.fedoraproject.org/koji/taskinfo?taskID=2080143 FC13 http://koji.fedoraproject.org/koji/taskinfo?taskID=2080158
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).
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
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
SPEC URL: http://rebus.webz.cz/d/skipfish.spec SRPM URL: http://rebus.webz.cz/d/skipfish-1.30-0.1.b.fc12.src.rpm Updated to version 1.30b Koji FC12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2107131
SPEC URL: http://rebus.fedorapeople.org/fedora/12/SPECS/skipfish.spec SRPM URL: http://rebus.fedorapeople.org/fedora/12/SRPMS/skipfish-1.31-0.1.b.fc12.src.rpm Updated to version 1.31b Koji FC12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2122334 Best regards Michal Ambroz
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.
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
(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.
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/
(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.
(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.
(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.
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
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
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.
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
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.
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
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.
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
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.
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
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.
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
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.
Created attachment 409289 [details] Proposed patch Here is the proposed patch that rewrites all ck_realloc calls appropriately.
I forgot to add that the patch is untested.
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
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
You're right that the ADD_STR_DATA is not correct. I'll look at this.
Created attachment 409422 [details] Patch with fixed ADD_STR_DATA
Hello Tomas, Something is wrong. If I compile the package with fortify it segfaults on: skipfish -o a http://a Best regards Michal Ambroz
Created attachment 409537 [details] Proposed patch Heh, stupid mistake of mine. This time I've at least trivially tested it.
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
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
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
CVS done (by process-cvs-requests.py).
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
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
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
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
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.
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.
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.
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.
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
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
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
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.
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.
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.
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
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
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
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.
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.
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.