Hi,
I verified that regression test
src/regress/lib/libssl/unit/tls_ext_alpn.c fails on these cases;
- proto_invalid_len5, 7, 8
- proto_invalid_missing1 - 5
- proto_invalid_missing8, 9
To correct these failures, ssl_parse_clienthello_tlsext() and
ssl_parse_serverhello_tlsext() in ssl/t1_lib.c should be fixed.
I attached the patch and it resolves the issues above.
BTW, ssl_parse_serverhello_tlsext() and ssl_parse_clienthello_tlsext()
of OpenSSL codebase seems to have issues below, I believe.
- fixes in ssl_parse_clienthello_tlsext() is NOT reflected to
ssl_parse_serverhello_tlsext() (ex. limit, parsing logic, etc.)
- ssl_parse_serverhello_tlsext() always `goto ri_check`
if data structure is invalid.
If I'm wrong, please let me know.
Thanks.
diff --git src/lib/libssl/src/ssl/t1_lib.c src/lib/libssl/src/ssl/t1_lib.c
index b225bb3..01c341f 100644
--- src/lib/libssl/src/ssl/t1_lib.c
+++ src/lib/libssl/src/ssl/t1_lib.c
@@ -1214,20 +1214,25 @@ ssl_parse_clienthello_tlsext(SSL *s, unsigned char **p,
unsigned char *d,
s->s3->next_proto_neg_seen = 0;
free(s->s3->alpn_selected);
s->s3->alpn_selected = NULL;
+ s->srtp_profile = NULL;
- if (data >= (d + n - 2))
+ if (data == (d + n))
goto ri_check;
- n2s(data, len);
- if (data > (d + n - len))
- goto ri_check;
+ if ((d + n) - data < 2)
+ goto err;
+
+ n2s(data, len);
+ if ((d + n) - data != len)
+ goto err;
- while (data <= (d + n - 4)) {
+ while ((d + n) - data >= 4) {
n2s(data, type);
n2s(data, size);
- if (data + size > (d + n))
- goto ri_check;
+ if ((d + n) - data < size)
+ goto err;
+
if (s->tlsext_debug_cb)
s->tlsext_debug_cb(s, 0, type, data, size,
s->tlsext_debug_arg);
@@ -1560,6 +1565,10 @@ ssl_parse_clienthello_tlsext(SSL *s, unsigned char **p,
unsigned char *d,
data += size;
}
+ /* Spurious data on the end */
+ if (data != (d + n))
+ goto err;
+
*p = data;
ri_check:
@@ -1574,6 +1583,10 @@ ri_check:
}
return 1;
+
+err:
+ *al = SSL_AD_DECODE_ERROR;
+ return 0;
}
/*
@@ -1599,9 +1612,9 @@ int
ssl_parse_serverhello_tlsext(SSL *s, unsigned char **p, unsigned char *d,
int n, int *al)
{
- unsigned short length;
unsigned short type;
unsigned short size;
+ unsigned short len;
unsigned char *data = *p;
int tlsext_servername = 0;
int renegotiate_seen = 0;
@@ -1610,21 +1623,22 @@ ssl_parse_serverhello_tlsext(SSL *s, unsigned char **p,
unsigned char *d,
free(s->s3->alpn_selected);
s->s3->alpn_selected = NULL;
- if (data >= (d + n - 2))
+ if (data == (d + n))
goto ri_check;
- n2s(data, length);
- if (data + length != d + n) {
- *al = SSL_AD_DECODE_ERROR;
- return 0;
- }
+ if ((d + n) - data < 2)
+ goto err;
+
+ n2s(data, len);
+ if ((d + n) - data != len)
+ goto err;
- while (data <= (d + n - 4)) {
+ while ((d + n) - data >= 4) {
n2s(data, type);
n2s(data, size);
- if (data + size > (d + n))
- goto ri_check;
+ if ((d + n) - data < size)
+ goto err;
if (s->tlsext_debug_cb)
s->tlsext_debug_cb(s, 1, type, data, size,
@@ -1818,6 +1832,10 @@ ri_check:
}
return 1;
+
+err:
+ *al = SSL_AD_DECODE_ERROR;
+ return 0;
}
int
