From owner-dev-commits-src-all@freebsd.org Tue Mar 2 12:52:43 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 55500568A07; Tue, 2 Mar 2021 12:52:43 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DqcVM1yzVz3qpS; Tue, 2 Mar 2021 12:52:43 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 365F71736A; Tue, 2 Mar 2021 12:52:43 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 122Cqh39050723; Tue, 2 Mar 2021 12:52:43 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 122CqhrK050722; Tue, 2 Mar 2021 12:52:43 GMT (envelope-from git) Date: Tue, 2 Mar 2021 12:52:43 GMT Message-Id: <202103021252.122CqhrK050722@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Michael Tuexen Subject: git: 16b538975024 - stable/13 - sctp: improve input validation MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: tuexen X-Git-Repository: src X-Git-Refname: refs/heads/stable/13 X-Git-Reftype: branch X-Git-Commit: 16b538975024e2b7038807bf5b712124f5a7b889 Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Mar 2021 12:52:43 -0000 The branch stable/13 has been updated by tuexen: URL: https://cgit.FreeBSD.org/src/commit/?id=16b538975024e2b7038807bf5b712124f5a7b889 commit 16b538975024e2b7038807bf5b712124f5a7b889 Author: Michael Tuexen AuthorDate: 2021-01-31 22:43:15 +0000 Commit: Michael Tuexen 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);