[Spice-devel] [RFCv5 46/47] (temp) client/red_channel: DEBUG: allow no SSL usage (useful for valgrind)

[Yaniv Kaul]

On 05/08/2011 04:11 PM, Alon Levy wrote:
> ---
>   client/red_channel.cpp |   40 +++++++++++++++++++++++++---------------
>   1 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/client/red_channel.cpp b/client/red_channel.cpp
> index d8dcc42..013a5a4 100644
> --- a/client/red_channel.cpp
> +++ b/client/red_channel.cpp
> @@ -173,23 +173,33 @@ void RedChannelBase::link(uint32_t connection_id, const std::string&  password,
>       if (bioKey != NULL) {
>           BIO_write(bioKey, reply->pub_key, SPICE_TICKET_PUBKEY_BYTES);
>           pubkey = d2i_PUBKEY_bio(bioKey, NULL);
> -        rsa = pubkey->pkey.rsa;
> -        nRSASize = RSA_size(rsa);
> -        AutoArray<unsigned char>  bufEncrypted(new unsigned char[nRSASize]);
> -
> -        /*
> -                The use of RSA encryption limit the potential maximum password length.
> -                for RSA_PKCS1_OAEP_PADDING it is RSA_size(rsa) - 41.
> -        */
> -        if (RSA_public_encrypt(password.length() + 1, (unsigned char *)password.c_str(),
> -                               (uint8_t *)bufEncrypted.get(),
> -                               rsa, RSA_PKCS1_OAEP_PADDING)>  0) {
> -            send((uint8_t*)bufEncrypted.get(), nRSASize);
> +        if (pubkey == NULL) {
> +#ifdef ALLOW_NO_SSL
> +            /* silly games vs valgrind */
> +            nRSASize = 128;
> +            uint8_t buf[128] = {0};
> +            send(buf, nRSASize);
> +#else
> +            THROW_ERR(SPICEC_ERROR_CODE_CONNECT_FAILED, "connect error - failed to get pubkey from server link message");
> +#endif
>           } else {
> -            THROW("could not encrypt password");
> +            rsa = pubkey->pkey.rsa;
> +            nRSASize = RSA_size(rsa);
> +            AutoArray<unsigned char>  bufEncrypted(new unsigned char[nRSASize]);
> +
> +            /*
> +                    The use of RSA encryption limit the potential maximum password length.
> +                    for RSA_PKCS1_OAEP_PADDING it is RSA_size(rsa) - 41.
> +            */
> +            if (RSA_public_encrypt(password.length() + 1, (unsigned char *)password.c_str(),
> +                                   (uint8_t *)bufEncrypted.get(),
> +                                   rsa, RSA_PKCS1_OAEP_PADDING)>  0) {
> +                send((uint8_t*)bufEncrypted.get(), nRSASize);
> +            } else {
> +                THROW("could not encrypt password");
> +            }
> +            memset(bufEncrypted.get(), 0, nRSASize);

I'd be surprised if this is actually doing what you expect it to do - 
clear sensitive data?
I suspect an optimizing compiler will just skip memset() an area which 
is never read again.
See http://msdn.microsoft.com/en-us/library/ms972826.aspx for a similar 
scenario.

(of course, the patch above did not introduce this issue)
Y.

>           }
> -
> -        memset(bufEncrypted.get(), 0, nRSASize);
>       } else {
>           THROW("Could not initiate BIO");
>       }