diff --git a/README.md b/README.md index e1c13dc..5ef2da5 100644 --- a/README.md +++ b/README.md @@ -22,6 +22,8 @@ SHA256:Q4DyBOwcoyYXWccVsVNXKusPE2Rk3SKDC4w3z7z4PXI (DSA) SHA256:L1fVseOYAAAyufsAskcWeKDgAlE4KJ9uCIIly0RXk3I (ECDSA) SHA256:68fgeoCzfSPPTLbSW272DnLYER2Z9S3w8h2GDEcjg7c (ED25519) SHA256:MTg+iT5vx0JojrBuOTrN4/MdDv0kL390byaC5iPHymE (RSA) +# you should mention that a possibly already running `sshd` needs to be +# temporarily disabled to do this > sudo ssh-pairing-server (no output yet) ``` diff --git a/ssh-pairing-server.c b/ssh-pairing-server.c index 8551fa3..a53296f 100644 --- a/ssh-pairing-server.c +++ b/ssh-pairing-server.c @@ -13,16 +13,45 @@ #define MAX_KEY_COUNT 10 +/* + * processing argv and providing a minimal usage / --help message describing + * what this program does would be helpful for any production level tool. + */ int main(int argc, char *argv[]) { ssh_bind bind = ssh_bind_new(); // If a file doesn't exist it's ignored. If no host keys are available, // ssh_bind_listen will fail immediately with a helpful error. + /* + * this mimics the current default host key settings for sshd. + * + * if these should ever change then we get an inconsistency. would it + * be possible to query these paths from libssh? + * + * furthermore, what if in sshd_config different settings are found? I + * understand this tool is focused on setting up new installs, but + * even custom images could have custom sshd settings, in which case + * this might produce bad results. + */ ssh_bind_options_set(bind, SSH_BIND_OPTIONS_HOSTKEY, "/etc/ssh/ssh_host_ecdsa_key"); ssh_bind_options_set(bind, SSH_BIND_OPTIONS_HOSTKEY, "/etc/ssh/ssh_host_ed25519_key"); ssh_bind_options_set(bind, SSH_BIND_OPTIONS_HOSTKEY, "/etc/ssh/ssh_host_rsa_key"); + /* + * the default settings from libssh result in sshd listening on IPv4 + * only on the wildcard address 0.0.0.0. + * + * custom settings in sshd_config are for some reason not honored + * (tested on Tumbleweed with sshd_config in + * /usr/etc/ssh/sshd_config). + * + * also why does it listen only on IPv4, not on IPv6? + * + * even in new image installations there might be different network + * security domains, so listening on all interfaces might be + * undesirable. + */ if (ssh_bind_listen(bind) < 0) { fprintf(stderr, "Failed to listen: %s\n", ssh_get_error(bind)); return 1; @@ -37,6 +66,16 @@ int main(int argc, char *argv[]) // Client identifier char clientname[INET6_ADDRSTRLEN]; + /* + * what is the purpose of this "ssh-pairing" default value of + * `clientname`. I find this spot confusing: + * + * - it's not really a `clientname`, it's a client's IP address. + * - INET6_ADDRSTRLEN is the maximum string length for an IPv6 + * numerical address, but you are copying an arbitrary string label + * into it. As luck has it, it fits in there, but semantically it + * doesn't really make any sense. + */ strlcpy(clientname, "ssh-pairing", sizeof(clientname)); // Get the client IP if possible, avoid a potentially slow reverse lookup. @@ -59,6 +98,12 @@ int main(int argc, char *argv[]) // the callback-based pubkey handling is not possible: // https://gitlab.com/libssh/libssh-mirror/-/issues/146 + /* + * I don't really get why the INTERACTIVE auth method is offered in + * the first place. It looks like the only purpose is to send the + * "Received %d public keys" message to the connecting client. This + * should be documented better. + */ ssh_set_auth_methods(session, SSH_AUTH_METHOD_PUBLICKEY | SSH_AUTH_METHOD_INTERACTIVE); char *authorized_keys = strdup(""); @@ -66,6 +111,8 @@ int main(int argc, char *argv[]) ssh_message message; while ((message = ssh_message_get(session))) { + /* I would move the processing done in the while loop into a + * separate function to get more digestible code portions */ int msg_type = ssh_message_type(message), msg_subtype = ssh_message_subtype(message); @@ -75,6 +122,25 @@ int main(int argc, char *argv[]) if(ssh_pki_export_pubkey_base64(pubkey, &key_fp) == 0) { const char *key_type = ssh_key_type_to_char(ssh_key_type(pubkey)); char *new_authorized_keys; + /* + * displaying IP addresses could give a false + * sense of security. On this level the IP + * address / the message is not verified in + * any way, except if there would be a client + * verification, but the client is not + * authenticated, it just presents its random + * public keys to the server. + * + * a network level attacker could spoof IP + * addresses and this way the output might + * look authentic, although it really isn't. + */ + /* + * instead of concatenating all keys into one + * big `authorized_keys` string I would simply + * format and print each key as it is + * encountered. + **/ if (asprintf(&new_authorized_keys, "%s%s %s %s@%s\n", authorized_keys, key_type, key_fp, ssh_message_auth_user(message), clientname) > 0) { free(authorized_keys); @@ -85,6 +151,34 @@ int main(int argc, char *argv[]) } } else if (msg_type == SSH_REQUEST_AUTH && msg_subtype == SSH_AUTH_METHOD_INTERACTIVE) { char *msg = NULL; + /* + * I nearly overlooked that this message goes out to + * the connecting client, not to stdout. + * + * Providing any information to a yet unauthenticated + * client can quickly become problematic. It looks + * like we are only reporting back what the client + * already knows: the amount of public keys it sent to + * us, the username it used and the IP address it + * uses. + * + * At least the latter (the IP address) could be a + * small information leak in some setups where some + * form of proxy / forwarding / NAT whatever is used. + * In this case the IP address would leak some network + * topology detail to unauthenticated clients. + * + * I'm not sure if there is much value in sending this + * out to the unauthenticated client. The user that + * rightfully uses this tool has two interaction + * points: The server side and the client side. It + * could be more clean and more secure to restrict + * any kind of informational output messages to the + * server side. + * + * Then you could also drop this INTERACTIVE auth path + * and make the code simpler. + */ if (asprintf(&msg, "Received %d public keys from %s@%s", keycount, ssh_message_auth_user(message), clientname) > 0) { ssh_message_auth_interactive_request(message, msg, "", 0, NULL, 0); free(msg);