[Spice-devel,v3] Report invalid password as a special auth error

Submitted by Cedric Bosdonnat on June 1, 2015, 1:22 p.m.

Details

Message ID 1433164961-25781-1-git-send-email-cbosdonnat@suse.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Cedric Bosdonnat June 1, 2015, 1:22 p.m.
Provide a special authentication error message for too long passwords.
Also check for too long passwords before sending them over the wire.
---
 Diff to v2:
   * Make spice_channel_send_spice_ticket return TRUE / FALSE to indicate success

 gtk/spice-channel.c | 74 ++++++++++++++++++++++++++++++++++-------------------
 gtk/spice-client.h  |  2 ++
 2 files changed, 50 insertions(+), 26 deletions(-)

Patch hide | download patch | download mbox

diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
index 4e7d8b7..bd7998a 100644
--- a/gtk/spice-channel.c
+++ b/gtk/spice-channel.c
@@ -1010,7 +1010,34 @@  static int spice_channel_read(SpiceChannel *channel, void *data, size_t length)
 }
 
 /* coroutine context */
-static void spice_channel_send_spice_ticket(SpiceChannel *channel)
+static void spice_channel_failed_authentication(SpiceChannel *channel,
+                                                gboolean invalidPassword)
+{
+    SpiceChannelPrivate *c = channel->priv;
+
+    if (c->auth_needs_username_and_password)
+        g_set_error_literal(&c->error,
+                            SPICE_CLIENT_ERROR,
+                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
+                            _("Authentication failed: password and username are required"));
+    else if (invalidPassword)
+        g_set_error_literal(&c->error,
+                            SPICE_CLIENT_ERROR,
+                            SPICE_CLIENT_ERROR_AUTH_INVALID_PASSWORD,
+                            _("Authentication failed: password is too long"));
+    else
+        g_set_error_literal(&c->error,
+                            SPICE_CLIENT_ERROR,
+                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
+                            _("Authentication failed: password is required"));
+
+    c->event = SPICE_CHANNEL_ERROR_AUTH;
+
+    c->has_error = TRUE; /* force disconnect */
+}
+
+/* coroutine context */
+static gboolean spice_channel_send_spice_ticket(SpiceChannel *channel)
 {
     SpiceChannelPrivate *c = channel->priv;
     EVP_PKEY *pubkey;
@@ -1020,6 +1047,7 @@  static void spice_channel_send_spice_ticket(SpiceChannel *channel)
     char *password;
     uint8_t *encrypted;
     int rc;
+    gboolean ret = FALSE;
 
     bioKey = BIO_new(BIO_s_mem());
     g_return_if_fail(bioKey != NULL);
@@ -1039,36 +1067,24 @@  static void spice_channel_send_spice_ticket(SpiceChannel *channel)
     g_object_get(c->session, "password", &password, NULL);
     if (password == NULL)
         password = g_strdup("");
+    if (strlen(password) > SPICE_MAX_PASSWORD_LENGTH) {
+        spice_channel_failed_authentication(channel, TRUE);
+        goto cleanup;
+    }
     rc = RSA_public_encrypt(strlen(password) + 1, (uint8_t*)password,
                             encrypted, rsa, RSA_PKCS1_OAEP_PADDING);
     g_warn_if_fail(rc > 0);
 
     spice_channel_write(channel, encrypted, nRSASize);
+    ret = TRUE;
+
+cleanup:
     memset(encrypted, 0, nRSASize);
     EVP_PKEY_free(pubkey);
     BIO_free(bioKey);
     g_free(password);
-}
-
-/* coroutine context */
-static void spice_channel_failed_authentication(SpiceChannel *channel)
-{
-    SpiceChannelPrivate *c = channel->priv;
 
-    if (c->auth_needs_username_and_password)
-        g_set_error_literal(&c->error,
-                            SPICE_CLIENT_ERROR,
-                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
-                            _("Authentication failed: password and username are required"));
-    else
-        g_set_error_literal(&c->error,
-                            SPICE_CLIENT_ERROR,
-                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
-                            _("Authentication failed: password is required"));
-
-    c->event = SPICE_CHANNEL_ERROR_AUTH;
-
-    c->has_error = TRUE; /* force disconnect */
+    return ret;
 }
 
 /* coroutine context */
@@ -1086,9 +1102,13 @@  static gboolean spice_channel_recv_auth(SpiceChannel *channel)
         return FALSE;
     }
 
-    if (link_res != SPICE_LINK_ERR_OK) {
+    if (link_res == SPICE_LINK_ERR_INVALID_PASSWORD) {
+        CHANNEL_DEBUG(channel, "link result: invalid password");
+        spice_channel_failed_authentication(channel, TRUE);
+        return FALSE;
+    } if (link_res != SPICE_LINK_ERR_OK) {
         CHANNEL_DEBUG(channel, "link result: reply %d", link_res);
-        spice_channel_failed_authentication(channel);
+        spice_channel_failed_authentication(channel, FALSE);
         return FALSE;
     }
 
@@ -1662,7 +1682,7 @@  error:
     if (saslconn)
         sasl_dispose(&saslconn);
 
-    spice_channel_failed_authentication(channel);
+    spice_channel_failed_authentication(channel, FALSE);
     ret = FALSE;
 
 cleanup:
@@ -1733,7 +1753,8 @@  static gboolean spice_channel_recv_link_msg(SpiceChannel *channel)
     if (!spice_channel_test_common_capability(channel,
             SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION)) {
         CHANNEL_DEBUG(channel, "Server supports spice ticket auth only");
-        spice_channel_send_spice_ticket(channel);
+        if (!spice_channel_send_spice_ticket(channel))
+            goto error;
     } else {
         SpiceLinkAuthMechanism auth = { 0, };
 
@@ -1749,7 +1770,8 @@  static gboolean spice_channel_recv_link_msg(SpiceChannel *channel)
         if (spice_channel_test_common_capability(channel, SPICE_COMMON_CAP_AUTH_SPICE)) {
             auth.auth_mechanism = SPICE_COMMON_CAP_AUTH_SPICE;
             spice_channel_write(channel, &auth, sizeof(auth));
-            spice_channel_send_spice_ticket(channel);
+            if (!spice_channel_send_spice_ticket(channel))
+                goto error;
         } else {
             g_warning("No compatible AUTH mechanism");
             goto error;
diff --git a/gtk/spice-client.h b/gtk/spice-client.h
index c2474d1..58d1f76 100644
--- a/gtk/spice-client.h
+++ b/gtk/spice-client.h
@@ -60,6 +60,7 @@  G_BEGIN_DECLS
  * @SPICE_CLIENT_USB_DEVICE_LOST: usb device disconnected (fatal IO error)
  * @SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD: password is required
  * @SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME: password and username are required
+ * @SPICE_CLIENT_ERROR_AUTH_INVALID_PASSWORD: password is too long
  *
  * Error codes returned by spice-client API.
  */
@@ -70,6 +71,7 @@  typedef enum
     SPICE_CLIENT_USB_DEVICE_LOST,
     SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
     SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
+    SPICE_CLIENT_ERROR_AUTH_INVALID_PASSWORD,
 } SpiceClientError;
 
 GQuark spice_client_error_quark(void);

Comments

Hi

----- Original Message -----
> Provide a special authentication error message for too long passwords.
> Also check for too long passwords before sending them over the wire.
> ---
>  Diff to v2:
>    * Make spice_channel_send_spice_ticket return TRUE / FALSE to indicate
>    success
> 
>  gtk/spice-channel.c | 74
>  ++++++++++++++++++++++++++++++++++-------------------
>  gtk/spice-client.h  |  2 ++
>  2 files changed, 50 insertions(+), 26 deletions(-)
> 
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 4e7d8b7..bd7998a 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -1010,7 +1010,34 @@ static int spice_channel_read(SpiceChannel *channel,
> void *data, size_t length)
>  }
>  
>  /* coroutine context */
> -static void spice_channel_send_spice_ticket(SpiceChannel *channel)
> +static void spice_channel_failed_authentication(SpiceChannel *channel,
> +                                                gboolean invalidPassword)
> +{
> +    SpiceChannelPrivate *c = channel->priv;
> +
> +    if (c->auth_needs_username_and_password)
> +        g_set_error_literal(&c->error,
> +                            SPICE_CLIENT_ERROR,
> +
> SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
> +                            _("Authentication failed: password and username
> are required"));
> +    else if (invalidPassword)
> +        g_set_error_literal(&c->error,
> +                            SPICE_CLIENT_ERROR,
> +                            SPICE_CLIENT_ERROR_AUTH_INVALID_PASSWORD,
> +                            _("Authentication failed: password is too
> long"));
> +    else
> +        g_set_error_literal(&c->error,
> +                            SPICE_CLIENT_ERROR,
> +                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
> +                            _("Authentication failed: password is
> required"));
> +
> +    c->event = SPICE_CHANNEL_ERROR_AUTH;
> +
> +    c->has_error = TRUE; /* force disconnect */
> +}
> +
> +/* coroutine context */
> +static gboolean spice_channel_send_spice_ticket(SpiceChannel *channel)
>  {
>      SpiceChannelPrivate *c = channel->priv;
>      EVP_PKEY *pubkey;
> @@ -1020,6 +1047,7 @@ static void
> spice_channel_send_spice_ticket(SpiceChannel *channel)
>      char *password;
>      uint8_t *encrypted;
>      int rc;
> +    gboolean ret = FALSE;
>  
>      bioKey = BIO_new(BIO_s_mem());
>      g_return_if_fail(bioKey != NULL);
> @@ -1039,36 +1067,24 @@ static void
> spice_channel_send_spice_ticket(SpiceChannel *channel)
>      g_object_get(c->session, "password", &password, NULL);
>      if (password == NULL)
>          password = g_strdup("");
> +    if (strlen(password) > SPICE_MAX_PASSWORD_LENGTH) {
> +        spice_channel_failed_authentication(channel, TRUE);
> +        goto cleanup;
> +    }
>      rc = RSA_public_encrypt(strlen(password) + 1, (uint8_t*)password,
>                              encrypted, rsa, RSA_PKCS1_OAEP_PADDING);
>      g_warn_if_fail(rc > 0);
>  
>      spice_channel_write(channel, encrypted, nRSASize);
> +    ret = TRUE;
> +
> +cleanup:
>      memset(encrypted, 0, nRSASize);
>      EVP_PKEY_free(pubkey);
>      BIO_free(bioKey);
>      g_free(password);
> -}
> -
> -/* coroutine context */
> -static void spice_channel_failed_authentication(SpiceChannel *channel)
> -{
> -    SpiceChannelPrivate *c = channel->priv;
>  
> -    if (c->auth_needs_username_and_password)
> -        g_set_error_literal(&c->error,
> -                            SPICE_CLIENT_ERROR,
> -
> SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
> -                            _("Authentication failed: password and username
> are required"));
> -    else
> -        g_set_error_literal(&c->error,
> -                            SPICE_CLIENT_ERROR,
> -                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
> -                            _("Authentication failed: password is
> required"));
> -
> -    c->event = SPICE_CHANNEL_ERROR_AUTH;
> -
> -    c->has_error = TRUE; /* force disconnect */
> +    return ret;
>  }
>  
>  /* coroutine context */
> @@ -1086,9 +1102,13 @@ static gboolean spice_channel_recv_auth(SpiceChannel
> *channel)
>          return FALSE;
>      }
>  
> -    if (link_res != SPICE_LINK_ERR_OK) {
> +    if (link_res == SPICE_LINK_ERR_INVALID_PASSWORD) {
> +        CHANNEL_DEBUG(channel, "link result: invalid password");
> +        spice_channel_failed_authentication(channel, TRUE);
> +        return FALSE;
> +    } if (link_res != SPICE_LINK_ERR_OK) {
>          CHANNEL_DEBUG(channel, "link result: reply %d", link_res);
> -        spice_channel_failed_authentication(channel);
> +        spice_channel_failed_authentication(channel, FALSE);
>          return FALSE;
>      }
>  
> @@ -1662,7 +1682,7 @@ error:
>      if (saslconn)
>          sasl_dispose(&saslconn);
>  
> -    spice_channel_failed_authentication(channel);
> +    spice_channel_failed_authentication(channel, FALSE);
>      ret = FALSE;
>  
>  cleanup:
> @@ -1733,7 +1753,8 @@ static gboolean
> spice_channel_recv_link_msg(SpiceChannel *channel)
>      if (!spice_channel_test_common_capability(channel,
>              SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION)) {
>          CHANNEL_DEBUG(channel, "Server supports spice ticket auth only");
> -        spice_channel_send_spice_ticket(channel);
> +        if (!spice_channel_send_spice_ticket(channel))
> +            goto error;
>      } else {
>          SpiceLinkAuthMechanism auth = { 0, };
>  
> @@ -1749,7 +1770,8 @@ static gboolean
> spice_channel_recv_link_msg(SpiceChannel *channel)
>          if (spice_channel_test_common_capability(channel,
>          SPICE_COMMON_CAP_AUTH_SPICE)) {
>              auth.auth_mechanism = SPICE_COMMON_CAP_AUTH_SPICE;
>              spice_channel_write(channel, &auth, sizeof(auth));
> -            spice_channel_send_spice_ticket(channel);
> +            if (!spice_channel_send_spice_ticket(channel))
> +                goto error;
>          } else {
>              g_warning("No compatible AUTH mechanism");
>              goto error;
> diff --git a/gtk/spice-client.h b/gtk/spice-client.h
> index c2474d1..58d1f76 100644
> --- a/gtk/spice-client.h
> +++ b/gtk/spice-client.h
> @@ -60,6 +60,7 @@ G_BEGIN_DECLS
>   * @SPICE_CLIENT_USB_DEVICE_LOST: usb device disconnected (fatal IO error)
>   * @SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD: password is required
>   * @SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME: password and
>   username are required
> + * @SPICE_CLIENT_ERROR_AUTH_INVALID_PASSWORD: password is too long
>   *
>   * Error codes returned by spice-client API.
>   */
> @@ -70,6 +71,7 @@ typedef enum
>      SPICE_CLIENT_USB_DEVICE_LOST,
>      SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
>      SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
> +    SPICE_CLIENT_ERROR_AUTH_INVALID_PASSWORD,

From a API user point-of-view, how different is INVALID_PASSWORD from the existing errors? As can be seen in the patch, if c->auth_needs_username_and_password is set, AUTH_INVALID_PASSWORD will never be used. I am not sure we need another enum value, the error message could be enough of a hint already.