266828 - Subscribing to a "write only" newsgroup causes Thunderbirds password manager to fail.

Status: RESOLVED DUPLICATE of bug 435306

(Thunderbird :: Security, defect)

Description

[Aaron W. A. Zabell]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10.1

The password manager breaks and TBird hangs when I subscribe to a "write only"
newsgroup.
I'm subscribed to several newsgroups (all on the same NNTP server) in which one
of the subscriptions has "write only" properties. If I'm not subscribed to the
"write only" group, all is well, but when I subscribe to it I get the standard
"A News (NNTP) error occurred: no permission" both in 1.7b, 0.5+ and Tbird 0.8
(expected behavior) and when I try and access the other subscriptions it always
asks for a user name and password whether I check the "save password" dialog box
or not (obviously when not).

Reproducible: Always
Steps to Reproduce:
1.Subscribe to various newsgroups including the "write only" group on the same
NNTP server.
2.Check dialog to remember password when prompted.
3.Access the "write only" newsgroup.



Expected Results:  
Thunderbird should remember the password saved in the password manager.

There is only one known workaround that I've been able to implement. Create a
seperate profile for the sole purpose of accessing the "write only" subscription.

Comment 1

[David :Bienvenu]

can we get an NNTP protocol log:

http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap

Comment 2

[Aaron W. A. Zabell]

Attachment: NNTP log#1 pre-write only, pre-password manager.

See comments at the beginning of the log file.

Comment 3

[Aaron W. A. Zabell]

Attachment: NNTP log#2 pre-write only, post-password manager.

See comments at the beginning of the log file.

Comment 4

[Aaron W. A. Zabell]

Attachment: NNTP log#3 post-write only, post-password manager.

See comments at the beginning of the log file.

Comment 5

[Aaron W. A. Zabell]

Attachment: NNTP log#3a post-write only, post-password manager, a variation.

See comments at the beginning of the log file.

Comment 6

[Aaron W. A. Zabell]

(In reply to comment #1)
> can we get an NNTP protocol log:
> 
> http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap

Hi David,

Thanks for responding so quickly.
I hope these logs help.

Aaron

Comment 7

[Christian Eyrich]

Thanks for all the logs, Aaron.

What happens is simple:
if (m_responseCode == 502) {
  /* forget the password & username, since login failed */
  rv = m_newsFolder->ForgetGroupUsername();
  rv = m_newsFolder->ForgetGroupPassword();
}

And that's ok since according to RFC 2980 502 means the user/password
combination is not valid.

In my opinion trying to determine a wrong login and discarding the password is a
weakness in the way Mozilla works. It's well meant but it always causes trouble
in Mail and also here in News.
If we don't change the fundamental way it works or at least stop forgetting
passwords at all, there's only one thing I can think of to help here.

We could remove the above code from NewsResponse() at let solely the
authentication functions AuthorizationResponse() and PasswordResponse() react
with forgetting credentials after a 502 response.
Secondly reaction on MK_NNTP_RESPONSE_PERMISSION_DENIED in NewsResponse() needs
to be modified. Currently it only leaves and directly returns to NewsResponse()
since m_nextState doesn't get updated. So it waits for new bytes forever.

Comment 8

[Aaron W. A. Zabell]

Thanks for your help Christian,

Hmm, I think I'm following you on this. If the lines of code were adjusted to
what you mentioned it would eliminate the problem with the password manager but
TBird would still be "broken" for this particular kind of situation. (Please
bear with me as I've only taken introductory programming classes, so my comments
may be oversimplified or incorrect or both) Now, to correct the "real" problem
this would entail some major overhaul of TBirds source code correct? Or is it
potentially a rewrite of a few lines of code? Regardless, I'd be happy to take a
look at the source and make a feeble attempt to correct the problem, just point
me in the right direction as far as where I should start looking in the source
code, I'll give it my best shot.

Aaron

Comment 9

[Christian Eyrich]

(In reply to comment #8)
> If the lines of code were adjusted to what you mentioned it would eliminate
> the problem with the password manager but TBird would still be "broken" for
> this particular kind of situation.

Since I'm not that familar with the NNTP code I can't say if what I suggested
doesn't create a problem somewhere else. But I think it should correct this issue.

> Now, to correct the "real" problem this would entail some major overhaul of
> TBirds source code correct? Or is it potentially a rewrite of a few lines of
> code?

IMO the "real problem" is that we try to detect when it's right to forget the
password to be nice to the user. But in reality there are numerous situations
(answers unexpected by the coder due to not fully reading the spec or real
faulty servers a.s.o.) where this leads to problems.
To help here some deeper changes and rewrites would be needed, yes.

But a) such changes are always dangerous too and b) if what I wrote about works,
it's unnecessary regarding your issue.

> Regardless, I'd be happy to take a look at the source and make a feeble
> attempt to correct the problem, just point me in the right direction as far as
> where I should start looking in the source code, I'll give it my best shot.

We don't expect you do to it. But to get an idea of the code, visit
http://lxr.mozilla.org/seamonkey/source/mailnews/news/src/nsNNTPProtocol.cpp and
search for the functions I mentioned.
I can develope a patch based on what I proposed. But what would be needed before
applying or even reviewing it are some tests on a real account with a write-only
newsgroup. 

CC'ing David - maybe you can say something about password forgetting.

Comment 10

[Aaron W. A. Zabell]

(In reply to comment #9)
> Since I'm not that familar with the NNTP code I can't say if what I suggested
> doesn't create a problem somewhere else. But I think it should correct this issue.

Right, even with the limited experience I have in programming, changing a few
lines of code can cause everyone to have a real bad day.

> I can develope a patch based on what I proposed. But what would be needed before
> applying or even reviewing it are some tests on a real account with a write-only
> newsgroup. 

I'd be more than happy to beta test. :)

Comment 11

[Aaron W. A. Zabell]

(In reply to comment #9)
In regards to,
> http://lxr.mozilla.org/seamonkey/source/mailnews/news/src/nsNNTPProtocol.cpp

Is it possible that lines 1562-8 or 1567-8 are causing the loop problem?
1562     }
1563     else {
1564         m_nextState = m_nextStateAfterResponse;
1565     }
1566 
1567     PR_FREEIF(line);
1568     return(0);  /* everything ok */

http://lxr.mozilla.org/seamonkey/source/mailnews/news/src/nsNNTPProtocol.cpp#1562

Esp. 1567-8 since these are a duplicate of what happens immediately after 1558
---------------
1558     else if (MK_NNTP_RESPONSE_PERMISSION_DENIED == m_responseCode)
1559     {
1560         PR_FREEIF(line);
1561         return (0);
1562     }
---------------
Or possibly 1560-1 being part of the issue. I dunno, I'm probably barking up the
wrong tree. I should probably just build TBird and play with the code a little
and see what happens, rather than asking questions that potentially take up
unnecessary space.

Comment 12

[Christian Eyrich]

Attachment: example patch

This patch is what I wrote about in comment #7.
It removes the NewsResponse()'s own 502-handling and makes it just throw an
alert.
To prevend Mozilla from waiting for further data from the server after the 502
(while the server waits for another command from us), m_nextState must get
changed to something else than it currently is (NNTP_RESPONSE).
The question is into what. Technically what my patch does (just continue on our
plan) works (at least here on my simulated news server) for the specific
problem since DisplayNewsRCResponse() (the function at the next state) can
handle unexpected responses.

But
a) What happens if the response commes on more groups or while marking articles
read (for whatever reason). This could result in hundred alerts and no way for
the user to cancel.
b) What if the next function isn't able to handle an unexpected response?

Additionally it would be great to not only quote the server response (e.g. "no
permission") but also inform the user, what command wasn't permitted. But IMO
this would require a facility to save our last command.

Comment 13

[Aaron W. A. Zabell]

(In reply to comment #12)

> This patch is what I wrote about in comment #7.
> It removes the NewsResponse()'s own 502-handling and makes it just throw an
> alert.

Thanks, I'll give it a try. Might be a few days though.

Sorry I've not responded lately, was on vacation.

Comment 14

[Ronald Killmer]

I am seeing far too many *502* errors while working the authenticated staff work
groups on news.annexcafe.com.  If I can't type a reply and post it within 35
seconds of opening a post, my send is rebuffed by the server with a 502 error.

This is not happening to any of the OE using staffers.  MS on this issue is very
user friendly.  Only guess is that OE is auto resending the ID/Pass as a packet
with the NNTP posting.

No proxies, direct access to the host server..  

Comment 15

[Ronald Killmer]

(In reply to comment #14)
> I am seeing far too many *502* errors while working the authenticated staff work
> groups on news.annexcafe.com.  If I can't type a reply and post it within 35
> seconds of opening a post, my send is rebuffed by the server with a 502 error.
> 
> This is not happening to any of the OE using staffers.  MS on this issue is very
> user friendly.  Only guess is that OE is auto resending the ID/Pass as a packet
> with the NNTP posting.
> 
> No proxies, direct access to the host server..  

The *502* errors are happening with a valid user id/pass combo with very high
frequency during TB startup as it auto gets msgs from all accounts.  Two servers
are requiring id/pass, news.annexcafe.com and news.mozdev.org where the
annexcafe requirement is for staff access to staff limited groups while the
mozdev purpose is controlled general server access.

I have yet to have a problem with the mozdev NNTP server. My concern is for
cases where there is a mixed subscription of public and controlled access groups
on a single specific NNTP server.

While the specs say a *502* is an id/pass failure the reality is this error will
happen during authentication collisions on the NNTP server.  Additionaly if the
NNTP server has an invalid id/pass limit of 3 per 30 minutes it can block all
access's even by valid id/pass access requests.  This is the case for 
annexcafe.com were the rule is set to block hacker intrusions and spam assults.

The assumtion that all *502* errors are UA caused is invalid and TB should have
a rule that permits a three strikes and your out before deleting id/pass.  Thus
permitting the user to execute two retries.  Also would be nice to get a warning
with the error alert that TB will delete the id/pass so users know what is
happening.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

This appears to be another manifestation of bug 286628.

*** This bug has been marked as a duplicate of 286628 ***

Comment 17

[Aaron W. A. Zabell]

This bug is still not resolved in v.1.5.0.5. and is unrelated to bug 286628. That bug is a periodic, seemingly at random, forgetting of the user ID and password. This bug is always repeatable, always occurs, so long as the "write only" newsgroup is subscribed to. I'm not sure what else to do here. Bug 278318 is a duplicate of this one.

Comment 18

[KC]

(In reply to comment #17)
> This bug is still not resolved in v.1.5.0.5. and is unrelated to bug 286628.
> That bug is a periodic, seemingly at random, forgetting of the user ID and
> password. This bug is always repeatable, always occurs, so long as the "write
> only" newsgroup is subscribed to. I'm not sure what else to do here. Bug 278318
> is a duplicate of this one.
> 

I am wondering if this is a problem with UOP newsgroups.  I too have issues with the write-only access newsgroup folder. TB stalls when checking the write-only newsgroup folder.  There seems to be a line missing that says to finish checking the rest of the groups after acknowledgment of the "News (NNTP error: no permission" box comes up.  I had seen this activity when I reported bug #359989. 

Comment 19

[Aaron W. A. Zabell]

(In reply to comment #18)
> I am wondering if this is a problem with UOP newsgroups.  I too have issues
> with the write-only access newsgroup folder. TB stalls when checking the
> write-only newsgroup folder.  There seems to be a line missing that says to
> finish checking the rest of the groups after acknowledgment of the "News (NNTP
> error: no permission" box comes up.  I had seen this activity when I reported
> bug #359989. 
> 

Yes, that is correct. This is the primary reason for opening the bug. Though, obviously, it may not be limited only to UOP "write only" newsgroups.

Comment 20

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Christian, could you please come up with an updated patch? (Patch no longer applies on my tree)

Comment 21

[Wayne Mery (:wsmwk)]

joshua wnat to take?

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 164731 [details] [diff] [review]
example patch

Patch has bitrotted.

$ patch -p0 --dry-run < ~/Desktop/attachment-1.cgi.txt 
patching file mailnews/news/src/nsNNTPProtocol.cpp
Hunk #1 FAILED at 1601.
Hunk #2 FAILED at 1609.
2 out of 2 hunks FAILED -- saving rejects to file mailnews/news/src/nsNNTPProtocol.cpp.rej

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Gary, do you really think that, after his patch was ignored for 4.5 years,
Christian will now JUMP at the chance to update his patch?  

Maybe Momo's response should be "We're sorry that your patch was ignored 
for 4.5 years, and now we'll try to unbitrot it so that your effort won't 
have been a total waste."

Comment 24

[Christian Eyrich]

> Christian will now JUMP at the chance to update his patch?  

No, I can't and I won't. Besides I'm out of practice with this code, the path never was more than an example, call it request for comments.
But everbody's surely free to unbitrot and play with it.

Comment 25

[Joshua Cranmer [:jcranmer]]

Looking at the patch, it's merely that Mozilla

Comment 26

[Joshua Cranmer [:jcranmer]]

Whoops, got cut off.

I meant to say:

Looking at the patch, it's merely that Mozilla is forgetting the password on the first authentication error, which is what bug 435306 would aim to fix.