Marco Trevisan
2025-Feb-13 16:22 UTC
[PATCH 0/4] [PATCH] Immediately report interactive instructions
From: "Marco Trevisan" <marco at ubuntu.com> This serie of patches have been already submitted via [1], but i'm sending them again to the ML, to see if they can get some more traction. The patches are already part of Ubuntu openssh since 24.04, and they basically allow proper immediate instruction reporting to clients using PAM (as per RFC4256). This follows the approach that has been reported and discussed in bug 2876 [2], although as per discussion in the said PR thare have been introduced no options to keep previous behavior working. [1] https://github.com/openssh/openssh-portable/pull/452 [2] https://bugzilla.mindrot.org/show_bug.cgi?id=2876 Marco Trevisan (Trevi?o) (4): auth: Add KbdintResult definition to define result values explicitly auth-pam: Add an enum to define the PAM done status auth-pam: Add debugging information when we receive PAM messages auth-pam: Immediately report interactive instructions to clients auth-bsdauth.c | 2 +- auth-pam.c | 51 ++++++++++++++++++++++++++++++-------------------- auth.h | 5 +++++ auth2-chall.c | 4 ++-- 4 files changed, 39 insertions(+), 23 deletions(-) -- 2.34.1
Marco Trevisan
2025-Feb-13 16:22 UTC
[PATCH 1/4] auth: Add KbdintResult definition to define result values explicitly
From: Marco Trevisan (Trevi?o) <mail at 3v1n0.net>
kbdint result vfunc may return various values, so use an enum to make it
clearer what each result means without having to dig into the struct
documentation.
---
auth-bsdauth.c | 2 +-
auth-pam.c | 10 +++++-----
auth.h | 5 +++++
auth2-chall.c | 4 ++--
4 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/auth-bsdauth.c b/auth-bsdauth.c
index d124e994e..ca41735de 100644
--- a/auth-bsdauth.c
+++ b/auth-bsdauth.c
@@ -111,7 +111,7 @@ bsdauth_respond(void *ctx, u_int numresponses, char
**responses)
authctxt->as = NULL;
debug3("bsdauth_respond: <%s> = <%d>", responses[0],
authok);
- return (authok == 0) ? -1 : 0;
+ return (authok == 0) ? KbdintResultFailure : KbdintResultSuccess;
}
static void
diff --git a/auth-pam.c b/auth-pam.c
index 13c0a792e..5dfa69202 100644
--- a/auth-pam.c
+++ b/auth-pam.c
@@ -990,15 +990,15 @@ sshpam_respond(void *ctx, u_int num, char **resp)
switch (ctxt->pam_done) {
case 1:
sshpam_authenticated = 1;
- return (0);
+ return KbdintResultSuccess;
case 0:
break;
default:
- return (-1);
+ return KbdintResultFailure;
}
if (num != 1) {
error("PAM: expected one response, got %u", num);
- return (-1);
+ return KbdintResultFailure;
}
if ((buffer = sshbuf_new()) == NULL)
fatal("%s: sshbuf_new failed", __func__);
@@ -1015,10 +1015,10 @@ sshpam_respond(void *ctx, u_int num, char **resp)
}
if (ssh_msg_send(ctxt->pam_psock, PAM_AUTHTOK, buffer) == -1) {
sshbuf_free(buffer);
- return (-1);
+ return KbdintResultFailure;
}
sshbuf_free(buffer);
- return (1);
+ return KbdintResultAgain;
}
static void
diff --git a/auth.h b/auth.h
index 98bb23d4c..aba6e775d 100644
--- a/auth.h
+++ b/auth.h
@@ -51,6 +51,7 @@ struct sshauthopt;
typedef struct Authctxt Authctxt;
typedef struct Authmethod Authmethod;
typedef struct KbdintDevice KbdintDevice;
+typedef int KbdintResult;
struct Authctxt {
sig_atomic_t success;
@@ -115,6 +116,10 @@ struct Authmethod {
int (*userauth)(struct ssh *, const char *);
};
+#define KbdintResultFailure -1
+#define KbdintResultSuccess 0
+#define KbdintResultAgain 1
+
/*
* Keyboard interactive device:
* init_ctx returns: non NULL upon success
diff --git a/auth2-chall.c b/auth2-chall.c
index 021df8291..047d4e83c 100644
--- a/auth2-chall.c
+++ b/auth2-chall.c
@@ -331,11 +331,11 @@ input_userauth_info_response(int type, u_int32_t seq,
struct ssh *ssh)
free(response);
switch (res) {
- case 0:
+ case KbdintResultSuccess:
/* Success! */
authenticated = authctxt->valid ? 1 : 0;
break;
- case 1:
+ case KbdintResultAgain:
/* Authentication needs further interaction */
if (send_userauth_info_request(ssh) == 1)
authctxt->postponed = 1;
--
2.34.1
Marco Trevisan
2025-Feb-13 16:22 UTC
[PATCH 2/4] auth-pam: Add an enum to define the PAM done status
From: Marco Trevisan (Trevi?o) <mail at 3v1n0.net>
Makes things more readable and easier to extend
---
auth-pam.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/auth-pam.c b/auth-pam.c
index 5dfa69202..ba01dfb0c 100644
--- a/auth-pam.c
+++ b/auth-pam.c
@@ -132,11 +132,16 @@ typedef pid_t sp_pthread_t;
#define pthread_join fake_pthread_join
#endif
+typedef int SshPamDone;
+#define SshPamError -1
+#define SshPamNone 0
+#define SshPamAuthenticated 1
+
struct pam_ctxt {
sp_pthread_t pam_thread;
int pam_psock;
int pam_csock;
- int pam_done;
+ SshPamDone pam_done;
};
static void sshpam_free_ctx(void *);
@@ -904,7 +909,7 @@ sshpam_query(void *ctx, char **name, char **info,
**prompts = NULL;
*num = 0;
**echo_on = 0;
- ctxt->pam_done = -1;
+ ctxt->pam_done = SshPamError;
free(msg);
sshbuf_free(buffer);
return 0;
@@ -931,7 +936,7 @@ sshpam_query(void *ctx, char **name, char **info,
import_environments(buffer);
*num = 0;
**echo_on = 0;
- ctxt->pam_done = 1;
+ ctxt->pam_done = SshPamAuthenticated;
free(msg);
sshbuf_free(buffer);
return (0);
@@ -944,7 +949,7 @@ sshpam_query(void *ctx, char **name, char **info,
*num = 0;
**echo_on = 0;
free(msg);
- ctxt->pam_done = -1;
+ ctxt->pam_done = SshPamError;
sshbuf_free(buffer);
return (-1);
}
@@ -988,10 +993,10 @@ sshpam_respond(void *ctx, u_int num, char **resp)
debug2("PAM: %s entering, %u responses", __func__, num);
switch (ctxt->pam_done) {
- case 1:
+ case SshPamAuthenticated:
sshpam_authenticated = 1;
return KbdintResultSuccess;
- case 0:
+ case SshPamNone:
break;
default:
return KbdintResultFailure;
--
2.34.1
Marco Trevisan
2025-Feb-13 16:22 UTC
[PATCH 3/4] auth-pam: Add debugging information when we receive PAM messages
From: Marco Trevisan (Trevi?o) <mail at 3v1n0.net>
---
auth-pam.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/auth-pam.c b/auth-pam.c
index ba01dfb0c..932c7e1e2 100644
--- a/auth-pam.c
+++ b/auth-pam.c
@@ -446,6 +446,9 @@ sshpam_thread_conv(int n, sshpam_const struct pam_message
**msg,
break;
case PAM_ERROR_MSG:
case PAM_TEXT_INFO:
+ debug3("PAM: Got message of type %d: %s",
+ PAM_MSG_MEMBER(msg, i, msg_style),
+ PAM_MSG_MEMBER(msg, i, msg));
if ((r = sshbuf_put_cstring(buffer,
PAM_MSG_MEMBER(msg, i, msg))) != 0)
fatal("%s: buffer error: %s",
--
2.34.1
Marco Trevisan
2025-Feb-13 16:22 UTC
[PATCH 4/4] auth-pam: Immediately report interactive instructions to clients
From: Marco Trevisan (Trevi?o) <mail at 3v1n0.net>
SSH keyboard-interactive authentication method supports instructions but
sshd didn't show them until an user prompt was requested.
This is quite inconvenient for various PAM modules that need to notify
an user without requiring for their explicit input.
So, properly implement RFC4256 making instructions to be shown to users
when they are requested from PAM.
Closes: https://bugzilla.mindrot.org/show_bug.cgi?id=2876
---
auth-pam.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/auth-pam.c b/auth-pam.c
index 932c7e1e2..cbec02b39 100644
--- a/auth-pam.c
+++ b/auth-pam.c
@@ -136,6 +136,7 @@ typedef int SshPamDone;
#define SshPamError -1
#define SshPamNone 0
#define SshPamAuthenticated 1
+#define SshPamAgain 2
struct pam_ctxt {
sp_pthread_t pam_thread;
@@ -868,6 +869,8 @@ sshpam_query(void *ctx, char **name, char **info,
**prompts = NULL;
plen = 0;
*echo_on = xmalloc(sizeof(u_int));
+ ctxt->pam_done = SshPamNone;
+
while (ssh_msg_recv(ctxt->pam_psock, buffer) == 0) {
if (++nmesg > PAM_MAX_NUM_MSG)
fatal_f("too many query messages");
@@ -888,15 +891,13 @@ sshpam_query(void *ctx, char **name, char **info,
return (0);
case PAM_ERROR_MSG:
case PAM_TEXT_INFO:
- /* accumulate messages */
- len = plen + mlen + 2;
- **prompts = xreallocarray(**prompts, 1, len);
- strlcpy(**prompts + plen, msg, len - plen);
- plen += mlen;
- strlcat(**prompts + plen, "\n", len - plen);
- plen++;
- free(msg);
- break;
+ *num = 0;
+ free(*info);
+ *info = msg; /* Steal the message */
+ msg = NULL;
+ ctxt->pam_done = SshPamAgain;
+ sshbuf_free(buffer);
+ return (0);
case PAM_ACCT_EXPIRED:
case PAM_MAXTRIES:
if (type == PAM_ACCT_EXPIRED)
@@ -1001,6 +1002,8 @@ sshpam_respond(void *ctx, u_int num, char **resp)
return KbdintResultSuccess;
case SshPamNone:
break;
+ case SshPamAgain:
+ return KbdintResultAgain;
default:
return KbdintResultFailure;
}
--
2.34.1