Closed
Bug 151279
Opened 22 years ago
Closed 22 years ago
SMTP-Auth is not found on a RFC-Valid-Response
Categories
(MailNews Core :: Networking: SMTP, defect)
MailNews Core
Networking: SMTP
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bugzilla, Assigned: emaijala+moz)
References
Details
Attachments
(2 files, 4 obsolete files)
3.64 KB,
patch
|
emaijala+moz
:
review+
emaijala+moz
:
superreview+
asa
:
approval1.3b+
|
Details | Diff | Splinter Review |
2.65 KB,
patch
|
Details | Diff | Splinter Review |
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•22 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•22 years ago
|
||
change qa contact ->nbaca. Ninoschka, Please reassign the bugs if not in your area.
QA Contact: sheelar → nbaca
Comment 2•22 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•22 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•22 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•22 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•22 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•22 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.
Updated•22 years ago
|
Flags: blocking1.3b?
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•22 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•22 years ago
|
||
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•22 years ago
|
Attachment #111559 -
Flags: review?(bienvenu)
Reporter | ||
Comment 12•22 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•22 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•22 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•22 years ago
|
||
There's bug 150212 for different SMTP authentication mechanisms.
Comment 16•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 years ago
|
||
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•22 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•22 years ago
|
||
Yes it does.
Assignee | ||
Updated•22 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•22 years ago
|
||
John?
Comment 28•22 years ago
|
||
I just started looking at this today. The day job tends to interfere, y'know.
Comment 29•22 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•22 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•22 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•22 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•22 years ago
|
||
(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•22 years ago
|
Attachment #112909 -
Flags: review?(jgmyers)
Updated•22 years ago
|
Attachment #112909 -
Flags: review?(jgmyers) → review+
Comment 34•22 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•22 years ago
|
||
Same as before, just the nsCString changed to nsCAutoString.
Attachment #112909 -
Attachment is obsolete: true
Assignee | ||
Comment 36•22 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?
Comment 37•22 years ago
|
||
Comment 38•22 years ago
|
||
sorry, wrong patch. here's the right one. (doing this to help reviewers and drivers assess risk)
Attachment #113029 -
Attachment is obsolete: true
Comment 39•22 years ago
|
||
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•22 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•22 years ago
|
||
Oops, it was actually checked in 30th January.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 42•21 years ago
|
||
*** Bug 170872 has been marked as a duplicate of this bug. ***
Comment 43•21 years ago
|
||
*** Bug 169757 has been marked as a duplicate of this bug. ***
Comment 44•21 years ago
|
||
*** Bug 204910 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Product: MailNews → Core
Updated•15 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•