On Mon, 5 Feb 2018, Alexander Bluhm wrote:
> Hi,
>
> While reading ah_massage_headers() for the erratas, I found some
> issues which are ugly, but not security relevant.
>
> - Declare global array ipseczeroes with zeroes constant.
> - The proto parameter contains the address family, so call it af.
> - Remove an unused if block, just keep the else.
> - If m_copyback(M_NOWAIT) fails, return with error instead of working
> with an inconsistent mbuf.
> - ip6_nxt is u_int8_t, no need to clear the high bits.
> - The offset and next protocol are advanced for all extension
> headers, move it after the switch.
> - ah_massage_headers() returns an errno, call the variable error.
>
> ok?
one comment inline
ok procter@
>
> bluhm
>
> Index: netinet/ip_ah.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ah.c,v
> retrieving revision 1.134
> diff -u -p -r1.134 ip_ah.c
> --- netinet/ip_ah.c 1 Feb 2018 21:06:31 -0000 1.134
> +++ netinet/ip_ah.c 5 Feb 2018 16:05:04 -0000
> @@ -80,7 +80,7 @@ void ah_output_cb(struct cryptop *);
> void ah_input_cb(struct cryptop *);
> int ah_massage_headers(struct mbuf **, int, int, int, int);
>
> -unsigned char ipseczeroes[IPSEC_ZEROES_SIZE]; /* zeroes! */
> +const unsigned char ipseczeroes[IPSEC_ZEROES_SIZE]; /* zeroes! */
>
>
> /*
> @@ -190,21 +190,19 @@ ah_zeroize(struct tdb *tdbp)
> * Massage IPv4/IPv6 headers for AH processing.
> */
> int
> -ah_massage_headers(struct mbuf **m0, int proto, int skip, int alg, int out)
> +ah_massage_headers(struct mbuf **m0, int af, int skip, int alg, int out)
> {
> struct mbuf *m = *m0;
> unsigned char *ptr;
> - int off, count;
> -
> + int error, off, count;
> struct ip *ip;
> -
> #ifdef INET6
> struct ip6_ext *ip6e;
> struct ip6_hdr ip6;
> int ad, alloc, nxt, noff;
> #endif /* INET6 */
>
> - switch (proto) {
> + switch (af) {
> case AF_INET:
> /*
> * This is the least painful way of dealing with IPv4 header
> @@ -229,10 +227,8 @@ ah_massage_headers(struct mbuf **m0, int
>
> /* IPv4 option processing */
> for (off = sizeof(struct ip); off < skip;) {
> - if (ptr[off] == IPOPT_EOL || ptr[off] == IPOPT_NOP ||
> - off + 1 < skip)
> - ;
> - else {
> + if (ptr[off] != IPOPT_EOL && ptr[off] != IPOPT_NOP &&
> + off + 1 >= skip) {
> DPRINTF(("%s: illegal IPv4 option length for"
> " option %d\n", __func__, ptr[off]));
>
> @@ -355,7 +351,14 @@ ah_massage_headers(struct mbuf **m0, int
> ip6.ip6_dst.s6_addr16[1] = 0;
>
> /* Done with IPv6 header. */
> - m_copyback(m, 0, sizeof(struct ip6_hdr), &ip6, M_NOWAIT);
> + error = m_copyback(m, 0, sizeof(struct ip6_hdr), &ip6,
> + M_NOWAIT);
> + if (error) {
> + DPRINTF(("%s: m_copyback no memory", __func__));
> + ahstat_inc(ahs_hdrops);
> + m_freem(m);
> + return error;
> + }
>
> /* Let's deal with the remaining headers (if any). */
> if (skip - sizeof(struct ip6_hdr) > 0) {
> @@ -386,7 +389,7 @@ ah_massage_headers(struct mbuf **m0, int
> } else
> break;
>
> - nxt = ip6.ip6_nxt & 0xff; /* Next header type. */
> + nxt = ip6.ip6_nxt; /* Next header type. */
>
> for (off = 0; off < skip - sizeof(struct ip6_hdr);) {
> if (off + sizeof(struct ip6_ext) >
> @@ -428,10 +431,6 @@ ah_massage_headers(struct mbuf **m0, int
>
> if (count != noff)
> goto error6;
> -
> - /* Advance. */
> - off += ((ip6e->ip6e_len + 1) << 3);
> - nxt = ip6e->ip6e_nxt;
> break;
>
> case IPPROTO_ROUTING:
> @@ -471,15 +470,17 @@ ah_massage_headers(struct mbuf **m0, int
> (caddr_t)&ip6);
> addr[0] = ip6.ip6_dst;
> ip6.ip6_dst = finaldst;
> - m_copyback(m, 0, sizeof(ip6), &ip6,
> - M_NOWAIT);
> -
> + error = m_copyback(m, 0, sizeof(ip6),
> + &ip6, M_NOWAIT);
> + if (error) {
> + if (alloc)
> + free(ptr, M_XDATA, 0);
> + ahstat_inc(ahs_hdrops);
> + m_freem(m);
> + return error;
> + }
> rh0->ip6r0_segleft = 0;
> }
> -
> - /* advance */
> - off += ((ip6e->ip6e_len + 1) << 3);
> - nxt = ip6e->ip6e_nxt;
> break;
> }
>
> @@ -493,13 +494,22 @@ error6:
> m_freem(m);
> return EINVAL;
> }
> +
> + /* Advance. */
> + off += ((ip6e->ip6e_len + 1) << 3);
> + nxt = ip6e->ip6e_nxt;
> }
>
> /* Copyback and free, if we allocated. */
> if (alloc) {
> - m_copyback(m, sizeof(struct ip6_hdr),
> + error = m_copyback(m, sizeof(struct ip6_hdr),
> skip - sizeof(struct ip6_hdr), ptr, M_NOWAIT);
> free(ptr, M_XDATA, 0);
> + if (error) {
> + ahstat_inc(ahs_hdrops);
> + m_freem(m);
> + return error;
> + }
> }
>
> break;
> @@ -520,7 +530,7 @@ ah_input(struct mbuf *m, struct tdb *tdb
> struct tdb_crypto *tc;
> u_int32_t btsx, esn;
> u_int8_t hl;
> - int rplen;
> + int error, rplen;
> #ifdef ENCDEBUG
> char buf[INET6_ADDRSTRLEN];
> #endif
> @@ -657,12 +667,13 @@ ah_input(struct mbuf *m, struct tdb *tdb
> m_copyback(m, skip + rplen, ahx->authsize, ipseczeroes, M_NOWAIT);
>
> /* "Massage" the packet headers for crypto processing. */
> - if ((btsx = ah_massage_headers(&m, tdb->tdb_dst.sa.sa_family,
> - skip, ahx->type, 0)) != 0) {
> + error = ah_massage_headers(&m, tdb->tdb_dst.sa.sa_family, skip,
> + ahx->type, 0);
> + if (error) {
> /* mbuf will be free'd by callee. */
This pre-existing comment muddled me. ah_massage_headers() has already
freed it on error.
(the callers of ah_input() pass it ownership of m, these being udp_input()
via ipsec_common_input() and if_bridge.c:bridge_ipsec() so far as I can
see.)
> free(tc, M_XDATA, 0);
> crypto_freereq(crp);
> - return btsx;
> + return error;
> }
>
> /* Crypto operation descriptor. */
> @@ -912,7 +923,7 @@ ah_output(struct mbuf *m, struct tdb *td
> struct mbuf *mi;
> struct cryptop *crp;
> u_int16_t iplen;
> - int len, rplen, roff;
> + int error, rplen, roff;
> u_int8_t prot;
> struct ah *ah;
> #if NBPFILTER > 0
> @@ -1144,12 +1155,13 @@ ah_output(struct mbuf *m, struct tdb *td
> m_copyback(m, protoff, sizeof(u_int8_t), &prot, M_NOWAIT);
>
> /* "Massage" the packet headers for crypto processing. */
> - if ((len = ah_massage_headers(&m, tdb->tdb_dst.sa.sa_family,
> - skip, ahx->type, 1)) != 0) {
> + error = ah_massage_headers(&m, tdb->tdb_dst.sa.sa_family, skip,
> + ahx->type, 1);
> + if (error) {
> /* mbuf will be free'd by callee. */
ditto
> free(tc, M_XDATA, 0);
> crypto_freereq(crp);
> - return len;
> + return error;
> }
>
> /* Crypto operation descriptor. */
>
>
