mike-jumper commented on code in PR #572:
URL: https://github.com/apache/guacamole-server/pull/572#discussion_r1907694281
##########
src/common-ssh/ssh.c:
##########
@@ -412,6 +428,84 @@ static int
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
+/**
+ * Verifies if given algorithms are supported by libssh2 and
+ * makes the |prefs| string to use with libssh2_session_method_pref().
+ *
+ * If the verification fails, all preferred algorithms |algs| are
+ * added to the |prefs| string.
+ *
+ * @param client
+ * The Guacamole client that is using SSH.
+ *
+ * @param session
+ * The session associated with the user to be authenticated.
+ *
+ * @param method_type
+ * One of the libssh2 Method Type constants for
libssh2_session_method_pref().
+ *
+ * @param algs
+ * A preferred list of algorithms, for example FIPS_COMPLIANT_CIPHERS.
+ *
+ * @return
+ * The |prefs| string to use with libssh2_session_method_pref().
+ */
+static char *get_method_pref_string(guac_client* client, LIBSSH2_SESSION*
session,
+ int method_type, const char** algs) {
+
+ /* Initial size of the string. It will be reallocated later if not enough.
*/
+ int algs_str_size = 1024;
+ char* algs_str = guac_mem_alloc(algs_str_size);
+
+ algs_str[0] = 0;
+
+ /* Request the list of supported algorithms/cyphers from libssh2. */
+ const char **supported_algs;
+ int supported_algs_count =
+ libssh2_session_supported_algs(session, method_type, &supported_algs);
+
+ /* This is not a show stopper. */
+ if (supported_algs_count <= 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Could not verify if preferred algorithms/ciphers are supported by
libssh2");
+ }
+
+ int i = 0;
+ while (algs[i]) {
+ bool found = false;
+ /* Check if the algorithm is found in the libssh2 supported list. */
+ for (int j = 0; j < supported_algs_count; j++) {
+ if (strcmp(algs[i], supported_algs[j]) == 0) {
+ found = true;
+ break;
+ }
+ }
+ /* Add the algorithm to the string if found or
libssh2_session_supported_algs() failed. */
+ if (found || supported_algs_count <= 0) {
+ bool req_comma = algs_str[0] != 0;
+ /* Size of the algorithm name plus 1 for comma (if required) plus
1 for '\0'. */
+ int req_size = strlen(algs[i]) + (req_comma ? 1 : 0) + 1;
+ if (strlen(algs_str) + req_size > algs_str_size) {
+ /* Either 1024 to not reallocate often or required size if
bigger. */
+ algs_str_size += MAX(req_size, 1024);
+ algs_str = guac_mem_realloc(algs_str, algs_str_size);
+ }
+ /* Add comma only if necessary. */
+ if (req_comma) {
+ strcat(algs_str, ",");
+ }
+ /* Add the algorithm name. */
+ strcat(algs_str, algs[i]);
+ } else {
Review Comment:
Please don't cuddle the `else`. See:
https://cwiki.apache.org/confluence/display/GUAC/Contribution+and+Style+Guidelines#ContributionandStyleGuidelines-Braces
##########
src/common-ssh/ssh.c:
##########
@@ -412,6 +428,84 @@ static int
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
+/**
+ * Verifies if given algorithms are supported by libssh2 and
+ * makes the |prefs| string to use with libssh2_session_method_pref().
+ *
+ * If the verification fails, all preferred algorithms |algs| are
+ * added to the |prefs| string.
+ *
+ * @param client
+ * The Guacamole client that is using SSH.
+ *
+ * @param session
+ * The session associated with the user to be authenticated.
+ *
+ * @param method_type
+ * One of the libssh2 Method Type constants for
libssh2_session_method_pref().
+ *
+ * @param algs
+ * A preferred list of algorithms, for example FIPS_COMPLIANT_CIPHERS.
+ *
+ * @return
+ * The |prefs| string to use with libssh2_session_method_pref().
Review Comment:
The fact that this pointer must eventually be freed with a call to
`guac_mem_free()` should be documented.
##########
src/common-ssh/ssh.c:
##########
@@ -412,6 +428,84 @@ static int
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
+/**
+ * Verifies if given algorithms are supported by libssh2 and
+ * makes the |prefs| string to use with libssh2_session_method_pref().
+ *
+ * If the verification fails, all preferred algorithms |algs| are
+ * added to the |prefs| string.
+ *
+ * @param client
+ * The Guacamole client that is using SSH.
+ *
+ * @param session
+ * The session associated with the user to be authenticated.
+ *
+ * @param method_type
+ * One of the libssh2 Method Type constants for
libssh2_session_method_pref().
+ *
+ * @param algs
+ * A preferred list of algorithms, for example FIPS_COMPLIANT_CIPHERS.
+ *
+ * @return
+ * The |prefs| string to use with libssh2_session_method_pref().
+ */
+static char *get_method_pref_string(guac_client* client, LIBSSH2_SESSION*
session,
+ int method_type, const char** algs) {
+
+ /* Initial size of the string. It will be reallocated later if not enough.
*/
+ int algs_str_size = 1024;
+ char* algs_str = guac_mem_alloc(algs_str_size);
+
+ algs_str[0] = 0;
+
+ /* Request the list of supported algorithms/cyphers from libssh2. */
+ const char **supported_algs;
+ int supported_algs_count =
+ libssh2_session_supported_algs(session, method_type, &supported_algs);
+
+ /* This is not a show stopper. */
+ if (supported_algs_count <= 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Could not verify if preferred algorithms/ciphers are supported by
libssh2");
+ }
+
+ int i = 0;
+ while (algs[i]) {
+ bool found = false;
+ /* Check if the algorithm is found in the libssh2 supported list. */
+ for (int j = 0; j < supported_algs_count; j++) {
+ if (strcmp(algs[i], supported_algs[j]) == 0) {
+ found = true;
+ break;
+ }
+ }
+ /* Add the algorithm to the string if found or
libssh2_session_supported_algs() failed. */
+ if (found || supported_algs_count <= 0) {
+ bool req_comma = algs_str[0] != 0;
+ /* Size of the algorithm name plus 1 for comma (if required) plus
1 for '\0'. */
+ int req_size = strlen(algs[i]) + (req_comma ? 1 : 0) + 1;
+ if (strlen(algs_str) + req_size > algs_str_size) {
+ /* Either 1024 to not reallocate often or required size if
bigger. */
+ algs_str_size += MAX(req_size, 1024);
+ algs_str = guac_mem_realloc(algs_str, algs_str_size);
Review Comment:
This can fail, which will overwrite `algs_str` with `NULL` and leave a
dangling block of memory. `guac_mem_realloc_or_die()` is a better choice if
always discarding the original pointer.
##########
src/common-ssh/ssh.c:
##########
@@ -412,6 +428,84 @@ static int
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
+/**
+ * Verifies if given algorithms are supported by libssh2 and
+ * makes the |prefs| string to use with libssh2_session_method_pref().
+ *
+ * If the verification fails, all preferred algorithms |algs| are
+ * added to the |prefs| string.
+ *
+ * @param client
+ * The Guacamole client that is using SSH.
+ *
+ * @param session
+ * The session associated with the user to be authenticated.
+ *
+ * @param method_type
+ * One of the libssh2 Method Type constants for
libssh2_session_method_pref().
+ *
+ * @param algs
+ * A preferred list of algorithms, for example FIPS_COMPLIANT_CIPHERS.
+ *
+ * @return
+ * The |prefs| string to use with libssh2_session_method_pref().
+ */
+static char *get_method_pref_string(guac_client* client, LIBSSH2_SESSION*
session,
+ int method_type, const char** algs) {
+
+ /* Initial size of the string. It will be reallocated later if not enough.
*/
+ int algs_str_size = 1024;
+ char* algs_str = guac_mem_alloc(algs_str_size);
+
+ algs_str[0] = 0;
+
+ /* Request the list of supported algorithms/cyphers from libssh2. */
+ const char **supported_algs;
+ int supported_algs_count =
+ libssh2_session_supported_algs(session, method_type, &supported_algs);
+
+ /* This is not a show stopper. */
+ if (supported_algs_count <= 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Could not verify if preferred algorithms/ciphers are supported by
libssh2");
+ }
+
+ int i = 0;
+ while (algs[i]) {
+ bool found = false;
+ /* Check if the algorithm is found in the libssh2 supported list. */
+ for (int j = 0; j < supported_algs_count; j++) {
+ if (strcmp(algs[i], supported_algs[j]) == 0) {
+ found = true;
+ break;
+ }
+ }
+ /* Add the algorithm to the string if found or
libssh2_session_supported_algs() failed. */
+ if (found || supported_algs_count <= 0) {
+ bool req_comma = algs_str[0] != 0;
+ /* Size of the algorithm name plus 1 for comma (if required) plus
1 for '\0'. */
+ int req_size = strlen(algs[i]) + (req_comma ? 1 : 0) + 1;
+ if (strlen(algs_str) + req_size > algs_str_size) {
+ /* Either 1024 to not reallocate often or required size if
bigger. */
+ algs_str_size += MAX(req_size, 1024);
Review Comment:
`MAX()` is not a macro defined by C or POSIX - it's a GNU and BSD extension.
We can't rely on this macro being available.
##########
src/common-ssh/ssh.c:
##########
@@ -412,6 +428,84 @@ static int
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
+/**
+ * Verifies if given algorithms are supported by libssh2 and
+ * makes the |prefs| string to use with libssh2_session_method_pref().
+ *
+ * If the verification fails, all preferred algorithms |algs| are
+ * added to the |prefs| string.
+ *
+ * @param client
+ * The Guacamole client that is using SSH.
+ *
+ * @param session
+ * The session associated with the user to be authenticated.
+ *
+ * @param method_type
+ * One of the libssh2 Method Type constants for
libssh2_session_method_pref().
+ *
+ * @param algs
+ * A preferred list of algorithms, for example FIPS_COMPLIANT_CIPHERS.
+ *
+ * @return
+ * The |prefs| string to use with libssh2_session_method_pref().
+ */
+static char *get_method_pref_string(guac_client* client, LIBSSH2_SESSION*
session,
+ int method_type, const char** algs) {
+
+ /* Initial size of the string. It will be reallocated later if not enough.
*/
+ int algs_str_size = 1024;
+ char* algs_str = guac_mem_alloc(algs_str_size);
+
+ algs_str[0] = 0;
+
+ /* Request the list of supported algorithms/cyphers from libssh2. */
+ const char **supported_algs;
+ int supported_algs_count =
+ libssh2_session_supported_algs(session, method_type, &supported_algs);
+
+ /* This is not a show stopper. */
+ if (supported_algs_count <= 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Could not verify if preferred algorithms/ciphers are supported by
libssh2");
+ }
+
+ int i = 0;
+ while (algs[i]) {
+ bool found = false;
+ /* Check if the algorithm is found in the libssh2 supported list. */
+ for (int j = 0; j < supported_algs_count; j++) {
+ if (strcmp(algs[i], supported_algs[j]) == 0) {
+ found = true;
+ break;
+ }
+ }
+ /* Add the algorithm to the string if found or
libssh2_session_supported_algs() failed. */
+ if (found || supported_algs_count <= 0) {
+ bool req_comma = algs_str[0] != 0;
+ /* Size of the algorithm name plus 1 for comma (if required) plus
1 for '\0'. */
+ int req_size = strlen(algs[i]) + (req_comma ? 1 : 0) + 1;
Review Comment:
This could wrap around due to a large value from `strlen()` and produce a
negative value. It would be better to use `guac_mem_ckd_add_or_die()`:
https://guacamole.apache.org/doc/libguac/mem_8h.html#a57af96611f73a3f2300ba9fd64b0167a
##########
src/common-ssh/ssh.c:
##########
@@ -445,9 +539,20 @@ guac_common_ssh_session*
guac_common_ssh_create_session(guac_client* client,
*
https://csrc.nist.gov/CSRC/media/projects/cryptographic-module-validation-program/documents/security-policies/140sp2906.pdf
*/
if (guac_fips_enabled()) {
- libssh2_session_method_pref(session, LIBSSH2_METHOD_KEX,
FIPS_COMPLIANT_KEX_ALGORITHMS);
- libssh2_session_method_pref(session, LIBSSH2_METHOD_CRYPT_CS,
FIPS_COMPLIANT_CIPHERS);
- libssh2_session_method_pref(session, LIBSSH2_METHOD_CRYPT_SC,
FIPS_COMPLIANT_CIPHERS);
+ char* kex_str = get_method_pref_string(client,
+ session, LIBSSH2_METHOD_KEX, FIPS_COMPLIANT_KEX_ALGORITHMS);
+ libssh2_session_method_pref(session, LIBSSH2_METHOD_KEX, kex_str);
+ guac_mem_free(kex_str);
+
+ char* crypt_cs_str = get_method_pref_string(client,
+ session, LIBSSH2_METHOD_CRYPT_CS, FIPS_COMPLIANT_CIPHERS);
+ libssh2_session_method_pref(session, LIBSSH2_METHOD_CRYPT_CS,
crypt_cs_str);
+ guac_mem_free(crypt_cs_str);
+
+ char* crypt_sc_str = get_method_pref_string(client,
+ session, LIBSSH2_METHOD_CRYPT_SC, FIPS_COMPLIANT_CIPHERS);
+ libssh2_session_method_pref(session, LIBSSH2_METHOD_CRYPT_SC,
crypt_sc_str);
+ guac_mem_free(crypt_sc_str);
Review Comment:
According to https://libssh2.org/libssh2_session_method_pref.html, the
`libssh2_session_method_pref()` function will ignore any preferred
cipers/algorithms/etc. if not supported by libssh2:
> ... If a method is listed which is not supported by libssh2 it will be
ignored and not sent to the remote host during protocol negotiation.
So, in case this helps simplify things, it's not necessary to dynamically
assemble a string based on what libssh2 has available (though it's still
helpful to log whether something isn't supported).
##########
src/common-ssh/ssh.c:
##########
@@ -412,6 +428,84 @@ static int
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
+/**
+ * Verifies if given algorithms are supported by libssh2 and
+ * makes the |prefs| string to use with libssh2_session_method_pref().
+ *
+ * If the verification fails, all preferred algorithms |algs| are
+ * added to the |prefs| string.
+ *
+ * @param client
+ * The Guacamole client that is using SSH.
+ *
+ * @param session
+ * The session associated with the user to be authenticated.
+ *
+ * @param method_type
+ * One of the libssh2 Method Type constants for
libssh2_session_method_pref().
+ *
+ * @param algs
+ * A preferred list of algorithms, for example FIPS_COMPLIANT_CIPHERS.
+ *
+ * @return
+ * The |prefs| string to use with libssh2_session_method_pref().
+ */
+static char *get_method_pref_string(guac_client* client, LIBSSH2_SESSION*
session,
+ int method_type, const char** algs) {
+
+ /* Initial size of the string. It will be reallocated later if not enough.
*/
+ int algs_str_size = 1024;
+ char* algs_str = guac_mem_alloc(algs_str_size);
+
+ algs_str[0] = 0;
+
+ /* Request the list of supported algorithms/cyphers from libssh2. */
+ const char **supported_algs;
+ int supported_algs_count =
+ libssh2_session_supported_algs(session, method_type, &supported_algs);
+
+ /* This is not a show stopper. */
+ if (supported_algs_count <= 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Could not verify if preferred algorithms/ciphers are supported by
libssh2");
+ }
+
+ int i = 0;
+ while (algs[i]) {
Review Comment:
This loop:
```c
int i = 0;
while (algs[i]) {
...
i++;
}
```
is equivalent to:
```c
for (int i = 0; algs[i]; i++) {
...
}
```
##########
src/common-ssh/ssh.c:
##########
@@ -412,6 +428,84 @@ static int
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
+/**
+ * Verifies if given algorithms are supported by libssh2 and
+ * makes the |prefs| string to use with libssh2_session_method_pref().
+ *
+ * If the verification fails, all preferred algorithms |algs| are
+ * added to the |prefs| string.
+ *
+ * @param client
+ * The Guacamole client that is using SSH.
+ *
+ * @param session
+ * The session associated with the user to be authenticated.
+ *
+ * @param method_type
+ * One of the libssh2 Method Type constants for
libssh2_session_method_pref().
+ *
+ * @param algs
+ * A preferred list of algorithms, for example FIPS_COMPLIANT_CIPHERS.
+ *
+ * @return
+ * The |prefs| string to use with libssh2_session_method_pref().
+ */
+static char *get_method_pref_string(guac_client* client, LIBSSH2_SESSION*
session,
+ int method_type, const char** algs) {
+
+ /* Initial size of the string. It will be reallocated later if not enough.
*/
+ int algs_str_size = 1024;
+ char* algs_str = guac_mem_alloc(algs_str_size);
+
+ algs_str[0] = 0;
+
+ /* Request the list of supported algorithms/cyphers from libssh2. */
+ const char **supported_algs;
+ int supported_algs_count =
+ libssh2_session_supported_algs(session, method_type, &supported_algs);
+
+ /* This is not a show stopper. */
+ if (supported_algs_count <= 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Could not verify if preferred algorithms/ciphers are supported by
libssh2");
Review Comment:
I like this warning, but we should include wording that makes it clear to
the admin reading these logs what this means in practical terms. Will this
potentially result in the connection failing? Or in lack of information
regarding future failures? Or ... ?
##########
src/common-ssh/ssh.c:
##########
@@ -412,6 +428,84 @@ static int
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
+/**
+ * Verifies if given algorithms are supported by libssh2 and
+ * makes the |prefs| string to use with libssh2_session_method_pref().
+ *
+ * If the verification fails, all preferred algorithms |algs| are
+ * added to the |prefs| string.
+ *
+ * @param client
+ * The Guacamole client that is using SSH.
+ *
+ * @param session
+ * The session associated with the user to be authenticated.
+ *
+ * @param method_type
+ * One of the libssh2 Method Type constants for
libssh2_session_method_pref().
+ *
+ * @param algs
+ * A preferred list of algorithms, for example FIPS_COMPLIANT_CIPHERS.
+ *
+ * @return
+ * The |prefs| string to use with libssh2_session_method_pref().
+ */
+static char *get_method_pref_string(guac_client* client, LIBSSH2_SESSION*
session,
+ int method_type, const char** algs) {
+
+ /* Initial size of the string. It will be reallocated later if not enough.
*/
+ int algs_str_size = 1024;
+ char* algs_str = guac_mem_alloc(algs_str_size);
+
+ algs_str[0] = 0;
+
+ /* Request the list of supported algorithms/cyphers from libssh2. */
+ const char **supported_algs;
+ int supported_algs_count =
+ libssh2_session_supported_algs(session, method_type, &supported_algs);
+
+ /* This is not a show stopper. */
+ if (supported_algs_count <= 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Could not verify if preferred algorithms/ciphers are supported by
libssh2");
+ }
+
+ int i = 0;
+ while (algs[i]) {
+ bool found = false;
+ /* Check if the algorithm is found in the libssh2 supported list. */
+ for (int j = 0; j < supported_algs_count; j++) {
+ if (strcmp(algs[i], supported_algs[j]) == 0) {
+ found = true;
+ break;
+ }
+ }
+ /* Add the algorithm to the string if found or
libssh2_session_supported_algs() failed. */
+ if (found || supported_algs_count <= 0) {
+ bool req_comma = algs_str[0] != 0;
+ /* Size of the algorithm name plus 1 for comma (if required) plus
1 for '\0'. */
+ int req_size = strlen(algs[i]) + (req_comma ? 1 : 0) + 1;
+ if (strlen(algs_str) + req_size > algs_str_size) {
+ /* Either 1024 to not reallocate often or required size if
bigger. */
+ algs_str_size += MAX(req_size, 1024);
+ algs_str = guac_mem_realloc(algs_str, algs_str_size);
+ }
+ /* Add comma only if necessary. */
+ if (req_comma) {
+ strcat(algs_str, ",");
+ }
+ /* Add the algorithm name. */
+ strcat(algs_str, algs[i]);
Review Comment:
Each call to `strcat()` is going to search through `algs_str` to find the
end of the string. Doing this in a loop, we're going to be unnecessarily
searching through `algs_str` repeatedly.
##########
src/common-ssh/ssh.c:
##########
@@ -412,6 +428,84 @@ static int
guac_common_ssh_authenticate(guac_common_ssh_session* common_session)
}
+/**
+ * Verifies if given algorithms are supported by libssh2 and
+ * makes the |prefs| string to use with libssh2_session_method_pref().
+ *
+ * If the verification fails, all preferred algorithms |algs| are
+ * added to the |prefs| string.
+ *
+ * @param client
+ * The Guacamole client that is using SSH.
+ *
+ * @param session
+ * The session associated with the user to be authenticated.
+ *
+ * @param method_type
+ * One of the libssh2 Method Type constants for
libssh2_session_method_pref().
+ *
+ * @param algs
+ * A preferred list of algorithms, for example FIPS_COMPLIANT_CIPHERS.
+ *
+ * @return
+ * The |prefs| string to use with libssh2_session_method_pref().
+ */
+static char *get_method_pref_string(guac_client* client, LIBSSH2_SESSION*
session,
+ int method_type, const char** algs) {
+
+ /* Initial size of the string. It will be reallocated later if not enough.
*/
+ int algs_str_size = 1024;
+ char* algs_str = guac_mem_alloc(algs_str_size);
+
+ algs_str[0] = 0;
+
+ /* Request the list of supported algorithms/cyphers from libssh2. */
+ const char **supported_algs;
+ int supported_algs_count =
+ libssh2_session_supported_algs(session, method_type, &supported_algs);
+
+ /* This is not a show stopper. */
+ if (supported_algs_count <= 0) {
+ guac_client_log(client, GUAC_LOG_WARNING,
+ "Could not verify if preferred algorithms/ciphers are supported by
libssh2");
+ }
+
+ int i = 0;
+ while (algs[i]) {
+ bool found = false;
+ /* Check if the algorithm is found in the libssh2 supported list. */
+ for (int j = 0; j < supported_algs_count; j++) {
+ if (strcmp(algs[i], supported_algs[j]) == 0) {
+ found = true;
+ break;
+ }
+ }
Review Comment:
I'm wary of doing a brute-force search in a loop like this, but if we're
certain the list cannot grow to any substantial length, then this is fine.
If the list might grow to become large, we should consider sorting the list
and doing a `bsearch()`.
--
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-unsubscr...@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
