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; }

change qa contact ->nbaca. Ninoschka, Please reassign the bugs if not in your area.

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.

would someone please attach a cvs diff -u patch?

I'll take this one.

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.

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.

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

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.

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.

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.

> 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.

(I meant FindCharInSet() in the last comment) Here's a patch fixed as per John's comments.

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

Same as before, just the nsCString changed to nsCAutoString.

Comment on attachment 112948 [details] [diff] [review] Fixed patch w/ nsCAutoString Carrying over r and sr, seeking approval.

sorry, wrong patch. here's the right one. (doing this to help reviewers and drivers assess risk)

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.

Oops, it was actually checked in 30th January.

*** 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. ***