Bug 2642

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)

Created attachment 2895 [details]
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)

Created attachment 2897 [details]
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.

Created attachment 2898 [details]
fixed diff

typo in previous

(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

Created attachment 2900 [details]
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

Comment on attachment 2900 [details]
Only reset 'tried' count (on reset) and 'isprivate' (when freeing key)

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

Patch applied - this will be in OpenSSH 7.4. Thanks!

Close all resolved bugs after release of OpenSSH 7.7.