- let SSL_CTX_set_cipher_list and SSL_set_cipher_list return an

  error if the cipher list is empty
- fix last commit in ssl_create_cipher_list
- clean up ssl_create_cipher_list

4 files changed, 53 insertions, 56 deletions

diff --git a/ssl/ssl.h b/ssl/ssl.h
index ea9a89762a..ab4626b2e7 100644
--- a/ssl/ssl.h
+++ b/ssl/ssl.h
@@ -1655,6 +1655,7 @@ void ERR_load_SSL_strings(void);
 #define SSL_F_SSL_CTRL					 232
 #define SSL_F_SSL_CTX_CHECK_PRIVATE_KEY			 168
 #define SSL_F_SSL_CTX_NEW				 169
+#define SSL_F_SSL_CTX_SET_CIPHER_LIST			 1026
 #define SSL_F_SSL_CTX_SET_PURPOSE			 226
 #define SSL_F_SSL_CTX_SET_SESSION_ID_CONTEXT		 219
 #define SSL_F_SSL_CTX_SET_SSL_VERSION			 170
@@ -1685,6 +1686,7 @@ void ERR_load_SSL_strings(void);
 #define SSL_F_SSL_SESSION_PRINT_FP			 190
 #define SSL_F_SSL_SESS_CERT_NEW				 225
 #define SSL_F_SSL_SET_CERT				 191
+#define SSL_F_SSL_SET_CIPHER_LIST			 1027
 #define SSL_F_SSL_SET_FD				 192
 #define SSL_F_SSL_SET_PKEY				 193
 #define SSL_F_SSL_SET_PURPOSE				 227
diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c
index b071a6309f..9493e2d85b 100644
--- a/ssl/ssl_ciph.c
+++ b/ssl/ssl_ciph.c
@@ -740,11 +740,18 @@ static int ssl_cipher_process_rulestr(const char *rule_str,
 			if (!found)
 				break;	/* ignore this entry */
 
-			algorithms = (ca_list[j]->algorithms & ~mask) |
-			             (ca_list[j]->algorithms &	algorithms & mask);
+			/* New algorithms:
+			 *  1 - any old restrictions apply outside new mask
+			 *  2 - any new restrictions apply outside old mask
+			 *  3 - enforce old & new where masks intersect
+			 */
+			algorithms = (algorithms & ~ca_list[j]->mask) |		/* 1 */
+			             (ca_list[j]->algorithms & ~mask) |		/* 2 */
+			             (algorithms & ca_list[j]->algorithms);	/* 3 */
 			mask |= ca_list[j]->mask;
-			algo_strength = (ca_list[j]->algo_strength & ~mask_strength) |
-			                (ca_list[j]->algo_strength & algo_strength & mask_strength);
+			algo_strength = (algo_strength & ~ca_list[j]->mask_strength) |
+			                (ca_list[j]->algo_strength & ~mask_strength) |
+			                (algo_strength & ca_list[j]->algo_strength);
 			mask_strength |= ca_list[j]->mask_strength;
 
 			if (!multi) break;
@@ -798,7 +805,7 @@ STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *ssl_method,
 	{
 	int ok, num_of_ciphers, num_of_alias_max, num_of_group_aliases;
 	unsigned long disabled_mask;
-	STACK_OF(SSL_CIPHER) *cipherstack;
+	STACK_OF(SSL_CIPHER) *cipherstack, *tmp_cipher_list;
 	const char *rule_p;
 	CIPHER_ORDER *co_list = NULL, *head = NULL, *tail = NULL, *curr;
 	SSL_CIPHER **ca_list = NULL;
@@ -806,7 +813,8 @@ STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *ssl_method,
 	/*
 	 * Return with error if nothing to do.
 	 */
-	if (rule_str == NULL) return(NULL);
+	if (rule_str == NULL || cipher_list == NULL || cipher_list_by_id == NULL)
+		return NULL;
 
 	if (init_ciphers)
 		{
@@ -912,54 +920,19 @@ STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *ssl_method,
 			}
 		}
 	OPENSSL_free(co_list);	/* Not needed any longer */
-	/* if no ciphers where selected let's return NULL */
-	if (sk_SSL_CIPHER_num(cipherstack) == 0)
-		{
-		SSLerr(SSL_F_SSL_CREATE_CIPHER_LIST, SSL_R_NO_CIPHER_MATCH);
-		sk_SSL_CIPHER_free(cipherstack);
-		return NULL;
-		}
 
-	/*
-	 * The following passage is a little bit odd. If pointer variables
-	 * were supplied to hold STACK_OF(SSL_CIPHER) return information,
-	 * the old memory pointed to is free()ed. Then, however, the
-	 * cipher_list entry will be assigned just a copy of the returned
-	 * cipher stack. For cipher_list_by_id a copy of the cipher stack
-	 * will be created. See next comment...
-	 */
-	if (cipher_list != NULL)
-		{
-		if (*cipher_list != NULL)
-			sk_SSL_CIPHER_free(*cipher_list);
-		*cipher_list = cipherstack;
-		}
-
-	if (cipher_list_by_id != NULL)
-		{
-		if (*cipher_list_by_id != NULL)
-			sk_SSL_CIPHER_free(*cipher_list_by_id);
-		*cipher_list_by_id = sk_SSL_CIPHER_dup(cipherstack);
-		}
-
-	/*
-	 * Now it is getting really strange. If something failed during
-	 * the previous pointer assignment or if one of the pointers was
-	 * not requested, the error condition is met. That might be
-	 * discussable. The strange thing is however that in this case
-	 * the memory "ret" pointed to is "free()ed" and hence the pointer
-	 * cipher_list becomes wild. The memory reserved for
-	 * cipher_list_by_id however is not "free()ed" and stays intact.
-	 */
-	if (	(cipher_list_by_id == NULL) ||
-		(*cipher_list_by_id == NULL) ||
-		(cipher_list == NULL) ||
-		(*cipher_list == NULL))
+	tmp_cipher_list = sk_SSL_CIPHER_dup(cipherstack);
+	if (tmp_cipher_list == NULL)
 		{
 		sk_SSL_CIPHER_free(cipherstack);
-		return(NULL);
+		return NULL;
 		}
-
+	if (*cipher_list != NULL)
+		sk_SSL_CIPHER_free(*cipher_list);
+	*cipher_list = cipherstack;
+	if (*cipher_list_by_id != NULL)
+		sk_SSL_CIPHER_free(*cipher_list_by_id);
+	*cipher_list_by_id = tmp_cipher_list;
 	sk_SSL_CIPHER_set_cmp_func(*cipher_list_by_id,ssl_cipher_ptr_id_cmp);
 
 	return(cipherstack);
diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 4f44920cdd..282f1a6755 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -183,6 +183,7 @@ static ERR_STRING_DATA SSL_str_functs[]=
 {ERR_FUNC(SSL_F_SSL_CTRL),	"SSL_ctrl"},
 {ERR_FUNC(SSL_F_SSL_CTX_CHECK_PRIVATE_KEY),	"SSL_CTX_check_private_key"},
 {ERR_FUNC(SSL_F_SSL_CTX_NEW),	"SSL_CTX_new"},
+{ERR_FUNC(SSL_F_SSL_CTX_SET_CIPHER_LIST),	"SSL_CTX_set_cipher_list"},
 {ERR_FUNC(SSL_F_SSL_CTX_SET_PURPOSE),	"SSL_CTX_set_purpose"},
 {ERR_FUNC(SSL_F_SSL_CTX_SET_SESSION_ID_CONTEXT),	"SSL_CTX_set_session_id_context"},
 {ERR_FUNC(SSL_F_SSL_CTX_SET_SSL_VERSION),	"SSL_CTX_set_ssl_version"},
@@ -213,6 +214,7 @@ static ERR_STRING_DATA SSL_str_functs[]=
 {ERR_FUNC(SSL_F_SSL_SESSION_PRINT_FP),	"SSL_SESSION_print_fp"},
 {ERR_FUNC(SSL_F_SSL_SESS_CERT_NEW),	"SSL_SESS_CERT_NEW"},
 {ERR_FUNC(SSL_F_SSL_SET_CERT),	"SSL_SET_CERT"},
+{ERR_FUNC(SSL_F_SSL_SET_CIPHER_LIST),	"SSL_set_cipher_list"},
 {ERR_FUNC(SSL_F_SSL_SET_FD),	"SSL_set_fd"},
 {ERR_FUNC(SSL_F_SSL_SET_PKEY),	"SSL_SET_PKEY"},
 {ERR_FUNC(SSL_F_SSL_SET_PURPOSE),	"SSL_set_purpose"},
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index d716d97c54..25efa29a57 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1153,8 +1153,21 @@ int SSL_CTX_set_cipher_list(SSL_CTX *ctx, const char *str)
 	
 	sk=ssl_create_cipher_list(ctx->method,&ctx->cipher_list,
 		&ctx->cipher_list_by_id,str);
-/* XXXX */
-	return((sk == NULL)?0:1);
+	/* ssl_create_cipher_list may return an empty stack if it
+	 * was unable to find a cipher matching the given rule string
+	 * (for example if the rule string specifies a cipher which
+	 * has been disabled). This is not an error as far as 
+	 * ssl_create_cipher_list is concerned, and hence 
+	 * ctx->cipher_list and ctx->cipher_list_by_id has been
+	 * updated. */
+	if (sk == NULL)
+		return 0;
+	else if (sk_SSL_CIPHER_num(sk) == 0)
+		{
+		SSLerr(SSL_F_SSL_CTX_SET_CIPHER_LIST, SSL_R_NO_CIPHER_MATCH);
+		return 0;
+		}
+	return 1;
 	}
 
 /** specify the ciphers to be used by the SSL */
@@ -1164,8 +1177,15 @@ int SSL_set_cipher_list(SSL *s,const char *str)
 	
 	sk=ssl_create_cipher_list(s->ctx->method,&s->cipher_list,
 		&s->cipher_list_by_id,str);
-/* XXXX */
-	return((sk == NULL)?0:1);
+	/* see comment in SSL_CTX_set_cipher_list */
+	if (sk == NULL)
+		return 0;
+	else if (sk_SSL_CIPHER_num(sk) == 0)
+		{
+		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 */
@@ -1377,8 +1397,8 @@ SSL_CTX *SSL_CTX_new(SSL_METHOD *meth)
 	ret->default_passwd_callback=0;
 	ret->default_passwd_callback_userdata=NULL;
 	ret->client_cert_cb=0;
-    ret->app_gen_cookie_cb=0;
-    ret->app_verify_cookie_cb=0;
+	ret->app_gen_cookie_cb=0;
+	ret->app_verify_cookie_cb=0;
 
 	ret->sessions=lh_new(LHASH_HASH_FN(SSL_SESSION_hash),
 			LHASH_COMP_FN(SSL_SESSION_cmp));