Bug in qmail-smtpd.c addrparse function

[Andre Oppermann]

There is a nasty bug in qmail-smtpd.c addrparse function which
allows "MAIL FROM" commands without a ":", but fails to parse
the address and assumes a null sender (bounce) or recipient.
The bug is a logic error in the pointer juggling code doing the
line parsing. However, this is a functional bug and not exploitable
for any kind of malicous code execution nor can it crash in any way.

The patch is really simple:

--- qmail-smtpd.c Mon Jun 15 12:53:16 1998
+++ qmail-smtpd.c.new Wed Oct 1 10:48:07 2003
@@ -155,6 +155,7 @@
terminator = ' ';
arg += str_chr(arg,':');
if (*arg == ':') ++arg;
+ if (*arg == '\0') return 0;
while (*arg == ' ') ++arg;
}


We found this while testing our new sender and recipient verify
code in qmail-ldap patch.

[James Craig Burley]

Post by Andre Oppermann
There is a nasty bug in qmail-smtpd.c addrparse function which
allows "MAIL FROM" commands without a ":", but fails to parse
the address and assumes a null sender (bounce) or recipient.
The bug is a logic error in the pointer juggling code doing the
line parsing. However, this is a functional bug and not exploitable
for any kind of malicous code execution nor can it crash in any way.
--- qmail-smtpd.c Mon Jun 15 12:53:16 1998
+++ qmail-smtpd.c.new Wed Oct 1 10:48:07 2003
@@ -155,6 +155,7 @@
terminator = ' ';
arg += str_chr(arg,':');
if (*arg == ':') ++arg;
+ if (*arg == '\0') return 0;
while (*arg == ' ') ++arg;
}

I'm confused. The patch shown won't cause addrparse to "parse the
address", which you complain it already fails to do.

It appears the only difference between addrparse before and after your
patch is that, before it, "MAIL FROM\0" was treated exactly the same
as "MAIL FROM:\0", "MAIL FROM : \0", and "MAIL FROM \0", whereas,
after your patch, "MAIL FROM\0" is special-cased to return a
parsing-failure indicator instead of a null sender.

Why do you want to special-case "MAIL FROM\0" to not mean a bounce
message is being sent?

[James Craig Burley]

Post by James Craig Burley
It appears the only difference between addrparse before and after your
patch is that, before it, "MAIL FROM\0" was treated exactly the same
as "MAIL FROM:\0", "MAIL FROM : \0", and "MAIL FROM \0", whereas,
after your patch, "MAIL FROM\0" is special-cased to return a
parsing-failure indicator instead of a null sender.

Whoops, I forgot about the fact that the str_chr function returns
either the character found *or* the end of the string ('\0'), as an
index into the string.

So, instead, what you appear to have done is change the handling of
"MAIL" followed by text containing no colon (or "<") to mean an
invalid address.

Whereas, previously, "MAIL " with no subsequent ":" or "<" was treated
as a bounce.

I still wonder: what exactly are you trying to do here? How is this a
"nasty bug"?

[Andre Oppermann]

Post by James Craig Burley

Post by James Craig Burley
It appears the only difference between addrparse before and after your
patch is that, before it, "MAIL FROM\0" was treated exactly the same
as "MAIL FROM:\0", "MAIL FROM : \0", and "MAIL FROM \0", whereas,
after your patch, "MAIL FROM\0" is special-cased to return a
parsing-failure indicator instead of a null sender.

Whoops, I forgot about the fact that the str_chr function returns
either the character found *or* the end of the string ('\0'), as an
index into the string.

Yes, now you have interpreted it correctly.

Post by James Craig Burley
So, instead, what you appear to have done is change the handling of
"MAIL" followed by text containing no colon (or "<") to mean an
invalid address.
Whereas, previously, "MAIL " with no subsequent ":" or "<" was treated
as a bounce.
I still wonder: what exactly are you trying to do here? How is this a
"nasty bug"?

See my next answer to Charles.

[Andre Oppermann]

Post by James Craig Burley

Post by Andre Oppermann
There is a nasty bug in qmail-smtpd.c addrparse function which
allows "MAIL FROM" commands without a ":", but fails to parse
the address and assumes a null sender (bounce) or recipient.
The bug is a logic error in the pointer juggling code doing the
line parsing. However, this is a functional bug and not exploitable
for any kind of malicous code execution nor can it crash in any way.
--- qmail-smtpd.c Mon Jun 15 12:53:16 1998
+++ qmail-smtpd.c.new Wed Oct 1 10:48:07 2003
@@ -155,6 +155,7 @@
terminator = ' ';
arg += str_chr(arg,':');
if (*arg == ':') ++arg;
+ if (*arg == '\0') return 0;
while (*arg == ' ') ++arg;
}

I'm confused. The patch shown won't cause addrparse to "parse the
address", which you complain it already fails to do.

No, it wont parse it correctly. But it will fail correctly. Sending
"mail from" without ":" is wrong and should not be accepted anyway.

Post by James Craig Burley
It appears the only difference between addrparse before and after your
patch is that, before it, "MAIL FROM\0" was treated exactly the same
as "MAIL FROM:\0", "MAIL FROM : \0", and "MAIL FROM \0", whereas,
after your patch, "MAIL FROM\0" is special-cased to return a
parsing-failure indicator instead of a null sender.

You don't read the code correctly.

Before the patch:

"MAIL FROM ***@jcb-sc.com\0" would give "" (empty sender)
"MAIL FROM <***@jcb-sc.com>\0" would give "***@jcb-sc.com"
"MAIL FROM: ***@jcb-sc.com\0" would give "***@jcb-sc.com"
"MAIL FROM\0" would give "" (empty sender)
"MAIL FROM:\0" would give "" (empty sender)
"MAIL FROM:<>\0" would give "" (empty sender)

after the patch:

"MAIL FROM ***@jcb-sc.com\0" will give syntax error
"MAIL FROM <***@jcb-sc.com>\0" will give "***@jcb-sc.com"
"MAIL FROM: ***@jcb-sc.com\0" will give "***@jcb-sc.com"
"MAIL FROM\0" will give syntax error
"MAIL FROM:\0" will give "" (empty sender)
"MAIL FROM:<>\0" will give "" (empty sender)

Post by James Craig Burley
Why do you want to special-case "MAIL FROM\0" to not mean a bounce
message is being sent?

First because "MAIL FROM ***@jcb-sc.com" must not become a empty
sender and second because a ":" is required by RFC2821 as part of
the command.

[James Craig Burley]

Post by Andre Oppermann
No, it wont parse it correctly. But it will fail correctly. Sending
"mail from" without ":" is wrong and should not be accepted anyway.

I agree with this sentiment, especially since you say "should" and not
"must".

So qmail is standard-conforming and its behavior here does not seem to
represent any kind of compromise in terms of security, reliability,
speed, etc.

But I think having it tell the client it doesn't like a MAIL or RCPT
command without a "<" or ":", instead of interpreting it as a null
sender, represents an improvement anyway.

Nice that it apparently can be done by one line of code. I haven't
read the pertinent RFCs to see what other input from the client would
meet this criteria, that qmail doesn't reject, but since this issue
has come up before and can get discussed into the ground, might as
well have a canonical patch that addresses it.

[Jose Celestino]

Post by Andre Oppermann
There is a nasty bug in qmail-smtpd.c addrparse function which
allows "MAIL FROM" commands without a ":", but fails to parse
the address and assumes a null sender (bounce) or recipient.
The bug is a logic error in the pointer juggling code doing the
line parsing. However, this is a functional bug and not exploitable
for any kind of malicous code execution nor can it crash in any way.

http://msgs.securepoint.com/cgi-bin/get/qmail0212/117.html

[Frank Tegtmeyer]

Post by Jose Celestino
http://msgs.securepoint.com/cgi-bin/get/qmail0212/117.html

Or even:
http://msgs.securepoint.com/cgi-bin/get/qmail0110/16/1/1.html

Regards, Frank

[Charles Cazabon]

Post by Andre Oppermann
There is a nasty bug in qmail-smtpd.c addrparse function which
allows "MAIL FROM" commands without a ":", but fails to parse
the address and assumes a null sender (bounce) or recipient.

Not sure why this is "a nasty bug". It's only triggered by invalid SMTP and
doesn't pose a security risk, so it should be a non-issue. Agreed, returning
a syntax error is more correct, but as this is a "shouldn't happen anyway"
case, I'm not sure it's worth worrying about.

The only people who get bitten by this issue are the ones who try to test
their qmail-smtpd setup but don't know how to speak proper SMTP.

Charles

[Andre Oppermann]

Post by Charles Cazabon

Post by Andre Oppermann
There is a nasty bug in qmail-smtpd.c addrparse function which
allows "MAIL FROM" commands without a ":", but fails to parse
the address and assumes a null sender (bounce) or recipient.

Not sure why this is "a nasty bug". It's only triggered by invalid SMTP and
doesn't pose a security risk, so it should be a non-issue. Agreed, returning
a syntax error is more correct, but as this is a "shouldn't happen anyway"
case, I'm not sure it's worth worrying about.

I don't like it because it says "250 ok" when in fact it ain't ok
at all because it didn't parse it correctly. When I get this message
I do not assume it just made a null sender out of it. This is not
the reliable operation that is a feature of qmail.

Second RCPT TO uses the same addrparse function. The problem happens
there too. And again I don't like getting a "250 ok" when in fact my
recipient has become a null recipient.

Post by Charles Cazabon
The only people who get bitten by this issue are the ones who try to test
their qmail-smtpd setup but don't know how to speak proper SMTP.

While technically correct qmail should either parse this wrong command
the intended way *or* reject it right out. Which is what the patch does.