Closed Bug 151279 Opened 22 years ago Closed 21 years ago

SMTP-Auth is not found on a RFC-Valid-Response

Categories

(MailNews Core :: Networking: SMTP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: emaijala+moz)

References

Details

Attachments

(2 files, 4 obsolete files)

From Bugzilla Helper: Heiko Studt
User-Agent: IE6/XP
BuildID: All     

The possibility to use SMTP-Auth (LOGIN) is only be found in Mozilla 
if there is a single line with "AUTH=LOGIN" in SMTP-Response.
That was the old "common practise" of deklaration of SMTP-AUTH.
The new one (RFC) is like "AUTH xyz" where xyz can be one or more 
different auth-methods like "CRAM-MD5" and of course "LOGIN" 
seperated by a blank.

Second bug (together with this): It will be choose the authentication-
method "PLAIN" if in the men-reading response the string " PLAIN" is 
used. (eg. "250 - This server is only for plain text")

Reproducible: Always
Steps to Reproduce:
1. Change in your SMTP-Server the SMTP-response-code to only "AUTH LOGIN" 
(blank)
2. Choose the server and look for using AUTH
3. It does not work.
4. Now choose "AUTH=LOGIN" as one line-response.
5. Now it works
6. Even "AUTH=PLAIN LOGIN" doesn't work

Expected Results:  It should search the response for AUTH and looks in the 
*same* line for LOGIN or PLAIN. But it should not test "AUTH=LOGIN" 
or only as extra extension.

I fixed this problem.
(My first C-code, but there should be no bugs in it. This code is compileable)

---mailnews/compose/src/nsSmtpProtocol.cpp
PRInt32 nsSmtpProtocol::SendEhloResponse(nsIInputStream * inputStream, 
PRUint32 length)
{
    PRInt32 status = 0;
    nsCAutoString buffer;
    nsCOMPtr<nsIURI> url = do_QueryInterface(m_runningURL);

    if (m_responseCode != 250)
    {
      /* EHLO must not be implemented by the server so fall back to the HELO 
case */

        if (m_prefTrySSL == PREF_SSL_ALWAYS)
        {
            m_nextState = SMTP_ERROR_DONE;
            m_urlErrorState = NS_ERROR_COULD_NOT_LOGIN_TO_SMTP_SERVER;
            return(NS_ERROR_COULD_NOT_LOGIN_TO_SMTP_SERVER);
        }
        buffer = "HELO ";
        buffer += GetUserDomainName();
        buffer += CRLF;

        status = SendData(url, buffer.get());

        m_nextState = SMTP_RESPONSE;
        m_nextStateAfterResponse = SMTP_SEND_HELO_RESPONSE;
        SetFlag(SMTP_PAUSE_FOR_READ);
        return (status);
    }
    else
    {
        char *ptr = NULL;

        if (PL_strcasestr(m_responseText.get(), "AUTH") != 0)    //HSR
                                                                 //Is this 
line used for AUTH-params?

        {
          ptr = PL_strcasestr(m_responseText.get(), "DSN");       //Can you 
use the Auth-method "DSN"?
          if (ptr && nsCRT::ToUpper(*(ptr-1)) != 'X')
              SetFlag(SMTP_EHLO_DSN_ENABLED);

          if (PL_strcasestr(m_responseText.get(), "PLAIN") != 0)  //Can you 
use the Auth-method "PLAIN" (textual user/pass)
                                                                  //(violenced 
by man-in-the-middle-attacks)
              SetFlag(SMTP_AUTH_PLAIN_ENABLED);

          if (PL_strcasestr(m_responseText.get(), "LOGIN") != 0) //Can you use 
the Auth-method "LOGIN"? (similar to "PLAIN")
              SetFlag(SMTP_AUTH_LOGIN_ENABLED);	/* old style */

          if (PL_strcasestr(m_responseText.get(), "STARTTLS") != 0) //Can you 
use the command "StartTLS"?
                                                                    //(Needed 
for secure SSL/TLS-connections over 'normal' servers)
              SetFlag(SMTP_EHLO_STARTTLS_ENABLED);

          if (PL_strcasestr(m_responseText.get(), "EXTERNAL") != 0) //Can you 
use the Auth-method "EXTERNAL"?
                                                                    //i.e. you 
are authenticate without need for own authentication.
                                                                    //e.g. you 
are authenticate by IP on your DialIn-ISP
              SetFlag(SMTP_AUTH_EXTERNAL_ENABLED);

        }

    }
    m_nextState = SMTP_AUTH_PROCESS_STATE;
    return status;
}
Summary: SMTP-Auth is not found on a RFC-Valid-Response → SourceFix: SMTP-Auth is not found on a RFC-Valid-Response
Target Milestone: --- → mozilla0.9.9
change qa contact ->nbaca.
Ninoschka, 
Please reassign the bugs if not in your area.
QA Contact: sheelar → nbaca
In the following code : m_responseText.get(), does the string represent /one/
line of server message or the full 250 message ?
I mean a server will give a 250 often on multiple lines as such :

250-this or that
250-and so on
250 the last line of 250 status

I wonder if all these tests for AUTH LOGIN or other do take line breaks in
consideration or of they treat the 250 as a long string as in :
250 this or that and so on the last line of 250 status

If that is the case, more effort will be required to correctly handle AUTH
capability. One has to check there is AUTH followed on the same line by some of
the protocol names we support.
AFAIR I had looked for that before and saw it was line by line interpreted, 
but I can be false.
Perhaps someone should make a debug-version and print the result here.

If I'm in mistake than we can interpret that line-by-line, we only have to 
search for CRLF and interpret that line. Then the next line and so on.


That is the only problem at all?
This bug seems to not raise any interest from any developer.
Though it is a Mozilla Mail Killer for many people.
Nowadays, more and more ISPs (at least in Europe) *require* users to use SMTP AUTH.
This plain basic bug of AUTH=LOGIN instead of AUTH LOGIN is shifting away users
from Mozilla, which should imho be considered as important a bug as anything
else which crash the software.
It has already been reported multiple times in the past, was doomed BOGUS (which
it is not). It was present before 1.0 release. Was still in 1.0. Was reported
before 1.1 beta, was present in 1.1 beta, in 1.1 release, is still there in 1.2
alpha and even in Netscape derived product version 7.
It may time to start taking into account things that people who want to work
with Mozilla and make it a success. Especially when the fix is so easy as this
one is.
Thanks for considering it, at least confirming it, but please do not leave this
one unconfirmed for another release or two !...
Hi,

Ok, this should solve all problems of the version before.

Two things to proove: 1. Is that a Memory-leck (char resp)
                      2. Should we change it so that AUTH have to stand at 
                         beginning?

BTW: I could compile it, but not confirm because I can't compile whole Mozilla-
     Code. Also I didn't confirm if anyone has changed this procedure between 
     last GET of latest source and now.


MFG

------- HTH -------


PRInt32 nsSmtpProtocol::SendEhloResponse(nsIInputStream * inputStream, 
PRUint32 length)
{
    PRInt32 status = 0;
    nsCAutoString buffer;
    nsCOMPtr<nsIURI> url = do_QueryInterface(m_runningURL);

    if (m_responseCode != 250)
    {
      /* EHLO must not be implemented by the server so fall back to the HELO 
case */

        if (m_prefTrySSL == PREF_SSL_ALWAYS)
        {
            m_nextState = SMTP_ERROR_DONE;
            m_urlErrorState = NS_ERROR_COULD_NOT_LOGIN_TO_SMTP_SERVER;
            return(NS_ERROR_COULD_NOT_LOGIN_TO_SMTP_SERVER);
        }
        buffer = "HELO ";
        buffer += GetUserDomainName();
        buffer += CRLF;

        status = SendData(url, buffer.get());

        m_nextState = SMTP_RESPONSE;
        m_nextStateAfterResponse = SMTP_SEND_HELO_RESPONSE;
        SetFlag(SMTP_PAUSE_FOR_READ);
        return (status);
    }
    else
    {
      /* HSR: Changed for new AUTH-Login */
        char *ptr = NULL;

        char *resp = strdup( m_responseText.get() );
        char *token = strtok( resp, "\n" );
        while( token != NULL )
        {
          /* Now we can search the lines securly for "AUTH" and then for AUTH-
mechanistics

             Perhaps we should change this so that AUTH have to be stand at 
the beginning
             Therefor we need to know if the Response_Code is striped out or 
only copied */

          if (PL_strcasestr(token, "AUTH") != 0)     //Is this line used for 
AUTH-params?

          {
            ptr = PL_strcasestr(token, "DSN");         //Can you use the Auth-
method "DSN"?
            if (ptr && nsCRT::ToUpper(*(ptr-1)) != 'X')
                SetFlag(SMTP_EHLO_DSN_ENABLED);

            if (PL_strcasestr(m_responseText.get(), "PLAIN") != 0)  //Can you 
use the Auth-method "PLAIN" (textual user/pass)
                                                                    //
(violenced by man-in-the-middle-attacks!)
                SetFlag(SMTP_AUTH_PLAIN_ENABLED);

            if (PL_strcasestr(m_responseText.get(), "LOGIN") != 0) //Can you 
use the Auth-method "LOGIN"? (similar to "PLAIN")
                SetFlag(SMTP_AUTH_LOGIN_ENABLED);	           /* old 
style */

            /* SASL
            if (PL_strcasestr(m_responseText.get(), "CRAM-MD5") != 0) //Can 
you use the Auth-method "CRAM-MD5"?
                SetFlag(SMTP_AUTH_CRAMMD5_ENABLED);

            if (PL_strcasestr(m_responseText.get(), "CRAM-SHA1") != 0) //Can 
you use the Auth-method "CRAM-SHA1"?
                SetFlag(SMTP_AUTH_CRAMSHA1_ENABLED);
               SASL */

            if (PL_strcasestr(m_responseText.get(), "STARTTLS") != 0) //Can 
you use the command "StartTLS"?
                                                                      //
(Needed for secure SSL/TLS-connections over 'normal' servers)
                SetFlag(SMTP_EHLO_STARTTLS_ENABLED);

            if (PL_strcasestr(m_responseText.get(), "EXTERNAL") != 0) //Can 
you use the Auth-method "EXTERNAL"?
                                                                      //i.e. 
you are authenticated without need for own authentication.
                                                                      //e.g. 
you are authenticated by IP on your DialIn-ISP
                SetFlag(SMTP_AUTH_EXTERNAL_ENABLED);

          }


          /* Get next token: */
          token = strtok( NULL, "\n" );
        }
        
        /* HSR: End-Of-Change */

    }
    m_nextState = SMTP_AUTH_PROCESS_STATE;
    return status;
}
Is it possible to get this merged into the daily builds so that users can soon
be able to send mail with Mozilla? This is costing Mozilla users, as the above
post mentions.

Cheers,
Boris
If someone tell me how to do it. AFAIR i read that someone 'oler' in the 
project has to confirm the fix and do it's including, but yet I didn't look 
for it. If you want to fix this ***-Problem then you (mozilla-community) can 
use my code. if you don't want --> anyway.
Keywords: mail4, mail6
OS: Windows XP → All
Hardware: PC → All
Target Milestone: mozilla0.9.9 → ---
Flags: blocking1.3b?
would someone please attach a cvs diff -u patch?
I'll take this one.
Assignee: mscott → ere
Status: NEW → ASSIGNED
Heiko,

thanks for the source. There were a few problems with it, so instead of just
making a patch from it I took the liberty of polishing the whole thing a bit.
Will attach a patch shortly.
Summary: SourceFix: SMTP-Auth is not found on a RFC-Valid-Response → SMTP-Auth is not found on a RFC-Valid-Response
This patch enhances the parsing of the EHLO response. I've still kept it a bit
fuzzy (as it was before) to avoid possible problems with different servers. I
also cleaned the code up a bit.
Attachment #111559 - Flags: review?(bienvenu)
The DSN-Thing is better now, but isn't it AUTH?
BTW: The comment "EHLO must not" is wrong. It "need not" is better...

This fix you attached don't work for
250-AUTH SOMETHING
250 Text only Plain
It would think 'PLAIN' is possible.

instead of
-  if (m_responseText.Find("PLAIN", PR_TRUE, authPos) >= 0)
+  if (m_responseText.Find("PLAIN", PR_TRUE, authPos) < m_responseText.Find(CRLF, PR_TRUE, authPos)

But that wouldn't work for
250-Auth is needed!
250 AUTH PLAIN

It have to parse the text into CRLF-Parts and entire of this part it have to search for AUTH and *after* that it have to search for "PLAIN" (and so on)

My code is not really good, cause I don't write in C and don't know about all those commands. :-/
No, DSN (Delivery Status Notification) is not auth. Neither is STARTTLS. I don't
think any sane server would return the sequence you described. So, I disagree
that it would be necessary to parse lines independently (keep it simple...), but
I can change it if the higher authorities find that necessary :) 250 lines are
meant to provide the client useful information. I don't think "Auth is needed"
or "Text only plain" would qualify. Bienvenu, what do you think?
At least that issue should be commented in, so that there won't be mistakes made in further development.
BTW: In this part the status for CRAM-MD5, CRAM-SHA1 and MD5-DIGEST have also to be got. Perhaps even this could be commented...
There's bug 150212 for different SMTP authentication mechanisms.
Ere, I'm not an expert on SMTP - what you've done seems OK to me, but I think
you know more about SMTP than I do. I'm cc'ing John Myers for his expert advice. 
Ok, thanks. Just for the record, as I think I wasn't too clear before, my
motivation for doing it this way was to avoid any unforeseen problems that a
stricter, and different from the old, parsing might cause.

BTW. A mean server might as well issue
250-STARTTLS is not available 
which would be break it anyway :)
No, that one is standarised like 'starttls' as signal word.
AUTH is AFAIK standarised in the way that the line have to begin with AUTH and the followed by a list of implemented features with each space in between.

The parsing can also be done like this. It should be more secur and even more understandable:

---
ptrEnd = *Str
ptrBegin = *Str
repeat 
 ptrBegin = find(CRLF + "250-AUTH", ptrEnd) + 2
 ptrEnd   = find(CRLF, ptrBegin)
 if ptrEnd=nil then ptrEnd = find(#0, ptrBegin)
 if find ("PLAIN", ptrBegin, ptrEnd) then [...]
 [...]
until ptrEnd = find(#0, ptrBegin)
---

Comment:
  find(#0, ptrBegin) is end of STRING (what's in C?)

  Perhaps we can use RegExp instead of this hard way


Only as an idea of a Delphi-Programmer... :)
I understood you already before, and yes, we could do that, but it wouldn't be
appropriate to change it for auth only, which means that we would need to change
a  whole lot of other code too. As far as I know, current parsing style has not
caused any problems, but changing it might just cause problems we haven't
thought of (eg AUTH=LOGIN). Anyway, let's hear what John Myers thinks. 
I strongly prefer strict parsers.  The way the existing code and proposed patch
handle recognizing the DSN extension is a gross kludge and an accident waiting
to happen.  Unless the code is written to recognize the token boundaries and
positional significance defined by the protocol, there is always risk that it
will incorrectly detect an extension that is not present.
Comment on attachment 111559 [details] [diff] [review]
Patch to enable better EHLO parsing

Ok, I'll rewrite it :)

Any special cases I should take care of (such as almost rfc compliant
responses)?
Attachment #111559 - Attachment is obsolete: true
Attachment #111559 - Flags: review?(bienvenu)
The only issue I know of is servers which follow the typo in the internet draft,
producing "AUTH=LOGIN" instead of "AUTH LOGIN".
Ok, here is the new, strict version. I decided to change to line-by-line
parsing because it's much cleaner and makes it easier to separate DSN from
XDSN.

Also fixed the smtp response parser so that it doesn't add extra newlines to
the response string.

What do you guys think?
It seems to be OK for me now, but do it handle *also* "auth=login"?
(Some servers are AFAIK also broken... :-/)
Yes it does.
Attachment #111990 - Flags: review?(jgmyers)
We won't hold 1.3beta for this, but Ere it would be great if you can get it in.
You have until about Tuesday or Wednesday.
Flags: blocking1.3b? → blocking1.3b-
John?
I just started looking at this today.  The day job tends to interfere, y'know.
Comment on attachment 111990 [details] [diff] [review]
Rewrite of the parser, quite strict now

The m_responsetext += "\n"; line has a tab.

I'm concerned that nsSmtpProtocol::ReadLine and nsSmtpProtocol::SmtpResponse
only look for \n , whereas the proposed patch has
nsSmtpProtocol::SendEhloResponse() look for \r\n.  It should match the other
code by looking only for \n.

I would prefer if the patch put braces around the "then" clasues of if
statements that are followed by "else if".

I wouldn't call the resulting parser "strict", but it is a sufficiently
worthwhile improvement over what was there before.
Attachment #111990 - Flags: review?(jgmyers) → review-
> I'm concerned that nsSmtpProtocol::ReadLine and nsSmtpProtocol::SmtpResponse
> only look for \n , whereas the proposed patch has
> nsSmtpProtocol::SendEhloResponse() look for \r\n.  It should match the other
> code by looking only for \n.

Then the other codes should be changed.


| RFC 2821             Simple Mail Transfer Protocol            April 2001
| 
| 
| 2.3.7 Lines
| 
|    SMTP commands and, unless altered by a service extension, message
|    data, are transmitted in "lines".  Lines consist of zero or more data
|    characters terminated by the sequence ASCII character "CR" (hex value
|    0D) followed immediately by ASCII character "LF" (hex value 0A).
|    This termination sequence is denoted as <CRLF> in this document.
|    Conforming implementations MUST NOT recognize or generate any other
|    character or character sequence as a line terminator.  Limits MAY be
|    imposed on line lengths by servers (see section 4.5.3).
| 
|    In addition, the appearance of "bare" "CR" or "LF" characters in text
|    (i.e., either without the other) has a long history of causing
|    problems in mail implementations and applications that use the mail
|    system as a tool.  SMTP client implementations MUST NOT transmit
|    these characters except when they are intended as line terminators
|    and then MUST, as indicated above, transmit them only as a <CRLF>
|    sequence.

So only /r/n is right.


> The m_responsetext += "\n"; line has a tab.

What do you think of?


MFG, HTH
Changing the code to no longer recognize bare LF in replies could be construed
as necessary to conform to the first MUST NOT directive in RFC 2821 section
2.3.7, though that requirement is primarily intended to apply to servers, not
clients.  Changing this would cause mozilla to stall the connection instead of
"just work" when talking to broken SMTP servers, if there are any such servers
out there.  I believe this issue should be a separate bug with a separate patch.

It is OK to leave existing tabs in the file, but per Mozilla policy the patch
should not introduce any new ones.  Please expand that tab to spaces.



Just to clarify, CharInSet() finds either \r or \n, but actually Mozilla
constructs the string itself from the response lines using only \n, so there is
no point in checking for \r where I did :) Fixed patch coming soon.
Attached patch Fixed patch (obsolete) — Splinter Review
(I meant FindCharInSet() in the last comment)

Here's a patch fixed as per John's comments.
Attachment #111990 - Attachment is obsolete: true
Attachment #112909 - Flags: review?(jgmyers)
Attachment #112909 - Flags: review?(jgmyers) → review+
Comment on attachment 112909 [details] [diff] [review]
Fixed patch

+	 nsCString responseLine;
you want an nsCAutoString here. Other than that, it looks OK, thx - sr=bienvenu
Attachment #112909 - Flags: superreview+
Same as before, just the nsCString changed to nsCAutoString.
Attachment #112909 - Attachment is obsolete: true
Comment on attachment 112948 [details] [diff] [review]
Fixed patch w/ nsCAutoString

Carrying over r and sr, seeking approval.
Attachment #112948 - Flags: superreview+
Attachment #112948 - Flags: review+
Attachment #112948 - Flags: approval1.3b?
sorry, wrong patch.  here's the right one.
(doing this to help reviewers and drivers assess risk)
Attachment #113029 - Attachment is obsolete: true
comments for drivers:

I just tested it, and stepped through it.  it looks ok, and relatively low risk.

bienvenu agrees, it seems low risk.

I think it's worth the risk, and worth taking for 1.3 beta.

Comment on attachment 112948 [details] [diff] [review]
Fixed patch w/ nsCAutoString

a=asa (on behalf of drivers) for checkin to 1.3beta.
Attachment #112948 - Flags: approval1.3b? → approval1.3b+
Oops, it was actually checked in 30th January.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
*** Bug 170872 has been marked as a duplicate of this bug. ***
*** Bug 169757 has been marked as a duplicate of this bug. ***
*** Bug 204910 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.