[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #349: GUACAMOLE-745: Support OpenSSH private keys & ED25519

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #349:
URL: https://github.com/apache/guacamole-server/pull/349#discussion_r777832140



##########
File path: src/common-ssh/key.c
##########
@@ -33,119 +31,81 @@
 #include <openssl/pem.h>
 #include <openssl/rsa.h>
 
+#include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
 
-guac_common_ssh_key* guac_common_ssh_key_alloc(char* data, int length,
-        char* passphrase) {
-
-    guac_common_ssh_key* key;
-    BIO* key_bio;
-
-    char* public_key;
-    char* pos;
-
-    /* Create BIO for reading key from memory */
-    key_bio = BIO_new_mem_buf(data, length);
-
-    /* If RSA key, load RSA */
-    if (length > sizeof(SSH_RSA_KEY_HEADER)-1
-            && memcmp(SSH_RSA_KEY_HEADER, data,
-                      sizeof(SSH_RSA_KEY_HEADER)-1) == 0) {
-
-        RSA* rsa_key;
-
-        const BIGNUM* key_e;
-        const BIGNUM* key_n;
-
-        /* Read key */
-        rsa_key = PEM_read_bio_RSAPrivateKey(key_bio, NULL, NULL, passphrase);
-        if (rsa_key == NULL)
-            return NULL;
-
-        /* Allocate key */
-        key = malloc(sizeof(guac_common_ssh_key));
-        key->rsa = rsa_key;
-
-        /* Set type */
-        key->type = SSH_KEY_RSA;
-
-        /* Allocate space for public key */
-        public_key = malloc(4096);
-        pos = public_key;
-
-        /* Retrieve public key */
-        RSA_get0_key(rsa_key, &key_n, &key_e, NULL);
-
-        /* Send public key formatted for SSH */
-        guac_common_ssh_buffer_write_string(&pos, "ssh-rsa", sizeof("ssh-rsa")-1);
-        guac_common_ssh_buffer_write_bignum(&pos, key_e);
-        guac_common_ssh_buffer_write_bignum(&pos, key_n);
-
-        /* Save public key to structure */
-        key->public_key = public_key;
-        key->public_key_length = pos - public_key;
+/* Check for a PKCS#1/PKCS#8 ENCRYPTED marker. */
+bool is_pkcs_encrypted_key(char* data, int length) {
+    return strstr(data, "ENCRYPTED") != NULL;
+}

Review comment:
       The same problem will occur with `memmem()`, I expect, as that's not a standard function, either. In either case, there would need to be a fallback implementation for platforms that lack the function.
   
   Implementing a `guac_strnstr()` within libguac could be a possibility. We do similar with `guac_strlcpy()`, which provides its own internal implementation of `strlcpy()` if the platform lacks the function:
   
   https://github.com/apache/guacamole-server/blob/084ddaf81f5c29d6cf8871fffe4e8353b10cfca8/src/libguac/string.c#L45-L71
   
   If we _know_ that the input will always be null-terminated, the simplest solution is probably to modify `guac_common_ssh_key_alloc()` to require null-terminated inputs, remove the `length` parameter from there, and thus allow the call to `strstr()` to be presumed safe.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org