Re: [PATCH net v11 13/17] rxrpc: Fix RxGK key parser to check enctype is supported

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch rewrote the RxGK key quota charge in
  rxrpc_preparse_xdr_yfs_rxgk() (net/rxrpc/key.c) and dropped the size…
- [Medium] The new key-length check in rxrpc_preparse_xdr_yfs_rxgk()
  (net/rxrpc/key.c:235-238, `if (raw_keylen != enc->key_len) { ret =…

--

rxrpc: Fix RxGK key parser to check enctype is supported

The RxGK key parser now checks that the token's enctype is supported and
that the key length matches that enctype. The RxGK key handling is now
built only when CONFIG_RXGK is set. Quota is now charged in both the rxkad
and RxGK parsers only after a token is accepted.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com

> diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
> index cbd26da449510..904da3fe7e478 100644
> --- a/net/rxrpc/key.c
> +++ b/net/rxrpc/key.c

[ ... ]

> @@ -202,10 +203,6 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  		goto reject;
>  	}
>  
> -	plen = sizeof(*token) + sizeof(*token->rxgk) + tktlen + keylen;
> -	prep->quotalen += datalen + plen;
> -
> -	plen -= sizeof(*token);
>  	token = kzalloc_obj(*token);
>  	if (!token)
>  		goto nomem;
> @@ -229,6 +226,17 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  	token->rxgk->key.data	= token->rxgk->_key;
>  	token->rxgk->ticket.len = raw_tktlen;
>  
> +	/* Check the enctype is supported. */
> +	enc = crypto_krb5_find_enctype(token->rxgk->enctype);
> +	if (!enc) {
> +		ret = -ENOPKG;
> +		goto reject_token;
> +	}
> +	if (raw_keylen != enc->key_len) {
> +		ret = -EKEYREJECTED;
> +		goto reject_token;
> +	}

[Severity: Medium]
Does the server side handle this new -EKEYREJECTED return? The server
reaches this parser through this chain:

rxgk_verify_response()
  -> rxgk_extract_token()
    -> rxgk_yfs_decode_ticket()
      -> key_instantiate_and_link()
        -> rxrpc_preparse()
          -> rxrpc_preparse_xdr()
            -> rxrpc_preparse_xdr_yfs_rxgk()

rxgk_yfs_decode_ticket() compares klen with the ticket length but not with
the enctype. It returns the error from key_instantiate_and_link() unchanged.

The cant_get_token switch in rxgk_extract_token() has no case for
-EKEYREJECTED:

net/rxrpc/rxgk_app.c:rxgk_extract_token() {
    ...
cant_get_token:
	switch (ret) {
	case -ENOMEM:
		goto temporary_error;
	case -EINVAL:
		return rxrpc_abort_conn(conn, skb, RXGK_NOTAUTH, -EKEYREJECTED,
					rxgk_abort_resp_tok_internal_error);
	case -ENOPKG:
		return rxrpc_abort_conn(conn, skb, KRB5_PROG_KEYTYPE_NOSUPP,
					-EKEYREJECTED, rxgk_abort_resp_tok_nopkg);
	}

temporary_error:
	...
	return ret;
}

So the error is handled like a temporary error. rxgk_verify_response()
then does goto out, the RESPONSE is dropped, and the connection stays in
RXRPC_CONN_SERVICE_CHALLENGING.

Before this patch, the same ticket got through key instantiation and failed
later, in rxgk_generate_transport_key() -> rxgk_set_up_ciphers() ->
crypto_krb5_calc_PRFplus().

For the RFC3961 enctypes, a key length mismatch made rfc3961_calc_DK()
return -EINVAL, and the connection was aborted with RXGK_NOTAUTH. For the
other enctypes, the authenticator failed to decrypt and the connection was
aborted with RXGK_SEALEDINCON.

The ticket is decrypted with the server key, so only a buggy or
misconfigured token issuer can produce one like this.

Should the cant_get_token switch in rxgk_extract_token() get an
-EKEYREJECTED case? Then the client would get an abort instead of being
re-challenged until it times out.

> +
>  	if (token->rxgk->endtime != 0) {
>  		expiry = rxrpc_s64_to_time64(token->rxgk->endtime);
>  		if (expiry < 0)

[ ... ]

> @@ -257,6 +265,8 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  	_debug("TICK: %*phN",
>  	       min_t(u32, token->rxgk->ticket.len, 32), token->rxgk->ticket.data);
>  
> +	prep->quotalen += sizeof(*token) + datalen + tktlen + keylen;

[Severity: Medium]
Was sizeof(*token->rxgk) meant to be dropped from the quota charge in
rxrpc_preparse_xdr_yfs_rxgk()? The removed code included it:

	plen = sizeof(*token) + sizeof(*token->rxgk) + tktlen + keylen;

token->rxgk is still allocated separately, and that allocation includes the
fixed struct rxgk_key header:

	token->rxgk = kzalloc_flex(struct rxgk_key, _key, raw_keylen);

The rxkad hunk in this same patch still includes sizeof(*token->kad) in
plen, so the two parsers now charge differently.

The datalen term doesn't make up the difference. rxrpc_preparse_xdr()
requires the remaining datalen to be 0 before it calls the token parsers,
so datalen is always 0 here.

So each accepted RxGK token uses about 72 bytes more (on 64-bit) than
generic_key_instantiate() reserves against the user's key quota.

Should this be something like:

	prep->quotalen += sizeof(*token) + sizeof(*token->rxgk) + datalen +
			  tktlen + keylen;

> +
>  	/* count the number of tokens attached */
>  	prep->payload.data[1] = (void *)((unsigned long)prep->payload.data[1] + 1);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923133706.1496540-1-dhowells%40redhat.com