Explanation of the two recent patches for NDOUtils

Yesterday one of our developpers wrote two small patches for NDOUtils. They were both related to the recent SSL patch from Jean Gabes. As they were rather trivial, Duncan Ferguson applied them and committed them almost instantly (thank you Duncs !).
Let’s see what was wrong with the original code.

The first issue was encountered while making some performance tests with ndo2db. Merethis is currently developping a new event broker module called CentreonBroker, and wanted to make some benchmarks to see how many times CentreonBroker was faster than ndo2db. For this purpose we used a plain text file dumped from ndomod which was netcat’d to ndo2db / CentreonBroker listening port. Obviously there was no need for any SSL feature, but despite it was deactivated (use_ssl=0 in configuration file), the log line INFO: SSL/TLS initialized. All network traffic will be encrypted. kept appearing. After a quick investigation we found the offending code in ndo2db.c :

#ifdef HAVE_SSL
       /* initialize SSL */
       if(use_ssl==NDO_TRUE){
               ...
               /* SSL initialization code */
               ...
               syslog(LOG_INFO,"INFO: SSL/TLS initialized. All network traffic will be encrypted.");
               }
       else{
               syslog(LOG_INFO,"INFO: SSL/TLS NOT initialized. Network encryption DISABLED.");
               }
       /*Fin Hack SSL*/
#endif

       /* initialize variables */
       ndo2db_initialize_variables();

       /* process config file */
       if(ndo2db_process_config_file(ndo2db_config_file)!=NDO_OK){
               printf("Error processing config file '%s'.\n",ndo2db_config_file);
               exit(1);
               }

See the problem ? SSL is initialized according to the use_ssl variable status. However this var is set to its real value only after config file has been processed (ndo2db_process_config_file()call). This issue itself does not cause any kind of real damage (only leaked memory because of the SSL initialization that might not be cleaned and a wrong log message). Patch was really trivial, just moved the ndo2db_process_config_file() call before SSL initialization.

The second issue occured when trying to compile NDOUtils without SSL support (./configure –disable-ssl). When make’ing, make complains about missing variables, undefined function, moon’s position, and life in general … and compilation fails. This happened because some SSL-specific code in io.c wasn’t properly protected with the HAVE_SSL macro :

 

               /* try to write everything we have left */
               if (use_ssl == NDO_FALSE) {
                       result=write(fd, buf+tbytes, buflen-tbytes);
               }
               else{
                       result=SSL_write(ssl, buf+tbytes, buflen-tbytes);
               }

was changed to

               /* try to write everything we have left */
#ifdef HAVE_SSL
               if (use_ssl == NDO_TRUE)
                       result=SSL_write(ssl, buf+tbytes, buflen-tbytes);
               else
#endif
                       result=write(fd, buf+tbytes, buflen-tbytes);

And by fixing this compilation problem this way, another problem was solved within ndomod. In ndomod, SSL is initialized when opening a new TCP data sink (in io.c) :

       /* we are sending output to a TCP socket */
       else if(type==NDO_SINK_TCPSOCKET){

               if(name==NULL)
                       return NDO_ERROR;

#ifdef HAVE_SSL
               if(use_ssl==NDO_TRUE){
                       ...
                       /* Global SSL initialization code */
                       ...
       }
#endif

               ...
               /* Generic data sink initialization */
               ...

               #ifdef HAVE_SSL
               if(use_ssl==NDO_TRUE){
                       if((ssl=SSL_new(ctx))!=NULL){
                               SSL_CTX_set_cipher_list(ctx,"ADH");
                               SSL_set_fd(ssl,newfd);
                               if((rc=SSL_connect(ssl))!=1){
                                       printf("Error - Could not complete SSL handshake.\n");
                                       SSL_CTX_free(ctx);
                                       close(newfd);
                                       return NDO_ERROR;
                               }
                       } else {
                               printf("CHECK_NRPE: Error - Could not create SSL connection structure.\n");
                               return NDO_ERROR;
                       }
               }
#endif
               }

So SSL-specific data (and in particular the ssl variable which is a pointer to a SSL structure) are only initialized when use_ssl is equal to NDO_TRUE. In io.c, when writing data (see above), it is assumed that if use_ssl is different of NDO_FALSEssl has been initialized (that is use_ssl is equivalent to NDO_TRUE). But is this a valid assumption ?

NB : use_ssl is an int, NDO_TRUE is a macro of value 1 and NDO_FALSE is a macro of value 0

Code in ndomod.c gives us the answer :

       else if(!strcmp(var,"use_ssl"))
               use_ssl = strtoul(val, NULL, 0);

So use_ssl is equal to the integer value specified in the configuration file. That is if you specifyuse_ssl=42 in your configuration file, ndomod will almost certainly makes Nagios crash because :

  1. SSL won’t be initialized (code is executed if (use_ssl==NDO_TRUE) )
  2. so the ssl pointer will hold to a potentially invalid memory address
  3. when writing data to the socket, we will use SSL_write() because the standard write() is used only if (use_ssl==NDO_FALSE)
  4. SSL_write will dereference an unknown pointer resulting in undefined behavior (usually Bus Error or Segmentation Fault)

IMHO this shouldn’t happen. Anybody have the right to be dumb and specify a meaningless value in the configuration file but Nagios shouldn’t crash because of it.

I would suggest to check use_ssl‘s value this way :

if (use_ssl)
       ; /* SSL is activated */
else
       ; /* SSL is not activated */

or fetch the value from the configuration file this way :

else if(!strcmp(var,"use_ssl"))
       use_ssl=(strtoul(val,NULL,0) ? NDO_TRUE : NDO_FALSE);

The latter requires fewer changes but I like the first better. The patch I provided comparesuse_ssl to NDO_TRUE because that’s how it’s most frequently made in NDOUtils, but I dislike it.

BTW, there’s one last if (use_ssl==NDO_FALSE) in ndo2db.c, meaning that potentially, ndo2db could crash with an invalid value for use_ssl in its conf file.

Leave a Reply