Bug 702 - dont call userauth_finish after auth2_challenge_stop
Summary: dont call userauth_finish after auth2_challenge_stop
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: PAM support (show other bugs)
Version: -current
Hardware: UltraSPARC Solaris
: P2 major
Assignee: OpenSSH Bugzilla mailing list
URL:
Keywords: openbsd, patch
Depends on:
Blocks:
 
Reported: 2003-09-23 02:55 AEST by Paul Bolton
Modified: 2008-04-04 09:54 AEDT (History)
0 users

See Also:


Attachments
call auth2_challenge_stop after userauth_finish (740 bytes, patch)
2003-09-23 02:57 AEST, Paul Bolton
no flags Details | Diff
alternative patch. (1.18 KB, patch)
2003-09-23 04:11 AEST, Markus Friedl
no flags Details | Diff
call userauth_finish early (673 bytes, patch)
2003-09-23 04:18 AEST, Markus Friedl
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Bolton 2003-09-23 02:55:09 AEST
Very occasionally users are experiencing sessions bailing on authentication with
a  "fatal: ssh_msg_send: write". After some analysis is seems that the common
factor is a Solaris account management module is printing a message via the
conversation function (e.g. Your password will expire in 7 days...).

It looks as if in auth2-chall.c in input_userauth_info_response() is the
culprit. auth2_challenge_stop() will eventually cause sshpam_free_ctx() to be
called in auth-pam.c, which will free ctxt. This contains important file
descriptors for the conversation function, which get closed before the free
(which is correct).

userauth_finish() can call do_account() if PAM is enabled. As
auth2_challenge_stop() is getting called beforehand, if the module generates
messages invalid references for FD's are found (probably because the memory has
been malloc'ed again no SEGV) and the error detailed above is activated.

It seems that it is possible to call auth2_challenge_stop() after
userauth_finish(). However there are a few comments in the code I have seen that
I don't like in relation to doing this. I will attach a patch with this bug.
Please can you advise on any possible issues in doing this, as the change would
probably need more sanity checking.
Comment 1 Paul Bolton 2003-09-23 02:57:57 AEST
Created attachment 447 [details]
call auth2_challenge_stop after userauth_finish

Biggest concern (at present) along with the comments in the original bug
submission is the comment " may set authctxt->postponed" for
auth2_challenge_start.
Comment 2 Markus Friedl 2003-09-23 04:11:33 AEST
Created attachment 448 [details]
alternative patch.

perhaps something like this?
Comment 3 Markus Friedl 2003-09-23 04:18:41 AEST
Created attachment 449 [details]
call userauth_finish early

i don't see, why userauth finish cannot be called early....
Comment 4 Paul Bolton 2003-09-23 04:31:17 AEST
That would be the way to maintain that part of the original logic. My concern is
in the form of the question "should we act upon the potentially changed value of
authctxt->postponed within the same call to input_userauth_info_response() ?"

Looking at auth2_challenge_stop() seems to suggest (on the face of it) the
original patch would be correct, as this function removes a callback from the
dispatch table and frees the kbdintctxt memory. The patch avoids adding more
(potentially unnecessary) variables into the code.

So, is it a case that it is safe to do, or was the original comment a warning
against doing what I have just described?

Obviously, if no-one can answer that, then play safe and go with the change to
the patch that you have submitted.
Comment 5 Paul Bolton 2003-09-23 04:37:15 AEST
I dont think you can call userauth_finish() early as auth2_challenge_start will
call the init_ctx. So for PAM this is sshpam_init_ctx(), which will set up PAM
including calling pam_start(). Calling any PAM function before pam_start() is bad.
Comment 6 Markus Friedl 2003-09-23 20:07:29 AEST
yes, my last patch was broken.

when authentication fails and postpone is not
set, then we need to call challenge_start _before_
userauth_finish because _start will set postpone
if it sends a packet to the client. if this is the
case then userauth_finish will behave different
and send _no_ failure packet. so the patch 448
or 447 are better.

however, the comments in the code need to explain
the mess better.
Comment 7 Darren Tucker 2004-02-15 16:12:42 AEDT
I think we should look at this for 3.8.

When you say "Solaris account management module is printing a message via the
conversation function (e.g. Your password will expire in 7 days...)" do you mean
that Solaris account modules are writing to arbitary descriptors, rather than
returning messages to the PAM conversation function?
Comment 8 Darren Tucker 2004-03-30 12:49:19 AEST
Does this still occur with a current snapshot?  A "pam_store_conv()" function
was added to catch messages from PAM modules for display to the user after login.

Of course, if the pam module is just writing to stderr then all bets are off...
Comment 9 Darren Tucker 2004-07-01 16:16:54 AEST
Can we close this bug?  Expiry warnings have worked for a while, are there
further changes to the challenge-response code required?

$ ssh -V
OpenSSH_3.8.1p1, OpenSSL 0.9.7d 17 Mar 2004
$ ssh testuser@localhost
Password:
Your password will expire in 7 days.

Sun Microsystems Inc.   SunOS 5.8       Generic February 2000
You have mail.
Comment 10 Damien Miller 2007-02-05 11:49:04 AEDT
I think this was fixed two and a half years ago. Please reopen if you can reproduce it with OpenSSH-4.5
Comment 11 Damien Miller 2008-04-04 09:54:48 AEDT
Close resolved bugs after release.