openssh unix dev - Jun 2015 - OpenSSH Linux portable patch proposal

Dear OpenSSH Developers,

I would like to propose a patch to OpenSSH for Linux. In the recent few
months, I have encountered a scenario where a PAM module used for
authentication in SSH should be informed about the previous successful
authentication methods. I described the complete scenario here:
http://serverfault.com/questions/690038/openssh-two-factor-authentication-combined-with-kerberos-public-key
In this use case, I want to introduce a 2nd factor for authentication while
accepting public key or GSSAPI authentication as first factor. If and only
if none of those methods were successful, a password authentication should
be performed before the second factor.

I also e-mailed this to this mailing list on 4 May. On the basis of a reply
from Damien Miller, there is currently no way to fully accomplish this
scenario with OpenSSH server. So I have made a PoC implementation that I
think does the trick:

https://github.com/dgyuri92/openssh-portable/commit/4a006cad8e3f8b9277ce41747d11261175c161e2

Would you be so kind as to take a look at it? Do you think it could be
beneficial for other users too? I think it would be a nice feature to have,
especially in use cases like mine and it is quite a small patch. Is there a
chance that this patch - or a functionally equivalent one - can be
integrated into future releases?

Thank you very much!

Cheers,
Gy?rgy Demarcsek

On 02/06/15 15:46, Gy?rgy Demarcsek Ifj. wrote:
> So I have made a PoC implementation that I think does the trick:
>
>
https://github.com/dgyuri92/openssh-portable/commit/4a006cad8e3f8b9277ce41747d11261175c161e2
>
> Would you be so kind as to take a look at it?
Minor cosmetic issue: you added space-indented lines to auth-pam.c, 
auth.h, auth2.c and session.c (last chunk), but those files are 
tab-indented.

You also removed a number of trailing spaces from the files, which make 
the patch harder to read.

> +  } else {
> +    am_copy = xstrdup(authctxt->last_auth_methods);
> +    free(authctxt->last_auth_methods);
> +    authctxt->last_auth_methods = xcalloc(strlen(am_copy) + 
> strlen(method) + 2, sizeof(char));
> +    strcpy(authctxt->last_auth_methods, am_copy);
> +    free(am_copy);
> +  }
Why not use realloc?

auth2_update_methods_lists() is called after authentication. Can't 
sshpam_auth_passwd be called
before auth2_update_methods_lists? (ie. last_auth_methods would be NULL) 
In that case do_pam_putenv()
would segfault...

On Tue, 2 Jun 2015, Gy?rgy Demarcsek Ifj. wrote:
> Dear OpenSSH Developers,
> 
> I would like to propose a patch to OpenSSH for Linux. In the recent few
> months, I have encountered a scenario where a PAM module used for
> authentication in SSH should be informed about the previous successful
> authentication methods. I described the complete scenario here:
>
http://serverfault.com/questions/690038/openssh-two-factor-authentication-combined-with-kerberos-public-key
I've wanted to expose more information about how the user authenticated
to the environment for a while, but I think that if we do it then we
should include (at least) key fingerprints too. Something like:

SSH_USER_AUTH=hostbased RSA SHA256:Iw75Ex+Re8WyIjqHEukxHtwz2weTFTBLPD2J9doYEfU,
publickey CA ED25519 SHA256:rLKEbjpoN2+kuMQB7EiPqaeHut65ZfSe/z1EaWtKEmk Cert ID
djm at mindrot.org Serial 27908739, password

We could probably expose this to PAM as well, as SSH_COMPLETED_AUTH or
similar.

Could you please file a bug at https://bugzilla.mindrot.org/ to track
this feature?

-d

On Wed, Jun 3, 2015 at 8:01 AM, ?ngel Gonz?lez <keisial at gmail.com>
wrote:
> On 02/06/15 15:46, Gy?rgy Demarcsek Ifj. wrote:
>
[...]
> You also removed a number of trailing spaces from the files, which make
> the patch harder to read.
>
We should clean up the trailing spaces upstream (there's more than I would
have thought) but that churn can wait until after the upcoming release.  I
agree that having no-op whitespace changes in the diff doesn't help.

Anyway, as to the diff itself:

+  if (authctxt->last_auth_methods == NULL) {
+    authctxt->last_auth_methods = xcalloc(strlen(method) + 2,
sizeof(char));

sizeof(char) == 1 by definition.

+  } else {
+    am_copy = xstrdup(authctxt->last_auth_methods);
+    free(authctxt->last_auth_methods);
+    authctxt->last_auth_methods = xcalloc(strlen(am_copy) + strlen(method)
+ 2, sizeof(char));
+    strcpy(authctxt->last_auth_methods, am_copy);

strcpy (and strcat) are not bounds checked (probably safe in this
particular case but we consider their use to be bad practice).

+    free(am_copy);
+  }
+
+  strcat(authctxt->last_auth_methods, method);
+  if (authctxt->num_auth_methods != 1)
+    strcat(authctxt->last_auth_methods, ",");

This seems to be an extremely complicated way of saying "append the method
to a comma separated list" (although it'll have a trailing comma, which
I'm
not sure is intended.  How about something like:

if (authctxt->last_auth_methods == NULL)
    authctxt->last_auth_methods = xstrdup(method);
else {
    am_copy = authctxt->last_auth_methods;
    xasprintf(&authctxt->last_auth_methods, "%s,%s", am_copy,
method);
    free(am_copy);
}

-- 
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4  37C9 C982 80C7 8FF4 FA69
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.