openssh bugs - Sep 2003 - [Bug 702] dont call userauth_finish after auth2_challenge_stop

http://bugzilla.mindrot.org/show_bug.cgi?id=702

           Summary: dont call userauth_finish after auth2_challenge_stop
           Product: Portable OpenSSH
           Version: 3.7.1p1
          Platform: UltraSparc
        OS/Version: Solaris
            Status: NEW
          Severity: major
          Priority: P2
         Component: PAM support
        AssignedTo: openssh-bugs at mindrot.org
        ReportedBy: paul.a.bolton at bt.com


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.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=702





------- Additional Comments From paul.a.bolton at bt.com  2003-09-23 02:57
-------
Created an attachment (id=447)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=447&action=view)
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.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=702

markus at openbsd.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #447 is|0                           |1
           obsolete|                            |



------- Additional Comments From markus at openbsd.org  2003-09-23 04:11 -------
Created an attachment (id=448)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=448&action=view)
alternative patch.

perhaps something like this?



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=702

markus at openbsd.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #448 is|0                           |1
           obsolete|                            |



------- Additional Comments From markus at openbsd.org  2003-09-23 04:18 -------
Created an attachment (id=449)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=449&action=view)
call userauth_finish early

i don't see, why userauth finish cannot be called early....



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=702





------- Additional Comments From paul.a.bolton at bt.com  2003-09-23 04:31
-------
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.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=702





------- Additional Comments From paul.a.bolton at bt.com  2003-09-23 04:37
-------
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.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=702

paul.a.bolton at bt.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #447 is|1                           |0
           obsolete|                            |





------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=702

paul.a.bolton at bt.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #448 is|1                           |0
           obsolete|                            |





------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=702

paul.a.bolton at bt.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #449 is|0                           |1
           obsolete|                            |





------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=702





------- Additional Comments From markus at openbsd.org  2003-09-23 20:07 -------
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.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.