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

Mon Apr 26 10:01:16 PDT 2010

Hi Gérald,

2010/4/25 Gérald Fenoy <gerald.fenoy at geolabs.fr>:
> 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.

Agreed.

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

Well done, good work!
Now, lets start first with the error response conception of the WPS
server. IMHO we need a convenient logging and error response
mechanism. This mechanism should create error XML response documents
if an error occurs or in case of wrong or incomplete requests. Because
ZOO-WPS supports many languages on shared library level, a convenient
mechanism to catch segmentation faults or exit() calls in service
libraries must be implemented, to assure correct error response. This
response mechanism should be an outer hull over the WPS logic to catch
any error.
We can use the C++ try/throw/catch or the setjmp/longjmp C mechanism as basis.

I would like to discuss this deeper in the zoo-project trac wiki, but
unfortunately i am unable to add content or tickets there.

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

Thats a huge benefit of the server design.

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

IMHO using YAML is a very good choice, thanks for this suggestion
Gérald. It is standardized and there are C/C++ and python libraries
for parsing available for many platforms. I would like to suggest to
use the WPS process description standard (translated to YAML) for the
process description part in the new YAML config files. If we use this
standard as precise as possible (which should work with YAML) we can
skip to think about our own YAML structure to represent the WPS
process description content, but only about the zoo related content.
Additionally this allows us to easily transform existing WPS XML
document description into the YAML format.

Best regards
Soeren

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