openssh bugs - Nov 2016 - [Bug 2642] New: [sshconnect2] publickey authentication only properly works if used first: pubkey_prepare doesn't work after pubkey_cleanup

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

            Bug ID: 2642
           Summary: [sshconnect2] publickey authentication only properly
                    works if used first: pubkey_prepare doesn't work after
                    pubkey_cleanup
           Product: Portable OpenSSH
           Version: 7.3p1
          Hardware: amd64
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: git at lerya.net

When using multiple Authentication method after a successful partial
authentication, the following code is run
(https://github.com/openssh/openssh-portable/blob/master/sshconnect2.c#L562-L564):
```
                /* reset state */
                pubkey_cleanup(authctxt);
                pubkey_prepare(authctxt);
```

Unfortunately, this does _not_ reset the state!
- pubkey_cleanup is simple, it just closes the agent connection and
delete all keys in authctxt->keys
- pubkey_prepare populate authctxt->keys and can create an agent
connection. However it cannot be called twice, because it modifies
options.identity_keys and leaks options.certificates:

 * When reading identity_keys, when storing the key in a new 'identity'
structure, it runs
(https://github.com/openssh/openssh-portable/blob/master/sshconnect2.c#L1287):
```options.identity_keys[i] = NULL;```. As a result, any subsequent run
of this function, when getting the key via ```key options.identity_keys[i];```
will only be able to retrieve 'NULL'

 * When reading options.num_certificate_files, it does not replace
options.certificates[i] by NULL but simply copy the pointer in the new
'identity' structure. When pubkey_cleanup run, it will free this value,
making any subsequent run of this function access freed memory? (not
tested)

A clean solution could be to copy the key over, instead of replacing
the original by NULL or leaking and freeing the original, but as far as
I can see, there is no sshkey_copy/sshkey_dup function...


A simple way of reproducing the identity_keys part of the problem (I'm
not using certificate) is to:
- Configure sshd with AuthenticationMethods
keyboard-interactive:pam,publickey
- Generate a public/private key
- Start an ssh agent, add the key
- Run ssh -i ${publickeyfile} -o IdentitiesOnly=yes -vv ${host},
properly authenticate with the password and see the publickey
authentication failing, logs with contain:
```
debug2: key: ${publickeyfile} (${pointer}), explicit, agent
[...]
Authenticated with partial success.
debug2: key: ${publickeyfile} ((nil)), explicit
```
The two key lines should have been identical

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

--- Comment #1 from Vincent Brillault <git at lerya.net> ---
I believe I've been able to observe the bug on the certificate path.
Step to reproduce:
- Configure sshd with AuthenticationMethods
keyboard-interactive:pam,publickey (in fact, can be any combination of
2 methods)
- Generate a valid certificate file
- Run ssh -o 'CertificateFile=${certfile}' -o IdentitiesOnly=yes -vvv
${host}, properly authenticate the first time. Logs should contain:
 * `debug2: key: ${certfile} (${pointer}), explicit` before the first
authentication
 * No corresponding line after the first authentication (the
certificate disappeared)

On my setup, `key_is_cert(key)` seems to return 0 when accessing the
freed memory, leading not to a crash but simply to that key being
ignored.

If run under valgrind, logs should contain (using
1a6f9d2e2493d445cd9ee496e6e3c2a2f283f66a of
https://github.com/openssh/openssh-portable):
Authenticated with partial success.
==25112== Invalid read of size 4
==25112==    at 0x1300E9: sshkey_is_cert (sshkey.c:308)
==25112==    by 0x1253A6: pubkey_prepare (sshconnect2.c:1298)
==25112==    by 0x1289F6: input_userauth_failure (sshconnect2.c:564)
==25112==    by 0x154758: ssh_dispatch_run (dispatch.c:119)
==25112==    by 0x12852B: ssh_userauth2 (sshconnect2.c:402)
==25112==    by 0x124D56: ssh_login (sshconnect.c:1383)
==25112==    by 0x113898: main (ssh.c:1418)
==25112==  Address 0x6138060 is 0 bytes inside a block of size 64
free'd
==25112==    at 0x4C2C4AB: free (vg_replace_malloc.c:473)
==25112==    by 0x12597A: pubkey_cleanup (sshconnect2.c:1411)
==25112==    by 0x1289EE: input_userauth_failure (sshconnect2.c:563)
==25112==    by 0x154758: ssh_dispatch_run (dispatch.c:119)
==25112==    by 0x12852B: ssh_userauth2 (sshconnect2.c:402)
==25112==    by 0x124D56: ssh_login (sshconnect.c:1383)
==25112==    by 0x113898: main (ssh.c:1418)

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Vincent Brillault <git at lerya.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |git at lerya.net

--- Comment #2 from Vincent Brillault <git at lerya.net> ---
Created attachment 2895
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2895&action=edit
Only reorder and resent count of authctxt->keys between authentications

(Sorry for the double-posting, I am not sure what is the preferred way
of submitting patches)

While taking another look at the code, I realised that most of the
accesses to the authctxt->keys list or its content do not modify it
(the attached patch 'constifies' the arguments functions called on the
content of the list, to make it easier to see that they don't modify
them). There is only one place (not counting prepare/cleanup) that
modifies it, userauth_pubkey. That function:
- Re-order the key, increasing the tries count each time (up to 2 if it
loops)
- Set the isprivate flag on private keys when they are loaded

This patch (also available at
https://github.com/openssh/openssh-portable/pull/57):
- Unset the isprivate flag on private keys when they are freed/cleared
- Add a pubkey_reset function (called between authentication) that
re-re-order the keys and reset the 'tries' count

This patch/the code could be simplified:
- The 'constification' could be ignored
- If we don't care about the order in which keys are tested, the
re-ordering could be removed
- pubkey_reset could be inlined (esp. if the reordering is removed)
- pubkey_cleanup could be inlined (only called once)

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Created attachment 2897
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2897&action=edit
more clear pubkey_reset()

Thanks for looking at this.

I not sure I properly understand why you change modifies id->tried. Is
it to move all tried keys to the end of the list? I think it might be
clearer to do something like the attached. It's a little longer, but
IMO easier to understand its intent.

Also, I don't understand why you reset isprivate. I think this might
cause re-prompting for passwords to load keys that have already been
loaded.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Created attachment 2898
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2898&action=edit
fixed diff

typo in previous

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2594


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
(In reply to Damien Miller from comment #4)
> Created attachment 2898 [details]
> fixed diff
> 
> typo in previous
oh, I see. You reset isprivate because the key is subsequently freed

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Vincent Brillault <git at lerya.net> changed:

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

--- Comment #6 from Vincent Brillault <git at lerya.net> ---
Created attachment 2900
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2900&action=edit
Only reset 'tried' count (on reset) and 'isprivate' (when
freeing key)

Thanks for the review!
> I not sure I properly understand why you change modifies id->tried.
My goal was to emulate the existing behavior:
- As the key list was rebuild between two authentications, the order
was always the same, with 'tried' set to 0 (xcalloc)
- When a key is tried, it is immediately moved to the end of the list
and the 'tried' counter is increased

My first loop continues to move keys to the end until the original
order is found (all keys have been 'tried' (i.e moved) the same time)

My second loop is resetting the 'tried' count because I understand it
is used in userauth_pubkey in the while loop to make that the keys are
not used twice (see 
https://github.com/openssh/openssh-portable/blob/master/sshconnect2.c#L1438).
> Is it to move all tried keys to the end of the list? I think it
> might be clearer to do something like the attached. It's a little
> longer, but IMO easier to understand its intent.
Mmm, I understand that by design, all keys are already at the end of
the list. If resetting the order is not important, just clearing the
id->tried should be enough
>> Also, I don't understand why you reset isprivate. I think this
might
>> cause re-prompting for passwords to load keys that have already been
>> loaded.
> oh, I see. You reset isprivate because the key is subsequently freed
Exactly!
I was not sure where to reset the value, in the 'if' block, where the
value is initialized (but it's not freed yet, so why?) or when it's
freed (but it was not always initialized on that execution path).

I've attached a stripped-down version of the patch, which only reset
the 'tried' count and reset the 'isprivate' id after the key has
been
freed

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au
   Attachment #2900|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2900
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2900
Only reset 'tried' count (on reset) and 'isprivate' (when
freeing key)

I think this is correct. Can you take a look, Darren?

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2900|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
Patch applied - this will be in OpenSSH 7.4. Thanks!

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2642

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after release of OpenSSH 7.7.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.