[openssl.org #3317] Patch: Avoid out-of-bounds write in SSL_get_shared_ciphers

[Günther Noack via RT]

Hello,

the attached patch fixes an out-of-bounds write in SSL_get_shared_ciphers.

According to Bodo Moeller, the bug should not be critical because the
function never gets called with an empty list, but it may still be nice to
have that check in place.

Without the patch, when SSL_get_shared_ciphers gets called with an empty
list of ciphers, it will skip the for-loop completely, write buf[-1]='\0',
and return a pointer to uninitialized memory.

I found this with the Clang static analyzer.  (Great tool. :))

Thanks,
Günther

PS: To illustrate,
1449 char *SSL_get_shared_ciphers(const SSL *s,char *buf,int len)
1450         {
1451         char *p;
1452         STACK_OF(SSL_CIPHER) *sk;
1453         SSL_CIPHER *c;
1454         int i;
1455
1456         if ((s->session == NULL) || (s->session->ciphers == NULL) ||
1457                 (len < 2))
1458                 return(NULL);
1459
1460         p=buf;
1461         sk=s->session->ciphers;
1462         for (i=0; i<sk_SSL_CIPHER_num(sk); i++)   // assume
sk_SSL_CIPHER_num(sk) == 0
1463                 {
1464                 int n;
1465
1466                 c=sk_SSL_CIPHER_value(sk,i);
1467                 n=strlen(c->name);
1468                 if (n+1 > len)
1469                         {
1470                         if (p != buf)
1471                                 --p;
1472                         *p='\0';
1473                         return buf;
1474                         }
1475                 strcpy(p,c->name);
1476                 p+=n;
1477                 *(p++)=':';
1478                 len-=n+1;
1479                 }
1480         p[-1]='\0';  // out of bounds write, returns pointer to
uninitialized memory
1481         return(buf);
1482         }


diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index c6ca137..4720680 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1442,40 +1442,44 @@ int SSL_set_cipher_list(SSL *s,const char *str)
                SSLerr(SSL_F_SSL_SET_CIPHER_LIST, SSL_R_NO_CIPHER_MATCH);
                return 0;
                }
        return 1;
        }
 
 /* works well for SSLv2, not so good for SSLv3 */
 char *SSL_get_shared_ciphers(const SSL *s,char *buf,int len)
        {
        char *p;
        STACK_OF(SSL_CIPHER) *sk;
        SSL_CIPHER *c;
        int i;
 
        if ((s->session == NULL) || (s->session->ciphers == NULL) ||
                (len < 2))
                return(NULL);
 
        p=buf;
        sk=s->session->ciphers;
+
+       if (sk_SSL_CIPHER_num(sk) == 0)
+               return NULL;
+
        for (i=0; i<sk_SSL_CIPHER_num(sk); i++)
                {
                int n;
 
                c=sk_SSL_CIPHER_value(sk,i);
                n=strlen(c->name);
                if (n+1 > len)
                        {
                        if (p != buf)
                                --p;
                        *p='\0';
                        return buf;
                        }
                strcpy(p,c->name);
                p+=n;
                *(p++)=':';
                len-=n+1;
                }
        p[-1]='\0';
        return(buf);