Bug#285554: [Pkg-nagios-devel] Bug#285554: acknowledged by developer (check_smtp update)
hey, just fyi: i'll be uploading a new version of nagios-plugins (with many other bugfixes) which will claim to fix this problem, as i think it does. if it doesn't, we can reopen it again and keep on hacking at it. sean signature.asc Description: Digital signature
Bug#285554: [Pkg-nagios-devel] Bug#285554: acknowledged by developer (check_smtp update)
On Mon, Oct 10, 2005 at 02:13:27AM +0200, Jeroen van Wolffelaar wrote: i'm still not quite happy with that last one, as gethostbyname isn't defined by posix/sus/iso, but it's being used elsewhere in the code base already, so oh well. As long as Debian supporst it across all architectures... But yeah, something for upstream to generalize if possible. yeah. i guess i tend to keep that in mind, as i'm also on the upstream development team :) but since they were using gethostname already in the code... whatever. Anyway, it doesn't work yet over here completely. I get a segmentation fault in the call to SSL_CTX_free(ctx). sounds like some error checking isn't catching the ssl connection not being properly established. Feel free to use mail.wolffelaar.nl as test SMTP server as much as you like, by the way. It's default sarge exim4 with TLS enabled, otherwise nothing really special that should be relevant for this check. cool, thanks. i suspect that it actually is relevant though, as i see something later on about gnutls, and istr reading another bug report about openssl extensions causing problems in non-openssl environments. don't know it well enough to know whether or not that could be the case here though, i'll look into it. #0 0x40072f77 in SSL_SESSION_hash () from /usr/lib/i686/cmov/libssl.so.0.9.8 #1 0x401272c7 in lh_delete () from /usr/lib/i686/cmov/libcrypto.so.0.9.8 #2 0x400773d8 in SSL_CTX_get_timeout () from /usr/lib/i686/cmov/libssl.so.0.9.8 #3 0x40126f34 in lh_doall_arg () from /usr/lib/i686/cmov/libcrypto.so.0.9.8 #4 0x4007750e in SSL_CTX_flush_sessions () from /usr/lib/i686/cmov/libssl.so.0.9.8 #5 0x400730cc in SSL_CTX_free () from /usr/lib/i686/cmov/libssl.so.0.9.8 #6 0x08049777 in my_close () at check_smtp.c:759 #7 0x08049813 in connect_STARTTLS () at check_smtp.c:655 #8 0x0804a7ae in main (argc=3, argv=0xbaf4) at check_smtp.c:236 probably freeing something that was never successfully allocated. I also see no code doing a QUIT over the TLS'd connection, or for that matter, anything. I don't think that'd really be needed though, it's definitely not in the scope of the original check_smtp... Though I guess it's actually going to cause a log entry (exim tends to log connection closed unexpectedly if there is no QUIT). Currently my logs for the SEGV'ing check_smtp say: it will send a QUIT if told to do starttls and the server doesn't support it, or at the end of the SMTP conversation (independant of whether the connection is SSL-enabled). iirc SMTP requires the client send a quit before closing the connection. the one place it doesn't do a QUIT is if there's an error with connect_STARTTLS, but that's because there's been some application error and the connection/encryption is in a rather undefined state. 2005-10-10 02:11:13 TLS error on connection from 22pc220.sshunet.nl (bla.wolffelaar.nl) [145.97.220.22] (gnutls_handshake): Could not negotiate a supported cipher suite. probably an openssl/gnutls thing. i don't know a lot about openssl, but i'm guessing some SSL_get/set_cipher_list might resolve that. + localhostname = malloc (HOST_MAX_BYTES); + gethostname(localhostname, HOST_MAX_BYTES); Hm, if gethostname fails, you end up with a bogus localhostname, but you don't detect that and continue anyway. Fwiw, this was already present in the old code too. Now fortunately nobody but root can set the hostname... Or it'd be a security hole. i'll put some more error checking in there. out of curiosity though, how can calling gethostname set the hostname? check_smtp.c:134: warning: unused variable 'ehlo_resp' check_smtp.c:131: warning: unused variable 'amt_read' my bad, was from earlier hacking attempts sean -- signature.asc Description: Digital signature
Bug#285554: [Pkg-nagios-devel] Bug#285554: acknowledged by developer (check_smtp update)
hi jeroen,
attached is a new patch.
On Mon, Oct 10, 2005 at 03:13:18AM -0400, sean finney wrote:
Anyway, it doesn't work yet over here completely. I get a segmentation fault
in the call to SSL_CTX_free(ctx).
sounds like some error checking isn't catching the ssl connection not
being properly established.
there was a double SSL_something_free in there if it couldn't establish
an ssl connection, and other SSL_*_free calls that weren't necessarily
properly checking to see if they should even be called. the latest
patch fixes this.
Feel free to use mail.wolffelaar.nl as test SMTP server as much as you like,
by the way. It's default sarge exim4 with TLS enabled, otherwise nothing
really special that should be relevant for this check.
cool, thanks. i suspect that it actually is relevant though, as i see
something later on about gnutls, and istr reading another bug report
about openssl extensions causing problems in non-openssl environments.
don't know it well enough to know whether or not that could be the
case here though, i'll look into it.
r'ing tfm on SSL_CTX_new, i found the following change seems to have
fixed the gnutls issue:
- meth = SSLv2_client_method ();
+ meth = SSLv23_client_method ();
i'll put some more error checking in there. out of curiosity though,
how can calling gethostname set the hostname?
now added.
sean
--
#! /bin/sh /usr/share/dpatch/dpatch-run
## 16_check_smtp_protocolfix.dpatch by [EMAIL PROTECTED]
##
## All lines beginning with `## DP:' are a description of the patch.
## DP: No description.
@DPATCH@
diff -urNad nagios-plugins~/plugins/check_smtp.c
nagios-plugins/plugins/check_smtp.c
--- nagios-plugins~/plugins/check_smtp.c2005-10-10 09:52:49.0
+0200
+++ nagios-plugins/plugins/check_smtp.c 2005-10-10 09:54:53.0 +0200
@@ -59,10 +59,17 @@
enum {
SMTP_PORT = 25
};
-const char *SMTP_EXPECT = 220;
-const char *SMTP_HELO = HELO ;
-const char *SMTP_QUIT = QUIT\r\n;
-const char *SMTP_STARTTLS = STARTTLS\r\n;
+#define SMTP_EXPECT 220
+#define SMTP_HELO HELO
+#define SMTP_EHLO EHLO
+#define SMTP_QUIT QUIT\r\n
+#define SMTP_STARTTLS STARTTLS\r\n
+
+#ifndef HOST_MAX_BYTES
+#define HOST_MAX_BYTES 255
+#endif
+
+#define EHLO_SUPPORTS_STARTTLS 1
int process_arguments (int, char **);
int validate_arguments (void);
@@ -101,6 +108,9 @@
int check_critical_time = FALSE;
int verbose = 0;
int use_ssl = FALSE;
+short use_ehlo = FALSE;
+short ssl_established = TRUE;
+char *localhostname = NULL;
int sd;
char buffer[MAX_INPUT_BUFFER];
enum {
@@ -112,7 +122,7 @@
int
main (int argc, char **argv)
{
-
+ short supports_tls=FALSE;
int n = 0;
double elapsed_time;
long microsec;
@@ -120,6 +130,7 @@
char *cmd_str = NULL;
char *helocmd = NULL;
struct timeval tv;
+ struct hostent *hp;
setlocale (LC_ALL, );
bindtextdomain (PACKAGE, LOCALEDIR);
@@ -129,12 +140,26 @@
usage4 (_(Could not parse arguments));
/* initialize the HELO command with the localhostname */
-#ifndef HOST_MAX_BYTES
-#define HOST_MAX_BYTES 255
-#endif
- helocmd = malloc (HOST_MAX_BYTES);
- gethostname(helocmd, HOST_MAX_BYTES);
- asprintf (helocmd, %s%s%s, SMTP_HELO, helocmd, \r\n);
+ if(! localhostname){
+ localhostname = malloc (HOST_MAX_BYTES);
+ if(!localhostname){
+ printf(_(malloc() failed!\n));
+ return STATE_CRITICAL;
+ }
+ if(gethostname(localhostname, HOST_MAX_BYTES)){
+ printf(_(gethostname() failed!\n));
+ return STATE_CRITICAL;
+ }
+ hp = gethostbyname(localhostname);
+ if(!hp) helocmd = localhostname;
+ else helocmd = hp-h_name;
+ } else {
+ helocmd = localhostname;
+ }
+ if(use_ehlo)
+ asprintf (helocmd, %s%s%s, SMTP_EHLO, helocmd, \r\n);
+ else
+ asprintf (helocmd, %s%s%s, SMTP_HELO, helocmd, \r\n);
/* initialize the MAIL command with optional FROM command */
asprintf (cmd_str, %sFROM: %s%s, mail_command, from_arg, \r\n);
@@ -178,11 +203,26 @@
}
}
- /* send the HELO command */
+ /* send the HELO/EHLO command */
send(sd, helocmd, strlen(helocmd), 0);
/* allow for response to helo command to reach us */
- read (sd, buffer, MAXBUF - 1);
+ if(read (sd, buffer, MAXBUF - 1) 0){
+ printf (_(recv() failed\n));
+ return STATE_WARNING;
+ } else if(use_ehlo){
+ buffer[MAXBUF-1]='\0';
+ if(strstr(buffer, 250 STARTTLS) != NULL ||
+ strstr(buffer, 250-STARTTLS) != NULL){
+
Bug#285554: [Pkg-nagios-devel] Bug#285554: acknowledged by developer (check_smtp update)
On Sun, Oct 09, 2005 at 06:15:18PM +0200, Jeroen van Wolffelaar wrote: It sends a HELO, yes, but not an EHLO, which is still wrong when you want to do TLS. 220 Personal mailserver of Jeroen van Wolffelaar [EMAIL PROTECTED], using Debian Exim HELO bla 250 mordor.wolffelaar.nl Hello 22pc220.sshunet.nl [145.97.220.22] STARTTLS 503 STARTTLS command used when not advertised [EMAIL PROTECTED] 421 mordor.wolffelaar.nl lost input connection Note that nagios starts TLS even though it's being refused by the SMTP server. okay, so then then that's two problems, as even though the starttls was refused to the first error, it blindly kept on going regardless of the 5xx error. It also doesn't send a FQDN as HELO/EHLO parameter, which is technically also an SMTP protocol failure. the hostname -f FQDN would do, for example, and/or making is configureable. i don't know of a generalizable-but-portable way to get the hostname+fqdn within C, but perhaps some ifdefs could be used to use gethostname/getdomainname if available for a default value, and provide a cmdline option to override that. how does that sound? You are right though that it behaves a little bit better, it's just not quite there yet. Something that, to be honest, should have been obvious reading the buglog peeking at the code(-changes), so I'm wondering why you closed the bug, but oh well. In the even I suddenly get magically enough time left over, I'll take a stab at the code myself... But honesty requires me to admit that's quite unlikely to happen anywhere soon. yeah, i suppose a more critical eye should have caught that, but given the large number of bugs i'm having to address w/nagios-plugins, i'm doing a bit of skimming here and there, as has been made evident. anyway, don't worry about spending time on this one, i'll hack a fix together. however, if you could address the issues in the ntp-related bugs (verifying the old patch is still valid, and any additional patching/removal of code you'd recommend for the ntpdate stuff), i'd be very appreciative. sean -- signature.asc Description: Digital signature
Bug#285554: [Pkg-nagios-devel] Bug#285554: acknowledged by developer (check_smtp update)
On Sun, Oct 09, 2005 at 04:40:37PM -0400, sean finney wrote:
On Sun, Oct 09, 2005 at 06:15:18PM +0200, Jeroen van Wolffelaar wrote:
It sends a HELO, yes, but not an EHLO, which is still wrong when you want to
do TLS.
220 Personal mailserver of Jeroen van Wolffelaar [EMAIL PROTECTED],
using Debian Exim
HELO bla
250 mordor.wolffelaar.nl Hello 22pc220.sshunet.nl [145.97.220.22]
STARTTLS
503 STARTTLS command used when not advertised
[EMAIL PROTECTED]
421 mordor.wolffelaar.nl lost input connection
Note that nagios starts TLS even though it's being refused by the SMTP
server.
okay, so then then that's two problems, as even though the starttls
was refused to the first error, it blindly kept on going regardless of
the 5xx error.
Yup, and the EHLO thingy being the primary one -- if all things go right, the
second one doesn't matter (but yeah, should still be fixed -- and result in a
nagios check failure).
It also doesn't send a FQDN as HELO/EHLO parameter, which is technically
also
an SMTP protocol failure. the hostname -f FQDN would do, for example, and/or
making is configureable.
i don't know of a generalizable-but-portable way to get the
hostname+fqdn within C, but perhaps some ifdefs could be used to
use gethostname/getdomainname if available for a default value,
and provide a cmdline option to override that. how does that sound?
I've no idea whether that'd work... on my system, I get '(none)' as
getdomainname() in any case.
what /bin/hostname does is basicly gethostbyname(hostname())-h_name. Without
any error checking, and prossibly dangerous C code:
#include netdb.h
main()
{
char* hostname;
struct hostent *hp;
hostname = (char*)malloc(250);
gethostname(hostname, 250);
hp = gethostbyname(hostname);
printf(%s\n, hp-h_name);
}
You are right though that it behaves a little bit better, it's just not
quite
there yet. Something that, to be honest, should have been obvious reading
the
buglog peeking at the code(-changes), so I'm wondering why you closed the
bug, but oh well. In the even I suddenly get magically enough time left
over,
I'll take a stab at the code myself... But honesty requires me to admit
that's
quite unlikely to happen anywhere soon.
yeah, i suppose a more critical eye should have caught that, but given
the large number of bugs i'm having to address w/nagios-plugins,
i'm doing a bit of skimming here and there, as has been made evident.
Yeah, I noticed :).
anyway, don't worry about spending time on this one, i'll hack a fix
together. however, if you could address the issues in the ntp-related
bugs (verifying the old patch is still valid, and any additional
patching/removal of code you'd recommend for the ntpdate stuff), i'd be
very appreciative.
I'll reply to this while cc'ing the relevant bug instead of this one.
Thanks,
--Jeroen
--
Jeroen van Wolffelaar
[EMAIL PROTECTED] (also for Jabber MSN; ICQ: 33944357)
http://Jeroen.A-Eskwadraat.nl
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#285554: [Pkg-nagios-devel] Bug#285554: acknowledged by developer (check_smtp update)
tags 285554 patch
thanks
On Sun, Oct 09, 2005 at 11:21:01PM +0200, Jeroen van Wolffelaar wrote:
okay, so then then that's two problems, as even though the starttls
was refused to the first error, it blindly kept on going regardless of
the 5xx error.
Yup, and the EHLO thingy being the primary one -- if all things go right, the
second one doesn't matter (but yeah, should still be fixed -- and result in a
nagios check failure).
okay, i've attached a patch which should work around this. i've
verified it on my end, but i'd appreciate if you could try it out as
well and point out any issues it may have. it does the following:
- send ehlo if starttls is required
- exit with STATE_WARNING if STARTTLS isn't listed as a server capability
- send QUIT commands in all premature return points.
- provide a -F/--fqdn option to pass to helo/ehlo
- if not specified, use a gethostbyname(hostname) method similar to what you
suggested for the helo/ehlo.
i'm still not quite happy with that last one, as gethostbyname isn't
defined by posix/sus/iso, but it's being used elsewhere in the
code base already, so oh well.
sean
--
#! /bin/sh /usr/share/dpatch/dpatch-run
## 16_check_smtp_protocolfix.dpatch by [EMAIL PROTECTED]
##
## All lines beginning with `## DP:' are a description of the patch.
## DP: No description.
@DPATCH@
diff -urNad nagios-plugins~/plugins/check_smtp.c
nagios-plugins/plugins/check_smtp.c
--- nagios-plugins~/plugins/check_smtp.c2005-10-10 00:55:39.0
+0200
+++ nagios-plugins/plugins/check_smtp.c 2005-10-10 01:05:31.0 +0200
@@ -59,10 +59,17 @@
enum {
SMTP_PORT = 25
};
-const char *SMTP_EXPECT = 220;
-const char *SMTP_HELO = HELO ;
-const char *SMTP_QUIT = QUIT\r\n;
-const char *SMTP_STARTTLS = STARTTLS\r\n;
+#define SMTP_EXPECT 220
+#define SMTP_HELO HELO
+#define SMTP_EHLO EHLO
+#define SMTP_QUIT QUIT\r\n
+#define SMTP_STARTTLS STARTTLS\r\n
+
+#ifndef HOST_MAX_BYTES
+#define HOST_MAX_BYTES 255
+#endif
+
+#define EHLO_SUPPORTS_STARTTLS 1
int process_arguments (int, char **);
int validate_arguments (void);
@@ -101,6 +108,8 @@
int check_critical_time = FALSE;
int verbose = 0;
int use_ssl = FALSE;
+short use_ehlo = FALSE;
+char *localhostname = NULL;
int sd;
char buffer[MAX_INPUT_BUFFER];
enum {
@@ -112,14 +121,16 @@
int
main (int argc, char **argv)
{
-
+ short supports_tls=FALSE;
int n = 0;
double elapsed_time;
long microsec;
- int result = STATE_UNKNOWN;
+ int amt_read=0, result = STATE_UNKNOWN;
char *cmd_str = NULL;
char *helocmd = NULL;
+ char *ehlo_resp = NULL;
struct timeval tv;
+ struct hostent *hp;
setlocale (LC_ALL, );
bindtextdomain (PACKAGE, LOCALEDIR);
@@ -129,12 +140,19 @@
usage4 (_(Could not parse arguments));
/* initialize the HELO command with the localhostname */
-#ifndef HOST_MAX_BYTES
-#define HOST_MAX_BYTES 255
-#endif
- helocmd = malloc (HOST_MAX_BYTES);
- gethostname(helocmd, HOST_MAX_BYTES);
- asprintf (helocmd, %s%s%s, SMTP_HELO, helocmd, \r\n);
+ if(! localhostname){
+ localhostname = malloc (HOST_MAX_BYTES);
+ gethostname(localhostname, HOST_MAX_BYTES);
+ hp = gethostbyname(localhostname);
+ if(!hp) helocmd = localhostname;
+ else helocmd = hp-h_name;
+ } else {
+ helocmd = localhostname;
+ }
+ if(use_ehlo)
+ asprintf (helocmd, %s%s%s, SMTP_EHLO, helocmd, \r\n);
+ else
+ asprintf (helocmd, %s%s%s, SMTP_HELO, helocmd, \r\n);
/* initialize the MAIL command with optional FROM command */
asprintf (cmd_str, %sFROM: %s%s, mail_command, from_arg, \r\n);
@@ -178,11 +196,26 @@
}
}
- /* send the HELO command */
+ /* send the HELO/EHLO command */
send(sd, helocmd, strlen(helocmd), 0);
/* allow for response to helo command to reach us */
- read (sd, buffer, MAXBUF - 1);
+ if(read (sd, buffer, MAXBUF - 1) 0){
+ printf (_(recv() failed\n));
+ return STATE_WARNING;
+ } else if(use_ehlo){
+ buffer[MAXBUF-1]='\0';
+ if(strstr(buffer, 250 STARTTLS) != NULL ||
+ strstr(buffer, 250-STARTTLS) != NULL){
+ supports_tls=TRUE;
+ }
+ }
+
+ if(use_ssl ! supports_tls){
+ printf(_(WARNING - TLS not supported by server\n));
+ send (sd, SMTP_QUIT, strlen (SMTP_QUIT), 0);
+ return STATE_WARNING;
+ }
#ifdef HAVE_SSL
if(use_ssl) {
@@ -192,6 +225,7 @@
recv(sd,buffer, MAX_INPUT_BUFFER-1, 0);
Bug#285554: [Pkg-nagios-devel] Bug#285554: acknowledged by developer (check_smtp update)
On Sun, Oct 09, 2005 at 07:14:37PM -0400, sean finney wrote: On Sun, Oct 09, 2005 at 11:21:01PM +0200, Jeroen van Wolffelaar wrote: okay, so then then that's two problems, as even though the starttls was refused to the first error, it blindly kept on going regardless of the 5xx error. Yup, and the EHLO thingy being the primary one -- if all things go right, the second one doesn't matter (but yeah, should still be fixed -- and result in a nagios check failure). okay, i've attached a patch which should work around this. i've verified it on my end, but i'd appreciate if you could try it out as well and point out any issues it may have. it does the following: - send ehlo if starttls is required check - exit with STATE_WARNING if STARTTLS isn't listed as a server capability couldn't set, smtp firewalled away, and no non-TLS capable smtp hosts in my net - send QUIT commands in all premature return points. check - provide a -F/--fqdn option to pass to helo/ehlo check - if not specified, use a gethostbyname(hostname) method similar to what you suggested for the helo/ehlo. check Cool. See some code comments below in the patch. i'm still not quite happy with that last one, as gethostbyname isn't defined by posix/sus/iso, but it's being used elsewhere in the code base already, so oh well. As long as Debian supporst it across all architectures... But yeah, something for upstream to generalize if possible. Anyway, it doesn't work yet over here completely. I get a segmentation fault in the call to SSL_CTX_free(ctx). Feel free to use mail.wolffelaar.nl as test SMTP server as much as you like, by the way. It's default sarge exim4 with TLS enabled, otherwise nothing really special that should be relevant for this check. Backtrace: #0 0x40072f77 in SSL_SESSION_hash () from /usr/lib/i686/cmov/libssl.so.0.9.8 #1 0x401272c7 in lh_delete () from /usr/lib/i686/cmov/libcrypto.so.0.9.8 #2 0x400773d8 in SSL_CTX_get_timeout () from /usr/lib/i686/cmov/libssl.so.0.9.8 #3 0x40126f34 in lh_doall_arg () from /usr/lib/i686/cmov/libcrypto.so.0.9.8 #4 0x4007750e in SSL_CTX_flush_sessions () from /usr/lib/i686/cmov/libssl.so.0.9.8 #5 0x400730cc in SSL_CTX_free () from /usr/lib/i686/cmov/libssl.so.0.9.8 #6 0x08049777 in my_close () at check_smtp.c:759 #7 0x08049813 in connect_STARTTLS () at check_smtp.c:655 #8 0x0804a7ae in main (argc=3, argv=0xbaf4) at check_smtp.c:236 I also see no code doing a QUIT over the TLS'd connection, or for that matter, anything. I don't think that'd really be needed though, it's definitely not in the scope of the original check_smtp... Though I guess it's actually going to cause a log entry (exim tends to log connection closed unexpectedly if there is no QUIT). Currently my logs for the SEGV'ing check_smtp say: 2005-10-10 02:11:13 TLS error on connection from 22pc220.sshunet.nl (bla.wolffelaar.nl) [145.97.220.22] (gnutls_handshake): Could not negotiate a supported cipher suite. (...) + localhostname = malloc (HOST_MAX_BYTES); + gethostname(localhostname, HOST_MAX_BYTES); Hm, if gethostname fails, you end up with a bogus localhostname, but you don't detect that and continue anyway. Fwiw, this was already present in the old code too. Now fortunately nobody but root can set the hostname... Or it'd be a security hole. I also get: check_smtp.c:134: warning: unused variable 'ehlo_resp' check_smtp.c:131: warning: unused variable 'amt_read' + hp = gethostbyname(localhostname); --Jeroen -- Jeroen van Wolffelaar [EMAIL PROTECTED] (also for Jabber MSN; ICQ: 33944357) http://Jeroen.A-Eskwadraat.nl -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
