git: 16b538975024 - stable/13 - sctp: improve input validation
Michael Tuexen
tuexen at FreeBSD.org
Tue Mar 2 12:52:43 UTC 2021
The branch stable/13 has been updated by tuexen:
URL: https://cgit.FreeBSD.org/src/commit/?id=16b538975024e2b7038807bf5b712124f5a7b889
commit 16b538975024e2b7038807bf5b712124f5a7b889
Author: Michael Tuexen <tuexen at FreeBSD.org>
AuthorDate: 2021-01-31 22:43:15 +0000
Commit: Michael Tuexen <tuexen at FreeBSD.org>
CommitDate: 2021-03-02 12:29:00 +0000
sctp: improve input validation
Improve the handling of INIT chunks in specific szenarios and
report and appropriate error cause.
Thanks to Anatoly Korniltsev for reporting the issue for the
userland stack.
(cherry picked from commit af885c57d65d33c0306e91d3e090e76772a0d012)
---
sys/netinet/sctp_output.c | 100 ++++++++++++++++++++++++++++------------------
1 file changed, 62 insertions(+), 38 deletions(-)
diff --git a/sys/netinet/sctp_output.c b/sys/netinet/sctp_output.c
index d58ebf785238..0f7ade931e61 100644
--- a/sys/netinet/sctp_output.c
+++ b/sys/netinet/sctp_output.c
@@ -5232,31 +5232,33 @@ invalid_size:
return (op_err);
}
-static int
+/*
+ * Given a INIT chunk, look through the parameters to verify that there
+ * are no new addresses.
+ * Return true, if there is a new address or there is a problem parsing
+ the parameters. Provide an optional error cause used when sending an ABORT.
+ * Return false, if there are no new addresses and there is no problem in
+ parameter processing.
+ */
+static bool
sctp_are_there_new_addresses(struct sctp_association *asoc,
- struct mbuf *in_initpkt, int offset, struct sockaddr *src)
+ struct mbuf *in_initpkt, int offset, int limit, struct sockaddr *src,
+ struct mbuf **op_err)
{
- /*
- * Given a INIT packet, look through the packet to verify that there
- * are NO new addresses. As we go through the parameters add reports
- * of any un-understood parameters that require an error. Also we
- * must return (1) to drop the packet if we see a un-understood
- * parameter that tells us to drop the chunk.
- */
struct sockaddr *sa_touse;
struct sockaddr *sa;
struct sctp_paramhdr *phdr, params;
- uint16_t ptype, plen;
- uint8_t fnd;
struct sctp_nets *net;
- int check_src;
#ifdef INET
struct sockaddr_in sin4, *sa4;
#endif
#ifdef INET6
struct sockaddr_in6 sin6, *sa6;
#endif
+ uint16_t ptype, plen;
+ bool fnd, check_src;
+ *op_err = NULL;
#ifdef INET
memset(&sin4, 0, sizeof(sin4));
sin4.sin_family = AF_INET;
@@ -5268,19 +5270,19 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
sin6.sin6_len = sizeof(sin6);
#endif
/* First what about the src address of the pkt ? */
- check_src = 0;
+ check_src = false;
switch (src->sa_family) {
#ifdef INET
case AF_INET:
if (asoc->scope.ipv4_addr_legal) {
- check_src = 1;
+ check_src = true;
}
break;
#endif
#ifdef INET6
case AF_INET6:
if (asoc->scope.ipv6_addr_legal) {
- check_src = 1;
+ check_src = true;
}
break;
#endif
@@ -5289,7 +5291,7 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
break;
}
if (check_src) {
- fnd = 0;
+ fnd = false;
TAILQ_FOREACH(net, &asoc->nets, sctp_next) {
sa = (struct sockaddr *)&net->ro._l_addr;
if (sa->sa_family == src->sa_family) {
@@ -5300,7 +5302,7 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
sa4 = (struct sockaddr_in *)sa;
src4 = (struct sockaddr_in *)src;
if (sa4->sin_addr.s_addr == src4->sin_addr.s_addr) {
- fnd = 1;
+ fnd = true;
break;
}
}
@@ -5312,16 +5314,22 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
sa6 = (struct sockaddr_in6 *)sa;
src6 = (struct sockaddr_in6 *)src;
if (SCTP6_ARE_ADDR_EQUAL(sa6, src6)) {
- fnd = 1;
+ fnd = true;
break;
}
}
#endif
}
}
- if (fnd == 0) {
- /* New address added! no need to look further. */
- return (1);
+ if (!fnd) {
+ /*
+ * If sending an ABORT in case of an additional
+ * address, don't use the new address error cause.
+ * This looks no different than if no listener was
+ * present.
+ */
+ *op_err = sctp_generate_cause(SCTP_BASE_SYSCTL(sctp_diag_info_code), "Address added");
+ return (true);
}
}
/* Ok so far lets munge through the rest of the packet */
@@ -5331,6 +5339,14 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
sa_touse = NULL;
ptype = ntohs(phdr->param_type);
plen = ntohs(phdr->param_length);
+ if (offset + plen > limit) {
+ *op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, "Partial parameter");
+ return (true);
+ }
+ if (plen < sizeof(struct sctp_paramhdr)) {
+ *op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, "Parameter length too small");
+ return (true);
+ }
switch (ptype) {
#ifdef INET
case SCTP_IPV4_ADDRESS:
@@ -5338,12 +5354,14 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
struct sctp_ipv4addr_param *p4, p4_buf;
if (plen != sizeof(struct sctp_ipv4addr_param)) {
- return (1);
+ *op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, "Parameter length illegal");
+ return (true);
}
phdr = sctp_get_next_param(in_initpkt, offset,
(struct sctp_paramhdr *)&p4_buf, sizeof(p4_buf));
if (phdr == NULL) {
- return (1);
+ *op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, "");
+ return (true);
}
if (asoc->scope.ipv4_addr_legal) {
p4 = (struct sctp_ipv4addr_param *)phdr;
@@ -5359,12 +5377,14 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
struct sctp_ipv6addr_param *p6, p6_buf;
if (plen != sizeof(struct sctp_ipv6addr_param)) {
- return (1);
+ *op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, "Parameter length illegal");
+ return (true);
}
phdr = sctp_get_next_param(in_initpkt, offset,
(struct sctp_paramhdr *)&p6_buf, sizeof(p6_buf));
if (phdr == NULL) {
- return (1);
+ *op_err = sctp_generate_cause(SCTP_CAUSE_PROTOCOL_VIOLATION, "");
+ return (true);
}
if (asoc->scope.ipv6_addr_legal) {
p6 = (struct sctp_ipv6addr_param *)phdr;
@@ -5381,7 +5401,7 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
}
if (sa_touse) {
/* ok, sa_touse points to one to check */
- fnd = 0;
+ fnd = false;
TAILQ_FOREACH(net, &asoc->nets, sctp_next) {
sa = (struct sockaddr *)&net->ro._l_addr;
if (sa->sa_family != sa_touse->sa_family) {
@@ -5392,7 +5412,7 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
sa4 = (struct sockaddr_in *)sa;
if (sa4->sin_addr.s_addr ==
sin4.sin_addr.s_addr) {
- fnd = 1;
+ fnd = true;
break;
}
}
@@ -5402,21 +5422,31 @@ sctp_are_there_new_addresses(struct sctp_association *asoc,
sa6 = (struct sockaddr_in6 *)sa;
if (SCTP6_ARE_ADDR_EQUAL(
sa6, &sin6)) {
- fnd = 1;
+ fnd = true;
break;
}
}
#endif
}
if (!fnd) {
- /* New addr added! no need to look further */
- return (1);
+ /*
+ * If sending an ABORT in case of an
+ * additional address, don't use the new
+ * address error cause. This looks no
+ * different than if no listener was
+ * present.
+ */
+ *op_err = sctp_generate_cause(SCTP_BASE_SYSCTL(sctp_diag_info_code), "Address added");
+ return (true);
}
}
offset += SCTP_SIZE32(plen);
+ if (offset >= limit) {
+ break;
+ }
phdr = sctp_get_next_param(in_initpkt, offset, ¶ms, sizeof(params));
}
- return (0);
+ return (false);
}
/*
@@ -5472,17 +5502,11 @@ sctp_send_initiate_ack(struct sctp_inpcb *inp, struct sctp_tcb *stcb,
}
if ((asoc != NULL) &&
(SCTP_GET_STATE(stcb) != SCTP_STATE_COOKIE_WAIT)) {
- if (sctp_are_there_new_addresses(asoc, init_pkt, offset, src)) {
+ if (sctp_are_there_new_addresses(asoc, init_pkt, offset, offset + ntohs(init_chk->ch.chunk_length), src, &op_err)) {
/*
* new addresses, out of here in non-cookie-wait
* states
- *
- * Send an ABORT, without the new address error
- * cause. This looks no different than if no
- * listener was present.
*/
- op_err = sctp_generate_cause(SCTP_BASE_SYSCTL(sctp_diag_info_code),
- "Address added");
sctp_send_abort(init_pkt, iphlen, src, dst, sh, 0, op_err,
mflowtype, mflowid, inp->fibnum,
vrf_id, port);
More information about the dev-commits-src-all
mailing list