Fix missing return value checks

Ensure that all functions have their return values checked where appropriate. This covers all functions defined and called from within libssl. Reviewed-by: Richard Levitte <levitte@openssl.org>
openssl · Mar 23, 2015 · 69f6823 · 69f6823
1 parent 4bcdb4a
commit 69f6823
Show file tree

Hide file tree

Showing 24 changed files with 237 additions and 136 deletions.
diff --git a/ssl/bio_ssl.c b/ssl/bio_ssl.c
@@ -292,7 +292,10 @@ static long ssl_ctrl(BIO *b, int cmd, long num, void *ptr)
         else if (ssl->handshake_func == ssl->method->ssl_accept)
             SSL_set_accept_state(ssl);
 
-        SSL_clear(ssl);
+        if(!SSL_clear(ssl)) {
+            ret = 0;
+            break;
+        }
 
         if (b->next_bio != NULL)
             ret = BIO_ctrl(b->next_bio, cmd, num, ptr);

diff --git a/ssl/d1_both.c b/ssl/d1_both.c
@@ -989,7 +989,10 @@ int dtls1_send_change_cipher_spec(SSL *s, int a, int b)
                                      s->d1->handshake_write_seq, 0, 0);
 
         /* buffer the message to handle re-xmits */
-        dtls1_buffer_message(s, 1);
+        if(!dtls1_buffer_message(s, 1)) {
+            SSLerr(SSL_F_DTLS1_SEND_CHANGE_CIPHER_SPEC, ERR_R_INTERNAL_ERROR);
+            return -1;
+        }
 
         s->state = b;
     }
@@ -1237,7 +1240,7 @@ void dtls1_clear_record_buffer(SSL *s)
     }
 }
 
-unsigned char *dtls1_set_message_header(SSL *s, unsigned char *p,
+void dtls1_set_message_header(SSL *s, unsigned char *p,
                                         unsigned char mt, unsigned long len,
                                         unsigned long frag_off,
                                         unsigned long frag_len)
@@ -1250,8 +1253,6 @@ unsigned char *dtls1_set_message_header(SSL *s, unsigned char *p,
 
     dtls1_set_message_header_int(s, mt, len, s->d1->handshake_write_seq,
                                  frag_off, frag_len);
-
-    return p += DTLS1_HM_HEADER_LENGTH;
 }
 
 /* don't actually do the writing, wait till the MTU has been retrieved */

diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
@@ -181,8 +181,10 @@ int dtls1_connect(SSL *s)
         cb = s->ctx->info_callback;
 
     s->in_handshake++;
-    if (!SSL_in_init(s) || SSL_in_before(s))
-        SSL_clear(s);
+    if (!SSL_in_init(s) || SSL_in_before(s)) {
+        if(!SSL_clear(s))
+            return -1;
+    }
 
 #ifndef OPENSSL_NO_SCTP
     /*

diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
@@ -567,7 +567,11 @@ static void dtls1_set_handshake_header(SSL *s, int htype, unsigned long len)
     s->init_num = (int)len + DTLS1_HM_HEADER_LENGTH;
     s->init_off = 0;
     /* Buffer the message to handle re-xmits */
-    dtls1_buffer_message(s, 0);
+    /*
+     * Deliberately swallow error return. We really should do something with
+     * this - but its a void function that can't (easily) be changed
+     */
+    if(!dtls1_buffer_message(s, 0));
 }
 
 static int dtls1_handshake_write(SSL *s)

diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
@@ -937,7 +937,10 @@ int dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
         }
 #ifndef OPENSSL_NO_HEARTBEATS
         else if (rr->type == TLS1_RT_HEARTBEAT) {
-            dtls1_process_heartbeat(s);
+            /* We allow a 0 return */
+            if(dtls1_process_heartbeat(s) < 0) {
+                return -1;
+            }
 
             /* Exit and notify application to read again */
             rr->length = 0;
@@ -1246,7 +1249,8 @@ int dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
             if (dtls1_check_timeout_num(s) < 0)
                 return -1;
 
-            dtls1_retransmit_buffered_messages(s);
+            /* Ignore retransmit failures - swallow return code */
+            if(dtls1_retransmit_buffered_messages(s));
             rr->length = 0;
             goto start;
         }

diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
@@ -184,8 +184,10 @@ int dtls1_accept(SSL *s)
 
     /* init things to blank */
     s->in_handshake++;
-    if (!SSL_in_init(s) || SSL_in_before(s))
-        SSL_clear(s);
+    if (!SSL_in_init(s) || SSL_in_before(s)) {
+        if(!SSL_clear(s))
+            return -1;
+    }
 
     s->d1->listen = listen;
 #ifndef OPENSSL_NO_SCTP

diff --git a/ssl/s23_clnt.c b/ssl/s23_clnt.c
@@ -157,8 +157,10 @@ int ssl23_connect(SSL *s)
         cb = s->ctx->info_callback;
 
     s->in_handshake++;
-    if (!SSL_in_init(s) || SSL_in_before(s))
-        SSL_clear(s);
+    if (!SSL_in_init(s) || SSL_in_before(s)) {
+        if(!SSL_clear(s))
+            return -1;
+    }
 
     for (;;) {
         state = s->state;

diff --git a/ssl/s23_srvr.c b/ssl/s23_srvr.c
@@ -156,8 +156,10 @@ int ssl23_accept(SSL *s)
         cb = s->ctx->info_callback;
 
     s->in_handshake++;
-    if (!SSL_in_init(s) || SSL_in_before(s))
-        SSL_clear(s);
+    if (!SSL_in_init(s) || SSL_in_before(s)) {
+        if(!SSL_clear(s))
+            return -1;
+    }
 
     for (;;) {
         state = s->state;

diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
@@ -197,8 +197,10 @@ int ssl3_connect(SSL *s)
         cb = s->ctx->info_callback;
 
     s->in_handshake++;
-    if (!SSL_in_init(s) || SSL_in_before(s))
-        SSL_clear(s);
+    if (!SSL_in_init(s) || SSL_in_before(s)) {
+        if(!SSL_clear(s))
+            return -1;
+    }
 
 #ifndef OPENSSL_NO_HEARTBEATS
     /*
@@ -3044,6 +3046,11 @@ int ssl3_send_client_key_exchange(SSL *s)
         OPENSSL_cleanse(pms, pmslen);
         OPENSSL_free(pms);
         s->cert->pms = NULL;
+        if(s->session->master_key_length < 0) {
+            ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
+            SSLerr(SSL_F_SSL3_SEND_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+            goto err;
+        }
     }
     return n;
  memerr:

diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c
@@ -253,7 +253,10 @@ int ssl3_change_cipher_state(SSL *s, int which)
             EVP_CIPHER_CTX_init(s->enc_read_ctx);
         dd = s->enc_read_ctx;
 
-        ssl_replace_hash(&s->read_hash, m);
+        if(!ssl_replace_hash(&s->read_hash, m)) {
+                SSLerr(SSL_F_SSL3_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);
+                goto err2;
+        }
 #ifndef OPENSSL_NO_COMP
         /* COMPRESS */
         if (s->expand != NULL) {
@@ -288,7 +291,10 @@ int ssl3_change_cipher_state(SSL *s, int which)
              */
             EVP_CIPHER_CTX_init(s->enc_write_ctx);
         dd = s->enc_write_ctx;
-        ssl_replace_hash(&s->write_hash, m);
+        if(!ssl_replace_hash(&s->write_hash, m)) {
+                SSLerr(SSL_F_SSL3_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);
+                goto err2;
+        }
 #ifndef OPENSSL_NO_COMP
         /* COMPRESS */
         if (s->compress != NULL) {

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
@@ -3114,7 +3114,8 @@ int ssl3_new(SSL *s)
     s->s3 = s3;
 
 #ifndef OPENSSL_NO_SRP
-    SSL_SRP_CTX_init(s);
+    if(!SSL_SRP_CTX_init(s))
+          goto err;
 #endif
     s->method->ssl_clear(s);
     return (1);

diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
@@ -1320,7 +1320,10 @@ int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
         }
 #ifndef OPENSSL_NO_HEARTBEATS
         else if (rr->type == TLS1_RT_HEARTBEAT) {
-            tls1_process_heartbeat(s);
+            /* We can ignore 0 return values */
+            if(tls1_process_heartbeat(s) < 0) {
+                return -1;
+            }
 
             /* Exit and notify application to read again */
             rr->length = 0;

diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
@@ -226,8 +226,10 @@ int ssl3_accept(SSL *s)
 
     /* init things to blank */
     s->in_handshake++;
-    if (!SSL_in_init(s) || SSL_in_before(s))
-        SSL_clear(s);
+    if (!SSL_in_init(s) || SSL_in_before(s)) {
+        if(!SSL_clear(s))
+            return -1;
+    }
 
     if (s->cert == NULL) {
         SSLerr(SSL_F_SSL3_ACCEPT, SSL_R_NO_CERTIFICATE_SET);
@@ -2227,6 +2229,11 @@ int ssl3_get_client_key_exchange(SSL *s)
                                                         sizeof
                                                         (rand_premaster_secret));
         OPENSSL_cleanse(p, sizeof(rand_premaster_secret));
+        if(s->session->master_key_length < 0) {
+            al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+            goto f_err;
+        }
     } else
 #endif
 #ifndef OPENSSL_NO_DH
@@ -2319,6 +2326,11 @@ int ssl3_get_client_key_exchange(SSL *s)
                                                         session->master_key,
                                                         p, i);
         OPENSSL_cleanse(p, i);
+        if(s->session->master_key_length < 0) {
+            al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+            goto f_err;
+        }
         if (dh_clnt)
             return 2;
     } else
@@ -2484,6 +2496,11 @@ int ssl3_get_client_key_exchange(SSL *s)
                                                         s->
                                                         session->master_key,
                                                         pms, outl);
+        if(s->session->master_key_length < 0) {
+            al = SSL_INTERNAL_ERROR;
+            SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+            goto f_err;
+        }
 
         if (kssl_ctx->client_princ) {
             size_t len = strlen(kssl_ctx->client_princ);
@@ -2632,6 +2649,11 @@ int ssl3_get_client_key_exchange(SSL *s)
                                                         p, i);
 
         OPENSSL_cleanse(p, i);
+        if(s->session->master_key_length < 0) {
+            al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+            goto f_err;
+        }
         return (ret);
     } else
 #endif
@@ -2716,6 +2738,11 @@ int ssl3_get_client_key_exchange(SSL *s)
                                                         session->master_key,
                                                         psk_or_pre_ms,
                                                         pre_ms_len);
+        if(s->session->master_key_length < 0) {
+            al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+            goto psk_err;
+        }
         psk_err = 0;
  psk_err:
         OPENSSL_cleanse(psk_or_pre_ms, sizeof(psk_or_pre_ms));
@@ -2817,6 +2844,11 @@ int ssl3_get_client_key_exchange(SSL *s)
                                                         s->
                                                         session->master_key,
                                                         premaster_secret, 32);
+        if(s->session->master_key_length < 0) {
+            al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
+            goto f_err;
+        }
         /* Check if pubkey from client certificate was used */
         if (EVP_PKEY_CTX_ctrl
             (pkey_ctx, -1, -1, EVP_PKEY_CTRL_PEER_KEY, 2, NULL) > 0)

diff --git a/ssl/ssl.h b/ssl/ssl.h
@@ -1773,7 +1773,7 @@ void SSL_set_tmp_ecdh_callback(SSL *ssl,
 __owur const COMP_METHOD *SSL_get_current_compression(SSL *s);
 __owur const COMP_METHOD *SSL_get_current_expansion(SSL *s);
 __owur const char *SSL_COMP_get_name(const COMP_METHOD *comp);
-__owur STACK_OF(SSL_COMP) *SSL_COMP_get_compression_methods(void);
+STACK_OF(SSL_COMP) *SSL_COMP_get_compression_methods(void);
 __owur STACK_OF(SSL_COMP) *SSL_COMP_set0_compression_methods(STACK_OF(SSL_COMP)
                                                       *meths);
 void SSL_COMP_free_compression_methods(void);
@@ -1782,7 +1782,7 @@ __owur int SSL_COMP_add_compression_method(int id, COMP_METHOD *cm);
 __owur const void *SSL_get_current_compression(SSL *s);
 __owur const void *SSL_get_current_expansion(SSL *s);
 __owur const char *SSL_COMP_get_name(const void *comp);
-__owur void *SSL_COMP_get_compression_methods(void);
+void *SSL_COMP_get_compression_methods(void);
 __owur int SSL_COMP_add_compression_method(int id, void *cm);
 # endif
 
@@ -1956,6 +1956,7 @@ void ERR_load_SSL_strings(void);
 # define SSL_F_DTLS1_READ_BYTES                           258
 # define SSL_F_DTLS1_READ_FAILED                          259
 # define SSL_F_DTLS1_SEND_CERTIFICATE_REQUEST             260
+# define SSL_F_DTLS1_SEND_CHANGE_CIPHER_SPEC              342
 # define SSL_F_DTLS1_SEND_CLIENT_CERTIFICATE              261
 # define SSL_F_DTLS1_SEND_CLIENT_KEY_EXCHANGE             262
 # define SSL_F_DTLS1_SEND_CLIENT_VERIFY                   263

diff --git a/ssl/ssl_algs.c b/ssl/ssl_algs.c
@@ -130,7 +130,7 @@ int SSL_library_init(void)
      * This will initialise the built-in compression algorithms. The value
      * returned is a STACK_OF(SSL_COMP), but that can be discarded safely
      */
-    (void)SSL_COMP_get_compression_methods();
+    SSL_COMP_get_compression_methods();
 #endif
     /* initialize cipher/digest methods table */
     ssl_load_ciphers();

diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c
@@ -532,10 +532,13 @@ int ssl_cipher_get_evp(const SSL_SESSION *s, const EVP_CIPHER **enc,
             else
                 *comp = NULL;
         }
+        /* If were only interested in comp then return success */
+        if((enc == NULL) && (md == NULL))
+            return 1;
     }
 
     if ((enc == NULL) || (md == NULL))
-        return (0);
+        return 0;
 
     switch (c->algorithm_enc) {
     case SSL_DES: