diff options
author | Matt Caswell <matt@openssl.org> | 2015-04-29 16:15:40 +0100 |
---|---|---|
committer | Matt Caswell <matt@openssl.org> | 2015-04-29 17:44:02 +0100 |
commit | 80a06268ae329a1d7e01292029f9ae3af172b4b8 (patch) | |
tree | e8199d3e5b497cc41afdd4f2c36dc61f33d2b061 | |
parent | e3dd33c25c885ab3bfe707d87ddb12f845d77032 (diff) |
Add length sanity check in SSLv2 n_do_ssl_write()
Fortify flagged up a problem in n_do_ssl_write() in SSLv2. Analysing the
code I do not believe there is a real problem here. However the logic flows
are complicated enough that a sanity check of |len| is probably worthwhile.
Thanks to Kevin Wojtysiak (Int3 Solutions) and Paramjot Oberoi (Int3
Solutions) for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(cherry picked from commit c5f8cd7bc661f90dc012c9d2bae1808a4281985f)
-rw-r--r-- | ssl/s2_pkt.c | 14 |
1 files changed, 14 insertions, 0 deletions
diff --git a/ssl/s2_pkt.c b/ssl/s2_pkt.c index 614b9a35d2..7a61888134 100644 --- a/ssl/s2_pkt.c +++ b/ssl/s2_pkt.c @@ -576,6 +576,20 @@ static int n_do_ssl_write(SSL *s, const unsigned char *buf, unsigned int len) s->s2->padding = p; s->s2->mac_data = &(s->s2->wbuf[3]); s->s2->wact_data = &(s->s2->wbuf[3 + mac_size]); + + /* + * It would be clearer to write this as follows: + * if (mac_size + len + p > SSL2_MAX_RECORD_LENGTH_2_BYTE_HEADER) + * However |len| is user input that could in theory be very large. We + * know |mac_size| and |p| are small, so to avoid any possibility of + * overflow we write it like this. + * + * In theory this should never fail because the logic above should have + * modified |len| if it is too big. But we are being cautious. + */ + if (len > (SSL2_MAX_RECORD_LENGTH_2_BYTE_HEADER - (mac_size + p))) { + return -1; + } /* we copy the data into s->s2->wbuf */ memcpy(s->s2->wact_data, buf, len); if (p) |