[ZOO-Discuss] Segfaults and security issues in ZOO WPS server

[Gérald Fenoy]

Hi Soeren,
nice to hear from you, you're of great help.

I'm sorry for answering so late, I've explained to you on IRC today the bad moments I spend during the end of this week :]  

To go back to your message, in fact, I wondered when this kind of message will arrived. Indeed I know that we have to correct lot of things in the current code and I fully agree about your comments on code quality and issues with it :)

Hopefully you're here to make us all remember this.

Please, don't hesitate to make your comments the way you want as you did in the mail, I really appriciate it and I think that we have to be as critical as we have to be more constructive. 

As you noticed, this is the first ZOO Kernel release. This first release was just a proof of concept for me and help us to define some rules for Services Providers conception mainly. Now that we proven the concept works and it's helpful for services providers developers, it's time to make it stable and ready for a production use.

Nevertheless, I think that the step to make the code public have to be done this way, as I think that the opensource community wait for us to make the source code publicly available as an opensource software to use and test it. Now, it's the case.

Furthermore, in my opinion, lot of things we will need to be able to provide a stable and robust solution are here already : the principle to parse a metadata file, to dynamically load the shared object (from different programming languages) / extract the service function from this last / run the function passing parameters by reference (after converting arguments in a specific language data type), then getting back the values to output them as a ResponseDocument or in RawData format (after making the convertion in the reverse order). So for me a big work has already been done.

Moreover, devs won't have to modify their own Services Providers source code to make them running on the new ZOO Kernel version. I think that is the point : making people already able to develop their own Services Providers using this first public release and then upgrade their ZOO Kernel installation to the new one without having to recode their services.

Nevertheless, we thought about using YAML[1] rather than the ZCFG syntax we defined, so devs should have to rewrite their ZCFG when they upgrade their ZOO Kernel installation to use this new syntax. By the way, to avoid the ZCFG rewrite, we will still support ZCFG files during some time, until we get a converter from a ZCFG file to its equivalent YAML file available. This way life of Services Providers coders will be made even easier.

Now that the first release is available, it is time to make it better !

Everybody which want to help in this step is welcome,
Regards,

[1] http://yaml.org/

Le 23 avr. 2010 à 01:36, Soeren Gebbert a écrit :

> Hello folks,
> congrats to the first official release of the ZOO project and thanks
> for the great work.
> 
> I'm using the latest svn zoo kernel and services for GIS GRASS
> integration on Suse linux 11.1 32bit.
> While playing around with the ZOO WPS server i noticed some issues.
> 
> The zoo_loader.cgi produces segfaults in the following cases:
> * Parsing of the zcfg files seems to be case sensitive. My zcfg files
> are currently hand-crafted, i.e: when i write by accident LIteralData
> instead of LiteralData the loader crashes. I would like to suggest to
> ignore the case in key word parsing, or to print an error
> message/generating a response document in case a keyword is missing.
> * When the metapath variable is missing in a request the loader crashes i.e.:
>  http://127.0.0.1/cgi-bin/zoo_loader.cgi?ServiceProvider=&Service=WPS&Request=GetCapabilities&Version=1.0.0
>  instead of
>  http://127.0.0.1/cgi-bin/zoo_loader.cgi?ServiceProvider=&metapath=&Service=WPS&Request=GetCapabilities&Version=1.0.0
>  Is the ServiceProvider and the metapath parameter mandatory?
> 
> Security and stability issues in the C source code:
> * I noticed that many times strcmp is used for comparing strings. I
> would like to suggest to use strncascmp instead for non case sensitive
> parsing and for security reason.
> * Memory allocation is many times not checked for success. To find
> segfaults resulting from missing allocation checks is always a pain in
> cgi scripts.
> * Please avoid the use of sprintf. This C-function is the main reason
> for critical security leaks and should IMHO not be used at all in
> server applications. Segfaults resulting from sprintf created by
> buffer over- or underflows are hard to find too. Always prefer
> snprintf (_snprintf on windows) when creating strings.
> * Why using the static function declaration in service.h and the
> external declaration (which is the default) for local functions (i.e:
> in service_internal.c, ...)?
> * Some functions getting quite large and using redundant code
> (service_internal.c)
> 
> I would like to suggest the use of doxygen comment style for all
> global and local functions for better documentation. The code will be
> much better maintainable and external folks will be able to fix bugs
> or enhance the core much easier.
> 
> Establishing code reviews and refactoring sessions will largely
> improve the code quality. :)
> 
> I know this is the first official release and its just the beginning.
> So please forgive me if i am too critical or offensive. IMHO the ZOO
> WPS server will largely benefit from a secure and stable
> implementation. Lets make th ZOO WPS server fit for production
> environment. :)
> 
> Best regards
> Soeren
> _______________________________________________
> Zoo-discuss mailing list
> Zoo-discuss at gisws.media.osaka-cu.ac.jp
> http://gisws.media.osaka-cu.ac.jp/mailman/listinfo/zoo-discuss

Gérald Fenoy
gerald.fenoy at geolabs.fr


GEOLABS
Siège social :
Futur Building I
1280, avenue des Platanes
34970 Lattes
Tél. fixe : 04 67 53 67 37
Tél. portable : 06 70 08 25 39

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/zoo-discuss/attachments/20100425/34bfc1cb/attachment.html>