Re: [evolution-patches] [Fwd: Use of invalid HELO.]

[Jeffrey Stedfast <fejj novell com>]

your patch will crash if name is NULL, also:

static gboolean smtp_helo_is_valid(char *name)

should be:

static gboolean
hostname_is_valid (const char *name)


I'd also prefer the following:

if (camel_getnameinfo(transport->localaddr, transport->localaddrlen, &name, NULL, NI_NAMEREQD, NULL) == 0) {
	if (!hostname_is_valid (name)) {
		g_free (name);
		name = NULL;
	}
} else {
	g_free (name);
	name = NULL;
}

+       if (!name) {
...

Jeff

On Tue, 2006-04-04 at 19:12 +0100, David Woodhouse wrote:
> This simple and obvious patch fixes bug #336035. 
> 
> It was rejected on spurious grounds when it was originally submitted,
> but since I believe there has been a change of maintainership, perhaps I
> could commit it now?
> 
> email message attachment, "Forwarded message - Use of invalid HELO."
> > -------- Forwarded Message --------
> > From: David Woodhouse <dwmw2 infradead org>
> > To: evolution-patches lists ximian com
> > Subject: Use of invalid HELO.
> > Date: Wed, 16 Mar 2005 17:53:56 +0000
> > Mailer: Evolution 2.0.4 (2.0.4-1.dwmw2.1) 
> > 
> > Evolution happily attempts to use the domain name given by DNS even if
> > it's invalid and contains underscores, etc. OK, admittedly the reverse
> > DNS _shouldn't_ contain such things, but when on free wavelan in an
> > airport one can't be picky. So let's make Evo validate the result of
> > getnameinfo() and pretend there was no result if it would be invalid
> > according to RFC2821...
> > 
> > This problem also highlighted two other bugs -- firstly,
> > camel_getnameinfo() appears to return an empty string on failure
> > sometimes, and smtp_helo() would leak it. I've worked around this by
> > checking for that case in smtp_helo() -- but I suspect the right answer
> > may be to change camel_getnameinfo() to not do that. Someone more
> > familiar with the other callers needs to make that call though.
> > 
> > The second bug is a usability problem -- I actually had to log in to the
> > mail server and view its logs, or run Evolution with debugging enabled
> > in a terminal and observe the SMTP conversation which said:
> > 
> > 550-Mail rejected. Underscores in HELO are not permitted by RFC2821.
> > 550-Ask your local system administrator to fix your broken mail server.
> > 550-For more information see http://www.ietf.org/rfc/rfc2821.txt or
> > 550 mail postmaster infradead org
> > 
> > Instead of displaying that message, Evolution simply made something up
> > instead -- it said "Requested action not taken: mailbox unavailable".
> > I'll look into fixing that bug later.
> > 
> > I know this has been discussed before, and it was pointed out that
> > messages from my own local mail server aren't translatable -- but still,
> > surely it's better to display the message from my _local_ mailserver
> > untranslated than it is to just make something up?
> > 
> > --- evolution-data-server-1.2.0/camel/providers/smtp/camel-smtp-transport.c~	2005-01-24 09:16:34.000000000 -0700
> > +++ evolution-data-server-1.2.0/camel/providers/smtp/camel-smtp-transport.c	2005-03-16 09:23:30.000000000 -0700
> > @@ -875,6 +875,39 @@ smtp_set_exception (CamelSmtpTransport *
> >  	}
> >  }
> >  
> > +static gboolean smtp_helo_is_valid(char *name)
> > +{
> > +	enum { ALNUM, DASH, DOT } state = DOT;
> > +	int dotseen = 0;
> > +
> > +	while (*name) {
> > +		switch(state) {
> > +		case ALNUM:
> > +			if (*name == '-') {
> > +				state = DASH;
> > +				break;
> > +			} else if (*name == '.') {
> > +				dotseen = 1;
> > +				state = DOT;
> > +				break;
> > +			} /* else ... */
> > +		case DOT:
> > +		case DASH:
> > +			if (!isalnum(*name))
> > +				return FALSE;
> > +			state = ALNUM;
> > +			break;
> > +		}
> > +		name++;
> > +	}
> > +
> > +	/* If it didn't end with an alphanumeric character, or there were no dots, it's invalid */
> > +	if (state != ALNUM || !dotseen)
> > +		return FALSE;
> > +	else
> > +		return TRUE;
> > +}
> > +
> >  static gboolean
> >  smtp_helo (CamelSmtpTransport *transport, CamelException *ex)
> >  {
> > @@ -896,8 +929,19 @@ smtp_helo (CamelSmtpTransport *transport
> >  	
> >  	camel_operation_start_transient (NULL, _("SMTP Greeting"));
> >  
> > -	/* force name resolution first, fallback to numerical, we need to know when it falls back */
> > -	if (camel_getnameinfo(transport->localaddr, transport->localaddrlen, &name, NULL, NI_NAMEREQD, NULL) != 0) {
> > +	/* force name resolution first, but check the resulting name is valid according to RFC2821 */
> > +	if (camel_getnameinfo(transport->localaddr, transport->localaddrlen, &name, NULL, NI_NAMEREQD, NULL) == 0 &&
> > +	    !smtp_helo_is_valid(name)) {
> > +		g_free(name);
> > +		name = NULL;
> > +	}
> > +	/* camel_getnameinfo() can return an empty string on failure, bizarrely */
> > +	if (name && !*name) {
> > +		g_free(name);
> > +		name = NULL;
> > +	}
> > +	/* If the name lookup failed, or the name wasn't acceptable, then try to use a numeric HELO domain */
> > +	if (!name) {
> >  		if (camel_getnameinfo(transport->localaddr, transport->localaddrlen, &name, NULL, NI_NUMERICHOST, NULL) != 0)
> >  			name = g_strdup("localhost.localdomain");
> >  		else {
> > 
> > 
> _______________________________________________
> Evolution-patches mailing list
> Evolution-patches gnome org
> http://mail.gnome.org/mailman/listinfo/evolution-patches
-- 
Jeffrey Stedfast
Evolution Hacker - Novell, Inc.
fejj ximian com  - www.novell.com