Re: [PATCH v4] Check too long password

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 03, 2015 at 04:22:35PM +0200, Cédric Bosdonnat wrote:
> Make sure that the password lenght is under the maximum lenght. If not
> report it as an authentication failure with an adapted message.
> ---
>  Diff to v3:
> 
>    * Removed the checks on the server side and the corresponding code here
>    * Removed the new error code to reuse SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD.
>    * spice_channel_send_spice_ticket returns SpiceChannelEvent instead of a gboolean:
>      this helps properly propagating the kind of error to the client.
> 
>  gtk/spice-channel.c | 80 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 51 insertions(+), 29 deletions(-)
> 
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 4e7d8b7..d01b147 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_NEEDS_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 SpiceChannelEvent spice_channel_send_spice_ticket(SpiceChannel *channel)
>  {
>      SpiceChannelPrivate *c = channel->priv;
>      EVP_PKEY *pubkey;
> @@ -1020,13 +1047,14 @@ static void spice_channel_send_spice_ticket(SpiceChannel *channel)
>      char *password;
>      uint8_t *encrypted;
>      int rc;
> +    SpiceChannelEvent ret = SPICE_CHANNEL_ERROR_LINK;
>  
>      bioKey = BIO_new(BIO_s_mem());
> -    g_return_if_fail(bioKey != NULL);
> +    g_return_val_if_fail(bioKey != NULL, ret);
>  
>      BIO_write(bioKey, c->peer_msg->pub_key, SPICE_TICKET_PUBKEY_BYTES);
>      pubkey = d2i_PUBKEY_bio(bioKey, NULL);
> -    g_return_if_fail(pubkey != NULL);
> +    g_return_val_if_fail(pubkey != NULL, ret);
>  
>      rsa = pubkey->pkey.rsa;
>      nRSASize = RSA_size(rsa);
> @@ -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);
> +        ret = SPICE_CHANNEL_ERROR_AUTH;
> +        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 = SPICE_CHANNEL_NONE;
> +
> +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 */
> @@ -1088,7 +1104,7 @@ static gboolean spice_channel_recv_auth(SpiceChannel *channel)
>  
>      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 +1678,7 @@ error:
>      if (saslconn)
>          sasl_dispose(&saslconn);
>  
> -    spice_channel_failed_authentication(channel);
> +    spice_channel_failed_authentication(channel, FALSE);
>      ret = FALSE;
>  
>  cleanup:
> @@ -1681,6 +1697,7 @@ static gboolean spice_channel_recv_link_msg(SpiceChannel *channel)
>      SpiceChannelPrivate *c;
>      int rc, num_caps, i;
>      uint32_t *caps;
> +    SpiceChannelEvent event = SPICE_CHANNEL_NONE;

I would set this to SPICE_CHANNEL_ERROR_LINK...

>  
>      g_return_val_if_fail(channel != NULL, FALSE);
>      g_return_val_if_fail(channel->priv != NULL, FALSE);
> @@ -1733,7 +1750,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 ((event = spice_channel_send_spice_ticket(channel)) != SPICE_CHANNEL_NONE)
> +            goto error;
>      } else {
>          SpiceLinkAuthMechanism auth = { 0, };
>  
> @@ -1749,7 +1767,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 ((event = spice_channel_send_spice_ticket(channel)) != SPICE_CHANNEL_NONE)
> +                goto error;
>          } else {
>              g_warning("No compatible AUTH mechanism");
>              goto error;
> @@ -1762,7 +1781,10 @@ static gboolean spice_channel_recv_link_msg(SpiceChannel *channel)
>  
>  error:
>      c->has_error = TRUE;
> -    c->event = SPICE_CHANNEL_ERROR_LINK;
> +    if (event == SPICE_CHANNEL_NONE)
> +        c->event = SPICE_CHANNEL_ERROR_LINK;
> +    else
> +        c->event = event;

... and just do c->event = event; here (with a g_warn_if_fail(event !=
SPICE_CHANNEL_NONE); if you want to be paranoid).

Looks good to me otherwise, ACK.

Christophe

Attachment: pgpioJbHxsRcq.pgp
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]