sunrpc: close length validation gaps in krb5p unwrap

From: Chuck Lever <[email protected]>

Neither svcauth_gss_unwrap_priv() nor gss_unwrap_resp_priv() enforces a minimum on the wire-supplied opaque length before forwarding it to the krb5 unwrap core. An authenticated krb5p peer -- NFS client or server -- can send a token shorter than the 16-byte RFC 4121 header and reach gss_krb5_unwrap_v2() with a length that drives pre-decrypt out-of-bounds reads, a divide-by-zero in _rotate_left(), and a post-decrypt unsigned underflow in the movelen computation that feeds a multi-gigabyte memmove.

The fixes are layered so that neither the caller boundary nor the unwrap core depends on the other for safety. The caller-side floor checks (patches 1, 2) are sufficient to close the attack, but gss_krb5_unwrap_v2() must also be self-defending because it is reachable through gss_unwrap() from any future caller that might not enforce the same floor. The xdr_buf_trim() clamp (patch 3) is independent of krb5 -- it corrects a generic invariant violation that gss_krb5_unwrap_v2() happens to be the only current trigger for, but that any caller could hit if buf->len and the component iov_lens get out of sync.

The client side (patch 2) has an additional u32 integer-overflow bypass that the server side does not: the original offset + opaque_len > rcv_buf->len wraps when opaque_len is near UINT_MAX, accepting a huge token length that re-enters the same underflow chain. The replacement splits the check into separate offset and length guards that are safe in u32 arithmetic.

Chris Mason (4):
SUNRPC: svcauth_gss: enforce krb5 token minimum length
SUNRPC: harden gss_unwrap_resp_priv length checks
SUNRPC: xdr_buf_trim: clamp buf->len to avoid underflow
SUNRPC: harden gss_krb5_unwrap_v2 against short tokens

 net/sunrpc/auth_gss/auth_gss.c      |  6 +++++-
 net/sunrpc/auth_gss/gss_krb5_wrap.c | 11 +++++++++--
 net/sunrpc/auth_gss/svcauth_gss.c   |  2 ++
 net/sunrpc/xdr.c                    |  2 +-
 4 files changed, 17 insertions(+), 4 deletions(-)

2.54.0

From: Chris Mason <[email protected]>

svcauth_gss_unwrap_priv() validates only an upper bound on the wire-supplied opaque length before handing the buffer to gss_unwrap():

    if (len > xdr_stream_remaining(xdr))
            goto unwrap_failed;
    offset = xdr_stream_pos(xdr);
    ...
    maj_stat = gss_unwrap(ctx, offset, offset + len, buf);

The wire value len flows unchanged as the upper bound into the krb5 unwrap path, so a len in [0, 16] passes this check and is handed to gss_unwrap(). For a krb5 v2 context that lands in gss_krb5_unwrap_v2(), which reads the 16-byte RFC 4121 token header fields at ptr+4 and ptr+6 and then calls rotate_left() before any integrity check. With a sub-header length the header reads run past the token, and _rotate_left()'s shift %= buf->len path can divide by zero when buf->len has been driven to zero by the truncated token. A header-only token (len == 16) is equally
invalid: with a non-zero RRC field and the opaque blob ending at the XDR buffer boundary, rotate_left() builds a zero-length subbuffer, reaching the same division.

Reject the token at the server entry point before it reaches the krb5 unwrap core. A valid sealed RFC 4121 token must contain the 16-byte header plus at least some encrypted payload.

Fix by adding a minimum-length check immediately after the existing upper-bound check:

    if (len <= GSS_KRB5_TOK_HDR_LEN)
            goto unwrap_failed;

---
 net/sunrpc/auth_gss/svcauth_gss.c | 2 ++
 1 file changed, 2 insertions(+)

index d14209031e18..8e8aceb31270 100644
@@ -949,6 +949,8 @@ svcauth_gss_unwrap_priv(struct svc_rqst *rqstp, u32 seq, struct gss_ctx *ctx)
 	}
 	if (len > xdr_stream_remaining(xdr))
 		goto unwrap_failed;
+	if (len <= GSS_KRB5_TOK_HDR_LEN)
+		goto unwrap_failed;
 	offset = xdr_stream_pos(xdr);
 
 	saved_len = buf->len;

2.54.0

From: Chris Mason <[email protected]>

gss_unwrap_resp_priv() validates the RPCSEC_GSS opaque length with

    offset = (u8 *)(p) - (u8 *)head->iov_base;
    if (offset + opaque_len > rcv_buf->len)
            goto unwrap_failed;
    maj_stat = gss_unwrap(ctx->gc_gss_ctx, offset,
                          offset + opaque_len, rcv_buf);

Both operands are u32 and the sum is computed in u32. A reply with opaque_len near 0xffffffff makes offset + opaque_len wrap to a small value that is below rcv_buf->len, so the bound check passes and gss_unwrap() is called with end < begin. The check also lacks a lower bound, so any opaque_len in [0, GSS_KRB5_TOK_HDR_LEN) is accepted and forwarded to gss_krb5_unwrap_v2(), whose pre-decrypt header reads at ptr+4 and ptr+6 then run past the token.

A krb5p NFS server returning a crafted RPCSEC_GSS reply can drive the client into out-of-bounds reads in gss_krb5_unwrap_v2() and the rotate_left() loop that follows.

Fix by replacing the single combined check with three guards that are safe in u32 arithmetic and that enforce the RFC 4121 minimum outer token length:

    if (offset > rcv_buf->len)
            goto unwrap_failed;
    if (opaque_len > rcv_buf->len - offset)
            goto unwrap_failed;
    if (opaque_len < GSS_KRB5_TOK_HDR_LEN)
            goto unwrap_failed;

The first guard makes the subtraction in the second guard unconditionally safe; offset is derived from a successful xdr_inline_decode() in the head kvec, so in practice it already satisfies the bound. The floor mirrors the server-side check added in commit 7507012eeb98 ("SUNRPC: svcauth_gss: enforce krb5 token minimum length").

---
 net/sunrpc/auth_gss/auth_gss.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 9d3fb6848f40..8ddc65e894da 100644
@@ -2072,7 +2072,11 @@ gss_unwrap_resp_priv(struct rpc_task *task, struct rpc_cred *cred,
 		goto unwrap_failed;
 	opaque_len = be32_to_cpup(p++);
 	offset = (u8 *)(p) - (u8 *)head->iov_base;
-	if (offset + opaque_len > rcv_buf->len)
+	if (offset > rcv_buf->len)
+		goto unwrap_failed;
+	if (opaque_len > rcv_buf->len - offset)
+		goto unwrap_failed;
+	if (opaque_len <= GSS_KRB5_TOK_HDR_LEN)
 		goto unwrap_failed;
 
 	maj_stat = gss_unwrap(ctx->gc_gss_ctx, offset,

2.54.0

From: Chris Mason <[email protected]>

xdr_buf_trim() trims len bytes from the tail of an xdr_buf by walking the tail, pages, and head iovecs. Each per-section step uses min_t() so it never removes more bytes than that section holds, but the final accounting at the fix_len label subtracts the total bytes actually consumed from buf->len without any clamp:

    fix_len:
            buf->len -= (len - trim);

When the caller has set buf->len to a value smaller than the sum of the iov_lens, (len - trim) can exceed buf->len and the unsigned subtraction wraps to near UINT_MAX. gss_krb5_unwrap_v2() reaches xdr_buf_trim() in exactly that state:

    buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
    buf->len = len - (GSS_KRB5_TOK_HDR_LEN + headskip);
    xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);

buf->len is a small wire-derived value while the iov_lens are at page scale, so the per-section loops legitimately consume far more bytes than buf->len records. The wrapped buf->len then propagates as the authoritative stream bound into every downstream XDR decoder.

Fix by clamping the decrement so buf->len bottoms out at zero:

    buf->len -= min_t(unsigned int, buf->len, len - trim);

On the normal path where the iov_lens sum to buf->len, (len - trim) is always <= buf->len and the result is identical to before. No callers change behavior outside the underflow case.

---
 net/sunrpc/xdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index fa6a30b5f046..cb2ef428651f 100644
@@ -2049,7 +2049,7 @@ void xdr_buf_trim(struct xdr_buf *buf, unsigned int len)
 		trim -= cur;
 	}
 fix_len:
-	buf->len -= (len - trim);
+	buf->len -= min_t(unsigned int, buf->len, len - trim);
 }
 EXPORT_SYMBOL_GPL(xdr_buf_trim);
 

2.54.0

From: Chris Mason <[email protected]>

gss_krb5_unwrap_v2() reads the EC and RRC header fields at ptr+4 and ptr+6 before validating that the token is at least GSS_KRB5_TOK_HDR_LEN
(16) bytes long, and its rotate_left() helper passes buf->len - base to xdr_buf_subsegment() without verifying that base <= buf->len. When a caller hands in a sub-16-byte token, or a token whose declared len leaves base past the end of the buffer, three distinct failures follow:

gss_krb5_unwrap_v2(offset, len, buf)
ptr = buf->head[0].iov_base + offset
ec = *(ptr + 4) /* OOB read on short head */
rrc = *(ptr + 6) /* OOB read on short head */
rotate_left(offset + 16, buf, rrc)
xdr_buf_subsegment(buf, &subbuf,
base, buf->len - base) /* u32 wrap when base > len */
_rotate_left(&subbuf, shift)
shift %= buf->len /* divide-by-zero when base == len */

After decryption, the cleanup arithmetic has the same shape:

    movelen = min_t(unsigned int, buf->head[0].iov_len, len);
    movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip;
    BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
                                            buf->head[0].iov_len);

The BUG_ON re-adds the value just subtracted, so it reduces to min(A, B) > A and is permanently false; it cannot catch the unsigned underflow of movelen, which then drives a ~UINT_MAX-byte memmove().

Add four defense-in-depth guards inside the unwrap core so it is safe regardless of what its callers validate:

- reject tokens with len - offset < GSS_KRB5_TOK_HDR_LEN before

    touching ptr+4/ptr+6;
  - bail from rotate_left() when buf->len <= base, covering both the
    underflow and zero-length cases;
  - return early from _rotate_left() when buf->len is zero, so the
    shift %= buf->len modulo cannot fault;

- replace the dead BUG_ON with a live check that returns GSS_S_DEFECTIVE_TOKEN before the movelen subtraction.

---
 net/sunrpc/auth_gss/gss_krb5_wrap.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index ac4b32df42b9..d84c35f779f5 100644
@@ -73,6 +73,8 @@ static void _rotate_left(struct xdr_buf *buf, unsigned int shift)
 	int shifted = 0;
 	int this_shift;
 
+	if (!buf->len)
+		return;
 	shift %= buf->len;
 	while (shifted < shift) {
 		this_shift = min(shift - shifted, LOCAL_BUF_LEN);

@@ -85,6 +87,8 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift)
 {
 	struct xdr_buf subbuf;
 
+	if (buf->len <= base)
+		return;
 	xdr_buf_subsegment(buf, &subbuf, base, buf->len - base);
 	_rotate_left(&subbuf, shift);
 }

@@ -154,6 +158,9 @@ gss_krb5_unwrap_v2(struct krb5_ctx *kctx, int offset, int len,
 
 	dprintk("RPC:       %s\n", __func__);
 
+	if (len - offset <= GSS_KRB5_TOK_HDR_LEN)
+		return GSS_S_DEFECTIVE_TOKEN;
+
 	ptr = buf->head[0].iov_base + offset;
 
 	if (be16_to_cpu(*((__be16 *)ptr)) != KG2_TOK_WRAP)

@@ -220,9 +227,9 @@ gss_krb5_unwrap_v2(struct krb5_ctx *kctx, int offset, int len,
 	 * head buffer space rather than that actually occupied.
 	 */
 	movelen = min_t(unsigned int, buf->head[0].iov_len, len);
+	if (movelen < offset + GSS_KRB5_TOK_HDR_LEN + headskip)
+		return GSS_S_DEFECTIVE_TOKEN;
 	movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip;
-	BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
-							buf->head[0].iov_len);
 	memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen);
 	buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
 	buf->len = len - (GSS_KRB5_TOK_HDR_LEN + headskip);

2.54.0

Looks good. The unsigned offset math in this parsing code is just so nasty. I wonder if we'd be better off using signed math here for some of this. Oh well...