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

[Alon Levy]

On Sun, May 08, 2011 at 04:44:55PM +0300, Yaniv Kaul wrote:
> 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.
> 
ok. Like you say this is not relevant to this patch, but I'll look, thanks.

> (of course, the patch above did not introduce this issue)
> Y.
> 
> >          }
> >-
> >-        memset(bufEncrypted.get(), 0, nRSASize);
> >      } else {
> >          THROW("Could not initiate BIO");
> >      }
>