[gnutls-devel] [RFC] Relaxing cipher suite (priority) string requirements
Jouko Orava
jouko.orava at helsinki.fi
Sat Jan 26 06:38:44 CET 2013
> I think the idea of simplifying the rules is a nice one.
Thanks!
Perhaps the lib/gnutls_priority.c:prio_remove() and optional "+" in
priority strings should be separated into simpler patches for now?
Suggested but untested patches are attached.
I'll post a separate message expanding on my ideas for enhancing the
priority string parsing.
> > - Allow full cipher names, adding/removing cipher, mac, and kx
>
> This is tricky. Although I don't think we are going to have a cipher
> called SHA1, I was afraid of collisions and that's why we have this
> awkward format. E.g. what does the +NULL mean? the NULL cipher? or
> compression? How could you handle that?
No, I meant that users seem to expect being able to specify e.g.
TLS_RSA_AES_256_CBC_SHA1 as a priority string. The untested patch
is supposed to detect that string as it matches the name entry in
cs_algorithms[], and add the cipher, mac, and kx from that entry
to the respective priority lists.
In other words, priority string
TLS_RSA_AES_256_CBC_SHA1
should have the same effect as
+RSA:+AES_256_CBC:+SHA1
(This would be based on matching to a cs_algorithms[] name,
and using the cipher/mac/kx fields in that entry,
not splitting or parsing the string itself.)
I do keenly understand it is paramount to not change the
interpretation of existing valid GnuTLS priority strings.
I only wanted to add new, unambiguous strings the parser
would recognize, not change existing interpretations or
risk confusion.
Best regards,
Jouko
-------------- next part --------------
This patch makes '+' prefix optional in priority strings.
diff -Naur gnutls.orig/lib/gnutls_priority.c gnutls.new/lib/gnutls_priority.c
--- gnutls.orig/lib/gnutls_priority.c 2013-01-26 05:58:18.948730334 +0200
+++ gnutls.new/lib/gnutls_priority.c 2013-01-26 07:21:48.763014539 +0200
@@ -792,8 +794,8 @@
{
char *broken_list[MAX_ELEMENTS];
int broken_list_size = 0, i = 0, j;
- char *darg = NULL;
- int algo;
+ char *darg = NULL, *part;
+ int algo, op;
rmadd_func *fn;
bulk_rmadd_func *bulk_fn;
@@ -849,44 +851,56 @@
{
continue;
}
- else if (broken_list[i][0] == '!' || broken_list[i][0] == '+'
- || broken_list[i][0] == '-')
+ else if (broken_list[i][0] == '!' || broken_list[i][0] == '-'
+ || (broken_list[i][0] >= 'A' && broken_list[i][0] <= 'Z')
+ || (broken_list[i][0] >= 'a' && broken_list[i][0] <= 'z')
+ || broken_list[i][0] == '+')
{
if (broken_list[i][0] == '+')
{
fn = prio_add;
bulk_fn = _set_priority;
+ op = '+';
+ part = &broken_list[i][1];
}
- else
+ else if (broken_list[i][0] == '!' || broken_list[i][0] == '-')
{
fn = prio_remove;
bulk_fn = _clear_priorities;
+ op = '-';
+ part = &broken_list[i][1];
+ }
+ else
+ {
+ fn = prio_add;
+ bulk_fn = _set_priority;
+ op = '+';
+ part = &broken_list[i][0];
}
- if (broken_list[i][0] == '+' && check_level(&broken_list[i][1], *priority_cache, 1) != 0)
+ if (op == '+' && check_level(part, *priority_cache, 1) != 0)
{
continue;
}
- else if ((algo =
- gnutls_mac_get_id (&broken_list[i][1])) != GNUTLS_MAC_UNKNOWN)
+ else if ((algo = gnutls_mac_get_id (part)) !=
+ GNUTLS_MAC_UNKNOWN)
fn (&(*priority_cache)->mac, algo);
- else if ((algo = gnutls_cipher_get_id (&broken_list[i][1])) !=
+ else if ((algo = gnutls_cipher_get_id (part)) !=
GNUTLS_CIPHER_UNKNOWN)
fn (&(*priority_cache)->cipher, algo);
- else if ((algo = gnutls_kx_get_id (&broken_list[i][1])) !=
+ else if ((algo = gnutls_kx_get_id (part)) !=
GNUTLS_KX_UNKNOWN)
fn (&(*priority_cache)->kx, algo);
- else if (strncasecmp (&broken_list[i][1], "VERS-", 5) == 0)
+ else if (strncasecmp (part, "VERS-", 5) == 0)
{
- if (strncasecmp (&broken_list[i][1], "VERS-TLS-ALL", 12) == 0)
+ if (strcasecmp (part, "VERS-TLS-ALL") == 0)
{
bulk_fn (&(*priority_cache)->protocol,
protocol_priority);
}
else
{
- if ((algo =
- gnutls_protocol_get_id (&broken_list[i][6])) !=
+ if ((algo = gnutls_protocol_get_id (part)) !=
GNUTLS_VERSION_UNKNOWN)
fn (&(*priority_cache)->protocol, algo);
else
@@ -894,85 +908,81 @@
}
} /* now check if the element is something like -ALGO */
- else if (strncasecmp (&broken_list[i][1], "COMP-", 5) == 0)
+ else if (strncasecmp (part, "COMP-", 5) == 0)
{
- if (strncasecmp (&broken_list[i][1], "COMP-ALL", 8) == 0)
+ if (strcasecmp (part, "COMP-ALL") == 0)
{
bulk_fn (&(*priority_cache)->compression,
comp_priority);
}
else
{
- if ((algo =
- gnutls_compression_get_id (&broken_list[i][6])) !=
+ if ((algo = gnutls_compression_get_id (part + 5)) !=
GNUTLS_COMP_UNKNOWN)
fn (&(*priority_cache)->compression, algo);
else
goto error;
}
} /* now check if the element is something like -ALGO */
- else if (strncasecmp (&broken_list[i][1], "CURVE-", 6) == 0)
+ else if (strncasecmp (part, "CURVE-", 6) == 0)
{
- if (strncasecmp (&broken_list[i][1], "CURVE-ALL", 9) == 0)
+ if (strcasecmp (part, "CURVE-ALL") == 0)
{
bulk_fn (&(*priority_cache)->supported_ecc,
supported_ecc_normal);
}
else
{
- if ((algo =
- _gnutls_ecc_curve_get_id (&broken_list[i][7])) !=
+ if ((algo = _gnutls_ecc_curve_get_id (part + 6)) !=
GNUTLS_ECC_CURVE_INVALID)
fn (&(*priority_cache)->supported_ecc, algo);
else
goto error;
}
} /* now check if the element is something like -ALGO */
- else if (strncasecmp (&broken_list[i][1], "CTYPE-", 6) == 0)
+ else if (strncasecmp (part, "CTYPE-", 6) == 0)
{
- if (strncasecmp (&broken_list[i][1], "CTYPE-ALL", 9) == 0)
+ if (strcasecmp (part, "CTYPE-ALL") == 0)
{
bulk_fn (&(*priority_cache)->cert_type,
cert_type_priority_all);
}
else
{
- if ((algo =
- gnutls_certificate_type_get_id (&broken_list[i][7])) !=
+ if ((algo = gnutls_certificate_type_get_id (part + 6)) !=
GNUTLS_CRT_UNKNOWN)
fn (&(*priority_cache)->cert_type, algo);
else
goto error;
}
} /* now check if the element is something like -ALGO */
- else if (strncasecmp (&broken_list[i][1], "SIGN-", 5) == 0)
+ else if (strncasecmp (part, "SIGN-", 5) == 0)
{
- if (strncasecmp (&broken_list[i][1], "SIGN-ALL", 8) == 0)
+ if (strcasecmp (part, "SIGN-ALL") == 0)
{
bulk_fn (&(*priority_cache)->sign_algo,
sign_priority_default);
}
else
{
- if ((algo =
- gnutls_sign_get_id (&broken_list[i][6])) !=
+ if ((algo = gnutls_sign_get_id (part + 5)) !=
GNUTLS_SIGN_UNKNOWN)
fn (&(*priority_cache)->sign_algo, algo);
else
goto error;
}
}
- else if (strncasecmp (&broken_list[i][1], "MAC-ALL", 7) == 0)
+ else if (strcasecmp (part, "MAC-ALL") == 0)
{
bulk_fn (&(*priority_cache)->mac,
mac_priority_normal);
}
- else if (strncasecmp (&broken_list[i][1], "CIPHER-ALL", 10) == 0)
+ else if (strcasecmp (part, "CIPHER-ALL") == 0)
{
bulk_fn (&(*priority_cache)->cipher,
cipher_priority_normal);
}
- else if (strncasecmp (&broken_list[i][1], "KX-ALL", 6) == 0)
+ else if (strcasecmp (part, "KX-ALL") == 0)
{
bulk_fn (&(*priority_cache)->kx,
kx_priority_secure);
@@ -982,70 +992,43 @@
}
else if (broken_list[i][0] == '%')
{
- if (strcasecmp (&broken_list[i][1], "COMPAT") == 0)
+ part = &broken_list[i][1];
+ if (strcasecmp (part, "COMPAT") == 0)
{
ENABLE_COMPAT((*priority_cache));
}
- else if (strcasecmp (&broken_list[i][1], "NO_EXTENSIONS") == 0)
- {
- (*priority_cache)->no_extensions = 1;
- }
- else if (strcasecmp (&broken_list[i][1], "STATELESS_COMPRESSION") == 0)
- {
- (*priority_cache)->stateless_compression = 1;
- }
- else if (strcasecmp (&broken_list[i][1],
- "VERIFY_ALLOW_SIGN_RSA_MD5") == 0)
+ else if (strcasecmp (part, "VERIFY_ALLOW_SIGN_RSA_MD5") == 0)
{
prio_add (&(*priority_cache)->sign_algo, GNUTLS_SIGN_RSA_MD5);
(*priority_cache)->additional_verify_flags |=
GNUTLS_VERIFY_ALLOW_SIGN_RSA_MD5;
}
- else if (strcasecmp (&broken_list[i][1],
- "VERIFY_DISABLE_CRL_CHECKS") == 0)
- {
- (*priority_cache)->additional_verify_flags |=
- GNUTLS_VERIFY_DISABLE_CRL_CHECKS;
- }
- else if (strcasecmp (&broken_list[i][1],
- "SSL3_RECORD_VERSION") == 0)
+ else if (strcasecmp (part, "NO_EXTENSIONS") == 0)
+ (*priority_cache)->no_extensions = 1;
+ else if (strcasecmp (part, "STATELESS_COMPRESSION") == 0)
+ (*priority_cache)->stateless_compression = 1;
+ else if (strcasecmp (part, "VERIFY_DISABLE_CRL_CHECKS") == 0)
+ (*priority_cache)->additional_verify_flags |=
+ GNUTLS_VERIFY_DISABLE_CRL_CHECKS;
+ else if (strcasecmp (part, "SSL3_RECORD_VERSION") == 0)
(*priority_cache)->ssl3_record_version = 1;
- else if (strcasecmp (&broken_list[i][1],
- "LATEST_RECORD_VERSION") == 0)
+ else if (strcasecmp (part, "LATEST_RECORD_VERSION") == 0)
(*priority_cache)->ssl3_record_version = 0;
- else if (strcasecmp (&broken_list[i][1],
- "VERIFY_ALLOW_X509_V1_CA_CRT") == 0)
+ else if (strcasecmp (part, "VERIFY_ALLOW_X509_V1_CA_CRT") == 0)
(*priority_cache)->additional_verify_flags |=
GNUTLS_VERIFY_ALLOW_X509_V1_CA_CRT;
- else if (strcasecmp (&broken_list[i][1],
- "UNSAFE_RENEGOTIATION") == 0)
- {
- (*priority_cache)->sr = SR_UNSAFE;
- }
- else if (strcasecmp (&broken_list[i][1], "SAFE_RENEGOTIATION") == 0)
- {
- (*priority_cache)->sr = SR_SAFE;
- }
- else if (strcasecmp (&broken_list[i][1],
- "PARTIAL_RENEGOTIATION") == 0)
- {
- (*priority_cache)->sr = SR_PARTIAL;
- }
- else if (strcasecmp (&broken_list[i][1],
- "DISABLE_SAFE_RENEGOTIATION") == 0)
- {
- (*priority_cache)->sr = SR_DISABLED;
- }
- else if (strcasecmp (&broken_list[i][1],
- "SERVER_PRECEDENCE") == 0)
- {
- (*priority_cache)->server_precedence = 1;
- }
- else if (strcasecmp (&broken_list[i][1],
- "NEW_PADDING") == 0)
- {
- (*priority_cache)->new_record_padding = 1;
- }
+ else if (strcasecmp (part, "UNSAFE_RENEGOTIATION") == 0)
+ (*priority_cache)->sr = SR_UNSAFE;
+ else if (strcasecmp (part, "SAFE_RENEGOTIATION") == 0)
+ (*priority_cache)->sr = SR_SAFE;
+ else if (strcasecmp (part, "PARTIAL_RENEGOTIATION") == 0)
+ (*priority_cache)->sr = SR_PARTIAL;
+ else if (strcasecmp (part, "DISABLE_SAFE_RENEGOTIATION") == 0)
+ (*priority_cache)->sr = SR_DISABLED;
+ else if (strcasecmp (part, "SERVER_PRECEDENCE") == 0)
+ (*priority_cache)->server_precedence = 1;
+ else if (strcasecmp (part, "NEW_PADDING") == 0)
+ (*priority_cache)->new_record_padding = 1;
else
goto error;
}
-------------- next part --------------
This patch changes lib/gnutls_priority.c:prio_remove() so that
instead of replacing the removed priority with the final priority,
the order of the priorities is kept intact by moving the entire tail
of the list, if necessary.
diff -Naur gnutls.orig/lib/gnutls_priority.c gnutls.new/lib/gnutls_priority.c
--- gnutls.orig/lib/gnutls_priority.c 2013-01-26 05:58:18.948730334 +0200
+++ gnutls.new/lib/gnutls_priority.c 2013-01-26 07:21:48.763014539 +0200
@@ -547,8 +547,10 @@
if (pos >= 0)
{
- priority_list->priority[pos] = priority_list->priority[i - 1];
- priority_list->priority[i - 1] = 0;
+ if (pos < i - 1)
+ memmove(priority_list->priority + pos + 1,
+ priority_list->priority + pos,
+ (i - 1 - pos) * sizeof priority_list->priority[0]);
priority_list->algorithms--;
}
More information about the Gnutls-devel
mailing list