mike-jumper commented on a change in pull request #228:
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r490482972
##########
File path: src/libguac/client.c
##########
@@ -633,6 +668,40 @@ static void* __webp_support_callback(guac_user* user,
void* data) {
}
#endif
+/**
+ * A callback function which is invoked by
guac_client_owner_supports_required()
+ * to determine if the owner of a client supports the "required" instruction,
+ * updating the flag to indicate support.
+ *
+ * @param user
+ * The guac_user that will be checked for "required" instruction support.
+ *
+ * @param data
+ * A pointer to an int containing the status for support of the "required"
+ * instruction. This will be 0 if the owner does not support this
+ * instruction, or 1 if the owner does support it.
+ *
+ * @return
+ * Always NULL.
+ */
+static void* guac_owner_supports_required_callback(guac_user* user, void*
data) {
+
+ data = (void *) ((intptr_t) guac_user_supports_required(user));
Review comment:
This effectively discards the result of `guac_user_supports_required()`.
I think you mean something like:
```c
*((int*) data) = (int) ((intptr_t) guac_user_supports_required(user));
```
That said, `guac_client_for_owner()` provides a mechanism for returning a
value from the callback which is being leveraged by
`guac_client_owner_send_required()` above. Why not do the same here?
##########
File path: src/protocols/vnc/vnc.h
##########
@@ -45,6 +45,16 @@
#include <pthread.h>
+/**
+ * A flag for tracking the status of requesting username from client.
+ */
+#define GUAC_VNC_COND_FLAG_USERNAME 1
+
+/**
+ * A flag for tracking the status of requesting password from client.
+ */
+#define GUAC_VNC_COND_FLAG_PASSWORD 2
+
Review comment:
Are these flags still used?
##########
File path: src/libguac/client.c
##########
@@ -633,6 +668,40 @@ static void* __webp_support_callback(guac_user* user,
void* data) {
}
#endif
+/**
+ * A callback function which is invoked by
guac_client_owner_supports_required()
+ * to determine if the owner of a client supports the "required" instruction,
+ * updating the flag to indicate support.
+ *
+ * @param user
+ * The guac_user that will be checked for "required" instruction support.
+ *
+ * @param data
+ * A pointer to an int containing the status for support of the "required"
+ * instruction. This will be 0 if the owner does not support this
+ * instruction, or 1 if the owner does support it.
+ *
+ * @return
+ * Always NULL.
+ */
+static void* guac_owner_supports_required_callback(guac_user* user, void*
data) {
+
+ data = (void *) ((intptr_t) guac_user_supports_required(user));
Review comment:
Same here - beware that `user` can be `NULL` if the owner has left the
connection.
##########
File path: src/libguac/client.c
##########
@@ -478,6 +478,41 @@ int guac_client_load_plugin(guac_client* client, const
char* protocol) {
}
+/**
+ * A callback function which is invoked by guac_client_owner_send_required() to
+ * send the required parameters to the specified user, who is the owner of the
+ * client session.
+ *
+ * @param user
+ * The guac_user that will receive the required parameters, who is the
owner
+ * of the client.
+ *
+ * @param data
+ * A pointer to a NULL-terminated array of required parameters that will be
+ * passed on to the owner to continue the connection.
+ *
+ * @return
+ * The integer status of the operation, cast as void*.
+ */
+static void* guac_client_owner_send_required_callback(guac_user* user, void*
data) {
+
+ const char** required = (const char **) data;
+
+ /* Send required parameters to owner. */
+ return (void*) ((intptr_t) guac_protocol_send_required(user->socket,
required));
Review comment:
Beware that `user` can be `NULL` if the owner has left the connection.
See:
https://guacamole.apache.org/doc/1.1.0/libguac/client_8h.html#af3f4ed85d98b16376e2cdc031ff1b44a
##########
File path: src/protocols/vnc/user.c
##########
@@ -98,6 +99,9 @@ int guac_vnc_user_join_handler(guac_user* user, int argc,
char** argv) {
/* Inbound (client to server) clipboard transfer */
if (!settings->disable_paste)
user->clipboard_handler = guac_vnc_clipboard_handler;
+
+ /* Updates to connection parameters */
+ user->argv_handler = guac_argv_handler;
Review comment:
Same here - I believe this should be set only for the connection owner.
##########
File path: src/protocols/rdp/user.c
##########
@@ -116,6 +117,9 @@ int guac_rdp_user_join_handler(guac_user* user, int argc,
char** argv) {
/* Inbound arbitrary named pipes */
user->pipe_handler = guac_rdp_pipe_svc_pipe_handler;
+
+ /* Handler for updating parameters during connection. */
+ user->argv_handler = guac_argv_handler;
Review comment:
I believe this should be set only for the connection owner, as only the
owner will receive the "required" instruction and only the owner is intended to
be allowed to respond.
##########
File path: src/protocols/rdp/rdp.h
##########
@@ -49,6 +49,21 @@
#include <pthread.h>
#include <stdint.h>
+/**
+ * A flag for tracking if we are waiting conditionally on a username.
+ */
+#define GUAC_RDP_CRED_FLAG_USERNAME 1
+
+/**
+ * A flag for tracking if we are waiting conditionally on a password.
+ */
+#define GUAC_RDP_CRED_FLAG_PASSWORD 2
+
+/**
+ * A flag for tracking if we are waiting conditionally on a domain.
+ */
+#define GUAC_RDP_CRED_FLAG_DOMAIN 4
+
Review comment:
Same here - still used?
##########
File path: src/protocols/rdp/rdp.c
##########
@@ -227,10 +234,60 @@ static BOOL rdp_freerdp_authenticate(freerdp* instance,
char** username,
rdpContext* context = instance->context;
guac_client* client = ((rdp_freerdp_context*) context)->client;
+ guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
+ guac_rdp_settings* settings = rdp_client->settings;
+ char* params[4] = {};
+ int i = 0;
+
+ /* If the client does not support the "required" instruction, warn and
+ * quit.
+ */
+ if (!guac_client_owner_supports_required(client)) {
+ guac_client_log(client, GUAC_LOG_WARNING, "Client does not support the
"
+ "\"required\" instruction. No authentication parameters will "
+ "be requested.");
+ return TRUE;
+ }
- /* Warn if connection is likely to fail due to lack of credentials */
- guac_client_log(client, GUAC_LOG_INFO,
- "Authentication requested but username or password not given");
+ /* If the username is undefined, add it to the requested parameters. */
+ if (settings->username == NULL) {
+ guac_argv_register(GUAC_RDP_ARGV_USERNAME, guac_rdp_argv_callback,
NULL, 0);
+ params[i] = GUAC_RDP_ARGV_USERNAME;
+ i++;
+ }
+
+ /* If the password is undefined, add it to the requested parameters. */
+ if (settings->password == NULL) {
+ guac_argv_register(GUAC_RDP_ARGV_PASSWORD, guac_rdp_argv_callback,
NULL, 0);
+ params[i] = GUAC_RDP_ARGV_PASSWORD;
+ i++;
+ }
+
+ /* If the domain is undefined, add it to the requested parameters. */
+ if (settings->domain == NULL) {
+ guac_argv_register(GUAC_RDP_ARGV_DOMAIN, guac_rdp_argv_callback, NULL,
0);
+ params[i] = GUAC_RDP_ARGV_DOMAIN;
+ i++;
+ }
+
+ /* NULL-terminate the array. */
+ params[i] = NULL;
+
+ if (i > 0) {
+
+ /* Send required parameters to the owner. */
+ guac_client_owner_send_required(client, (const char**) params);
+
+ guac_argv_await((const char**) params);
+
+ /* Get new values from settings. */
+ *username = settings->username;
+ *password = settings->password;
+ *domain = settings->domain;
Review comment:
There may already be pointers to allocated memory assigned to
`*username`, `*password`, etc. which will orphaned through these assignments.
The existing values should first be freed.
As FreeRDP will also automatically free the assigned strings, and we already
free our copies of these strings stored within `settings`, these should be
duplicated so that they aren't double-freed. This is how things are currently
done within `guac_rdp_push_settings()`:
https://github.com/apache/guacamole-server/blob/382d72a26a04008aa936680deed00e3a31086b1e/src/protocols/rdp/settings.c#L1296-L1299
##########
File path: src/protocols/vnc/auth.c
##########
@@ -19,27 +19,96 @@
#include "config.h"
+#include "argv.h"
#include "auth.h"
#include "vnc.h"
+#include <guacamole/argv.h>
#include <guacamole/client.h>
+#include <guacamole/protocol.h>
+#include <guacamole/socket.h>
#include <rfb/rfbclient.h>
#include <rfb/rfbproto.h>
+#include <pthread.h>
+#include <string.h>
+
char* guac_vnc_get_password(rfbClient* client) {
guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
- return ((guac_vnc_client*) gc->data)->settings->password;
+ guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+ guac_vnc_settings* settings = vnc_client->settings;
+
+ /* If the client does not support the "required" instruction, just return
+ the configuration data. */
+ if (!guac_client_owner_supports_required(gc))
+ return ((guac_vnc_client*) gc->data)->settings->password;
+
+ /* If password isn't around, prompt for it. */
+ if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+
+ guac_argv_register(GUAC_VNC_ARGV_PASSWORD, guac_vnc_argv_callback,
NULL, 0);
+
+ const char* params[] = {GUAC_VNC_ARGV_PASSWORD, NULL};
+
+ /* Send the request for password to the owner. */
+ guac_client_owner_send_required(gc, params);
+
+ /* Wait for the arguments to be returned */
+ guac_argv_await(params);
+
+ }
+
+ return settings->password;
+
}
rfbCredential* guac_vnc_get_credentials(rfbClient* client, int credentialType)
{
guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
- guac_vnc_settings* settings = ((guac_vnc_client*) gc->data)->settings;
+ guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+ guac_vnc_settings* settings = vnc_client->settings;
+ /* Handle request for Username/Password credentials */
if (credentialType == rfbCredentialTypeUser) {
rfbCredential *creds = malloc(sizeof(rfbCredential));
- creds->userCredential.username = settings->username;
- creds->userCredential.password = settings->password;
- return creds;
+
+ /* If the client does not support the "required" instruction, just
return
+ the parameters already configured. */
+ if (!guac_client_owner_supports_required(gc)) {
+ creds->userCredential.username = settings->username;
+ creds->userCredential.password = settings->password;
+ return creds;
+ }
+
+ char* params[2] = {NULL};
+ int i = 0;
+
+ /* Check if username is null or empty. */
+ if (settings->username == NULL) {
+ guac_argv_register(GUAC_VNC_ARGV_USERNAME, guac_vnc_argv_callback,
NULL, 0);
+ params[i] = GUAC_VNC_ARGV_USERNAME;
+ i++;
+ }
+
+ /* Check if password is null or empty. */
+ if (settings->password == NULL) {
+ guac_argv_register(GUAC_VNC_ARGV_PASSWORD, guac_vnc_argv_callback,
NULL, 0);
+ params[i] = GUAC_VNC_ARGV_PASSWORD;
+ i++;
+ }
+
+ /* If we have empty parameters, request them. */
+ if (i > 0) {
+
+ /* Send required parameters to owner. */
+ guac_client_owner_send_required(gc, (const char**) params);
Review comment:
`guac_client_owner_send_required()` requires a null-terminated array.
This will be the case here if only one parameter is requested, but because the
length of `params` is 2, it cannot be the case if two parameters are requested,
and the function may overrun array bounds.
I think your `params` array needs to be one element longer to ensure there
is room for the NULL terminator.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
