From fd5bc65cc889848100ef47436e31da82604b38e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bodo=20M=C3=B6ller?= Date: Tue, 20 Feb 2007 16:36:58 +0000 Subject: [PATCH] Improve ciphersuite order stability when disabling ciphersuites. Change ssl_create_cipher_list() to prefer ephemeral ECDH over ephemeral DH. --- CHANGES | 21 +++++++++++ ssl/ssl_ciph.c | 97 +++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 98 insertions(+), 20 deletions(-) diff --git a/CHANGES b/CHANGES index 44200d6f2e..837cce4981 100644 --- a/CHANGES +++ b/CHANGES @@ -4,6 +4,27 @@ Changes between 0.9.8e and 0.9.9 [xx XXX xxxx] + *) Change ssl_cipher_apply_rule(), the internal function that does + the work each time a ciphersuite string requests enabling + ("foo+bar"), moving ("+foo+bar"), disabling ("-foo+bar", or + removing ("!foo+bar") a class of ciphersuites: Now it maintains + the order of disabled ciphersuites such that those ciphersuites + that most recently went from enabled to disabled not only stay + in order with respect to each other, but also have higher priority + than other disabled ciphersuites the next time ciphersuites are + enabled again. + + This means that you can now say, e.g., "PSK:-PSK:HIGH" to enable + the same ciphersuites as with "HIGH" alone, but in a specific + order where the PSK ciphersuites come first (since they are the + most recently disabled ciphersuites when "HIGH" is parsed). + + Also, change ssl_create_cipher_list() (using this new + funcionality) such that between otherwise identical + cihpersuites, ephemeral ECDH is preferred over ephemeral DH in + the default order. + [Bodo Moeller] + *) Change ssl_create_cipher_list() so that it automatically arranges the ciphersuites in reasonable order before starting to process the rule string. Thus, the definition for "DEFAULT" diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c index a9d11ca474..fdfc0e8117 100644 --- a/ssl/ssl_ciph.c +++ b/ssl/ssl_ciph.c @@ -476,7 +476,7 @@ static void ll_append_tail(CIPHER_ORDER **head, CIPHER_ORDER *curr, *head=curr->next; if (curr->prev != NULL) curr->prev->next=curr->next; - if (curr->next != NULL) /* should always be true */ + if (curr->next != NULL) curr->next->prev=curr->prev; (*tail)->next=curr; curr->prev= *tail; @@ -484,6 +484,22 @@ static void ll_append_tail(CIPHER_ORDER **head, CIPHER_ORDER *curr, *tail=curr; } +static void ll_append_head(CIPHER_ORDER **head, CIPHER_ORDER *curr, + CIPHER_ORDER **tail) + { + if (curr == *head) return; + if (curr == *tail) + *tail=curr->prev; + if (curr->next != NULL) + curr->next->prev=curr->prev; + if (curr->prev != NULL) + curr->prev->next=curr->next; + (*head)->prev=curr; + curr->next= *head; + curr->prev=NULL; + *head=curr; + } + static void ssl_cipher_get_disabled(unsigned long *mkey, unsigned long *auth, unsigned long *enc, unsigned long *mac, unsigned long *ssl) { *mkey = 0; @@ -586,19 +602,27 @@ static void ssl_cipher_collect_ciphers(const SSL_METHOD *ssl_method, /* * Prepare linked list from list entries */ - for (i = 1; i < co_list_num - 1; i++) - { - co_list[i].prev = &(co_list[i-1]); - co_list[i].next = &(co_list[i+1]); - } if (co_list_num > 0) { - (*head_p) = &(co_list[0]); - (*head_p)->prev = NULL; - (*head_p)->next = &(co_list[1]); - (*tail_p) = &(co_list[co_list_num - 1]); - (*tail_p)->prev = &(co_list[co_list_num - 2]); - (*tail_p)->next = NULL; + co_list[0].prev = NULL; + + if (co_list_num > 1) + { + co_list[0].next = &co_list[1]; + + for (i = 1; i < co_list_num - 1; i++) + { + co_list[i].prev = &co_list[i - 1]; + co_list[i].next = &co_list[i + 1]; + } + + co_list[co_list_num - 1].prev = &co_list[co_list_num - 2]; + } + + co_list[co_list_num - 1].next = NULL; + + *head_p = &co_list[0]; + *tail_p = &co_list[co_list_num - 1]; } } @@ -679,22 +703,38 @@ static void ssl_cipher_apply_rule(unsigned long cipher_id, int rule, int strength_bits, CIPHER_ORDER **head_p, CIPHER_ORDER **tail_p) { - CIPHER_ORDER *head, *tail, *curr, *curr2, *tail2; + CIPHER_ORDER *head, *tail, *curr, *curr2, *last; SSL_CIPHER *cp; + int reverse = 0; #ifdef CIPHER_DEBUG printf("Applying rule %d with %08lx/%08lx/%08lx/%08lx/%08lx %08lx (%d)\n", rule, alg_mkey, alg_auth, alg_enc, alg_mac, alg_ssl, algo_strength, strength_bits); #endif - curr = head = *head_p; - curr2 = head; - tail2 = tail = *tail_p; + if (rule == CIPHER_DEL) + reverse = 1; /* needed to maintain sorting between currently deleted ciphers */ + + head = *head_p; + tail = *tail_p; + + if (reverse) + { + curr = tail; + last = head; + } + else + { + curr = head; + last = tail; + } + + curr2 = curr; for (;;) { - if ((curr == NULL) || (curr == tail2)) break; + if ((curr == NULL) || (curr == last)) break; curr = curr2; - curr2 = curr->next; + curr2 = reverse ? curr->prev : curr->next; cp = curr->cipher; @@ -736,6 +776,7 @@ static void ssl_cipher_apply_rule(unsigned long cipher_id, /* add the cipher if it has not been added yet. */ if (rule == CIPHER_ADD) { + /* reverse == 0 */ if (!curr->active) { ll_append_tail(&head, curr, &tail); @@ -745,15 +786,27 @@ static void ssl_cipher_apply_rule(unsigned long cipher_id, /* Move the added cipher to this location */ else if (rule == CIPHER_ORD) { + /* reverse == 0 */ if (curr->active) { ll_append_tail(&head, curr, &tail); } } else if (rule == CIPHER_DEL) - curr->active = 0; + { + /* reverse == 1 */ + if (curr->active) + { + /* most recently deleted ciphersuites get best positions + * for any future CIPHER_ADD (note that the CIPHER_DEL loop + * works in reverse to maintain the order) */ + ll_append_head(&head, curr, &tail); + curr->active = 0; + } + } else if (rule == CIPHER_KILL) { + /* reverse == 0 */ if (head == curr) head = curr->next; else @@ -1123,7 +1176,11 @@ STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *ssl_method, /* Now arrange all ciphers by preference: */ - /* Temporarily enabled AES first (preferred cipher) */ + /* Everything else being equal, prefer ephemeral ECDH over other key exchange mechanisms */ + ssl_cipher_apply_rule(0, SSL_kEECDH, 0, 0, 0, 0, 0, CIPHER_ADD, -1, &head, &tail); + ssl_cipher_apply_rule(0, SSL_kEECDH, 0, 0, 0, 0, 0, CIPHER_DEL, -1, &head, &tail); + + /* Temporarily enable AES first (preferred cipher) */ ssl_cipher_apply_rule(0, 0, 0, SSL_AES, 0, 0, 0, CIPHER_ADD, -1, &head, &tail); /* Temporarily enable everything else */