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

RESOLVED FIXED

Status

MailNews Core
Networking: SMTP
RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: Heiko Studt, Assigned: Ere Maijala (slow))

Tracking

Bug Flags:
blocking1.3b -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
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;
}
(Reporter)

Updated

16 years ago
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

Comment 1

16 years ago
change qa contact ->nbaca.
Ninoschka, 
Please reassign the bugs if not in your area.
QA Contact: sheelar → nbaca

Comment 2

16 years ago
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.
(Reporter)

Comment 3

16 years ago
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?

Comment 4

16 years ago
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 !...
(Reporter)

Comment 5

16 years ago
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;
}

Comment 6

16 years ago
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
(Reporter)

Comment 7

16 years ago
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 → ---

Updated

16 years ago
Flags: blocking1.3b?

Comment 8

16 years ago
would someone please attach a cvs diff -u patch?
(Assignee)

Comment 9

16 years ago
I'll take this one.
Assignee: mscott → ere
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 10

16 years ago
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
(Assignee)

Comment 11

16 years ago
Created attachment 111559 [details] [diff] [review]
Patch to enable better EHLO parsing

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.
(Assignee)

Updated

16 years ago
Attachment #111559 - Flags: review?(bienvenu)
(Reporter)

Comment 12

16 years ago
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. :-/
(Assignee)

Comment 13

16 years ago
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?
(Reporter)

Comment 14

16 years ago
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...
(Assignee)

Comment 15

16 years ago
There's bug 150212 for different SMTP authentication mechanisms.

Comment 16

16 years ago
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. 
(Assignee)

Comment 17

16 years ago
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 :)
(Reporter)

Comment 18

16 years ago
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... :)
(Assignee)

Comment 19

16 years ago
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. 

Comment 20

16 years ago
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.
(Assignee)

Comment 21

16 years ago
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)

Comment 22

16 years ago
The only issue I know of is servers which follow the typo in the internet draft,
producing "AUTH=LOGIN" instead of "AUTH LOGIN".
(Assignee)

Comment 23

16 years ago
Created attachment 111990 [details] [diff] [review]
Rewrite of the parser, quite strict now

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?
(Reporter)

Comment 24

16 years ago
It seems to be OK for me now, but do it handle *also* "auth=login"?
(Some servers are AFAIK also broken... :-/)
(Assignee)

Comment 25

16 years ago
Yes it does.
(Assignee)

Updated

16 years ago
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-
(Assignee)

Comment 27

16 years ago
John?

Comment 28

16 years ago
I just started looking at this today.  The day job tends to interfere, y'know.

Comment 29

16 years ago
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-
(Reporter)

Comment 30

16 years ago
> 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

Comment 31

16 years ago
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.



(Assignee)

Comment 32

16 years ago
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.
(Assignee)

Comment 33

16 years ago
Created attachment 112909 [details] [diff] [review]
Fixed patch

(I meant FindCharInSet() in the last comment)

Here's a patch fixed as per John's comments.
Attachment #111990 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #112909 - Flags: review?(jgmyers)

Updated

16 years ago
Attachment #112909 - Flags: review?(jgmyers) → review+

Comment 34

16 years ago
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+
(Assignee)

Comment 35

16 years ago
Created attachment 112948 [details] [diff] [review]
Fixed patch w/ nsCAutoString

Same as before, just the nsCString changed to nsCAutoString.
Attachment #112909 - Attachment is obsolete: true
(Assignee)

Comment 36

16 years ago
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?
Created attachment 113032 [details] [diff] [review]
cvs diff -uw version of the patch

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 40

16 years ago
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+
(Assignee)

Comment 41

16 years ago
Oops, it was actually checked in 30th January.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 42

15 years ago
*** Bug 170872 has been marked as a duplicate of this bug. ***

Comment 43

15 years ago
*** Bug 169757 has been marked as a duplicate of this bug. ***

Comment 44

15 years ago
*** 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.