From 824d5b3ef785766d2997b2f359da63c2d6639e85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 17 Oct 2023 04:04:13 +0200 Subject: [PATCH 1/6] auth: Add KbdintResult definition to define result values explicitly 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 d124e994e77..ca41735debb 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 13c0a792e99..5dfa69202d0 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 98bb23d4c5c..aba6e775ddd 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 021df829173..047d4e83c33 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; From fd2814626adadf1335a6983455679ea50b874e3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 16 Oct 2023 21:15:45 +0200 Subject: [PATCH 2/6] auth-pam: Add an enum to define the PAM done status 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 5dfa69202d0..ba01dfb0c0e 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; From f36415d61b6b8b547152940f29228f8a55c55dec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 17 Oct 2023 04:35:17 +0200 Subject: [PATCH 3/6] auth-pam: Add debugging information when we receive PAM messages --- auth-pam.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/auth-pam.c b/auth-pam.c index ba01dfb0c0e..932c7e1e257 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", From d2e0c0560484d3712d7cf739dc678db5a2b89dd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 17 Oct 2023 04:27:32 +0200 Subject: [PATCH 4/6] auth-pam: Immediately report interactive instructions to clients 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 932c7e1e257..cbec02b3997 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; } From 19972045b460fff2d503cc3641a350d9328bf139 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 17 Oct 2023 06:12:03 +0200 Subject: [PATCH 5/6] sshconnect2: Write kbd-interactive service, info and instructions as utf-8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As per the previous server change now the keyboard-interactive service and instruction values could be reported as soon as they are available and so they're not prompts anymore and not parsed like them. While this was already supported by the SSH client, these messages were not properly written as the escaped sequences they contained were not correctly reported. So for example a message containing "\" was represented as "\\" and similarly for all the other C escape sequences. This was leading to more problems when it come to utf-8 chars, as they were only represented by their octal representation. This was easily testable by adding a line like the one below to the sshd PAM service: auth requisite pam_echo.so Hello SSHD! Want some 🍕? Which was causing this to be written instead: Hello SSHD! Want some \360\237\215\225? To handle this, instead of simply using fmprintf, we're using the notifier in a way can be exposed to users in the proper format and UI. --- sshconnect2.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/sshconnect2.c b/sshconnect2.c index 11fcdea8aff..95bdee3d449 100644 --- a/sshconnect2.c +++ b/sshconnect2.c @@ -1075,6 +1075,7 @@ input_userauth_passwd_changereq(int type, u_int32_t seqnr, struct ssh *ssh) char *info = NULL, *lang = NULL, *password = NULL, *retype = NULL; char prompt[256]; const char *host; + size_t info_len; int r; debug2("input_userauth_passwd_changereq"); @@ -1084,11 +1085,15 @@ input_userauth_passwd_changereq(int type, u_int32_t seqnr, struct ssh *ssh) "no authentication context"); host = options.host_key_alias ? options.host_key_alias : authctxt->host; - if ((r = sshpkt_get_cstring(ssh, &info, NULL)) != 0 || + if ((r = sshpkt_get_cstring(ssh, &info, &info_len)) != 0 || (r = sshpkt_get_cstring(ssh, &lang, NULL)) != 0) goto out; - if (strlen(info) > 0) - logit("%s", info); + if (info_len > 0) { + struct notifier_ctx *notifier = NULL; + debug_f("input_userauth_passwd_changereq info: %s", info); + notifier = notify_start(0, "%s", info); + notify_complete(notifier, NULL); + } if ((r = sshpkt_start(ssh, SSH2_MSG_USERAUTH_REQUEST)) != 0 || (r = sshpkt_put_cstring(ssh, authctxt->server_user)) != 0 || (r = sshpkt_put_cstring(ssh, authctxt->service)) != 0 || @@ -1942,8 +1947,10 @@ input_userauth_info_req(int type, u_int32_t seq, struct ssh *ssh) Authctxt *authctxt = ssh->authctxt; char *name = NULL, *inst = NULL, *lang = NULL, *prompt = NULL; char *display_prompt = NULL, *response = NULL; + struct notifier_ctx *notifier = NULL; u_char echo = 0; u_int num_prompts, i; + size_t name_len, inst_len; int r; debug2_f("entering"); @@ -1953,14 +1960,22 @@ input_userauth_info_req(int type, u_int32_t seq, struct ssh *ssh) authctxt->info_req_seen = 1; - if ((r = sshpkt_get_cstring(ssh, &name, NULL)) != 0 || - (r = sshpkt_get_cstring(ssh, &inst, NULL)) != 0 || + if ((r = sshpkt_get_cstring(ssh, &name, &name_len)) != 0 || + (r = sshpkt_get_cstring(ssh, &inst, &inst_len)) != 0 || (r = sshpkt_get_cstring(ssh, &lang, NULL)) != 0) goto out; - if (strlen(name) > 0) - logit("%s", name); - if (strlen(inst) > 0) - logit("%s", inst); + if (name_len > 0) { + debug_f("kbd int name: %s", name); + notifier = notify_start(0, "%s", name); + notify_complete(notifier, NULL); + notifier = NULL; + } + if (inst_len > 0) { + debug_f("kbd int inst: %s", inst); + notifier = notify_start(0, "%s", inst); + notify_complete(notifier, NULL); + notifier = NULL; + } if ((r = sshpkt_get_u32(ssh, &num_prompts)) != 0) goto out; From 541850b5fa02642a06ba9f961b9c863e46bd13f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 17 Oct 2023 06:05:59 +0200 Subject: [PATCH 6/6] auth2-chall: Fix selection of the keyboard-interactive device We were only checking if the prefix of a device name was matching what we had in the devices list, so if the device list contained "pam", then also the device "pam-foo" was matching. --- auth2-chall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/auth2-chall.c b/auth2-chall.c index 047d4e83c33..db658c9b4a7 100644 --- a/auth2-chall.c +++ b/auth2-chall.c @@ -170,7 +170,7 @@ kbdint_next_device(Authctxt *authctxt, KbdintAuthctxt *kbdintctxt) "keyboard-interactive", devices[i]->name)) continue; if (strncmp(kbdintctxt->devices, devices[i]->name, - len) == 0) { + len) == 0 && strlen(devices[i]->name) == len) { kbdintctxt->device = devices[i]; kbdintctxt->devices_done |= 1 << i; }