Re: [PATCH 1/1] KEYS: Account for asymmetric key payload data in the quota

[Yuqi Xu <xuyuqiabc@gmail.com>]

Hi all,

Thanks for the review.

Sashiko reported the following findings on the patchset page; they have
not been posted to lore.  I looked at both against the tree; the
mechanism they describe is real, but it is not reachable by an
unprivileged user with the default quota, and the root case is not a
privilege boundary.  Details inline below.

The tree is crypto-2.6.git master at 10396a2d6d41; line numbers below are
for that revision plus this patch.

> 1) Does this code overflow key->quotalen if pub->keylen is large? While
>    prep->quotalen is a size_t, the internal key->quotalen field is an
>    unsigned short with a 16-bit limit of 65,535 bytes. If a user adds an
>    oversized asymmetric key and the quota capacity check passes (for
>    example, for the root user, or if kernel.keys.maxbytes is raised),
>    key->quotalen will silently overflow and truncate the length. When the
>    key is later destroyed in key_put(), user->qnbytes is decremented by the
>    truncated amount, permanently leaking the remainder.

The types are as stated: key->quotalen is unsigned short
(include/linux/key.h:216), prep->quotalen is size_t
(include/linux/key-type.h:37), and key_payload_reserve() computes
"int delta = (int)datalen - key->datalen" and then does
"key->quotalen += delta" (security/keys/key.c:376 and :396), so a charge
above USHRT_MAX would indeed truncate.  So the truncation is mechanically
correct.

It is not reachable by an unprivileged user, though.  key_payload_reserve()
checks the quota and returns -EDQUOT (key.c:392) *before* it touches
key->quotalen:

	security/keys/key.c:389
	if (delta > 0 &&
	    (key->user->qnbytes + delta > maxbytes || ...))
		ret = -EDQUOT;
	else {
		key->user->qnbytes += delta;
		key->quotalen += delta;
	}

For a non-root user maxbytes is key_quota_maxbytes = 20000
(security/keys/key.c:29).  The reserved amount for a single key cannot
exceed that: key_alloc() already charged desclen + 1 + def_datalen and set
key->quotalen to it (key.c:248, :272, :292), and the key_payload_reserve()
delta is bounded by the remaining quota (maxbytes - qnbytes).  After a
successful reserve the key's share of qnbytes is desclen + 1 +
prep->quotalen and it equals key->quotalen, so it is <= 20000 < USHRT_MAX.
The overflow value can never be committed; add_key() just fails with
-EDQUOT first.  In our testing, userspace sees errno 122 (EDQUOT) from
add_key() of a roughly 65000-byte PKCS#8 key, with no charge ever recorded
above 20000.

The root / raised-maxbytes case is also not a security boundary:

  - /proc/sys/kernel/keys/maxbytes is mode 0644 (security/keys/sysctl.c:26),
    i.e. only root can raise it, so "if maxbytes is raised" is itself a
    privileged action; root is not the boundary we are protecting.
  - Root does not need to raise that sysctl to pass the quota check.
    key_payload_reserve() selects maxbytes with
    uid_eq(key->user->uid, GLOBAL_ROOT_UID) (key.c:383), so the global
    root already uses key_quota_root_maxbytes = 25000000 (key.c:27).
    A user-namespace root is not GLOBAL_ROOT_UID and still uses
    key_quota_maxbytes = 20000.
  - asn1_ber_decoder() rejects a DER blob with datalen > 65535
    (lib/asn1_decoder.c:197, -EMSGSIZE), so a single key cannot grow
    without bound.  sizeof(*pub) + keylen can still exceed USHRT_MAX
    for a large but legal blob, so root (or a raised maxbytes) can hit
    the truncation.
  - The direction of the error is an over-charge, not a bypass.  qnbytes is
    credited with the full delta but debited with the truncated quotalen at
    key_put() (key.c:659), so the remainder simply stays charged against the
    same user, who hits EDQUOT sooner.  It cannot let anyone retain more than
    the accounted quota, and it does not weaken the check that protects
    unprivileged users.

> 2) Can this dynamically calculated payload size also overflow the 16-bit
>    limit of key->quotalen?

Same reasoning for the X.509 side.  The new expression is a sum over the
objects the payload actually retains (public key, parameters, signature,
authority key IDs and key IDs), and each is bounded by the same 65535-byte
DER limit.  For a non-root key the whole sum must satisfy the 20000-byte
quota check before key->quotalen is updated, so it cannot exceed USHRT_MAX
either.  As above, only root (or a root raising the sysctl) can get a single
key's charge past 65535, and the result is a self-inflicted quota
over-charge rather than any kind of bypass.

So we don't see a security issue here and would keep v1 as is.

That said, if you would prefer the byte accounting to stay self-consistent
even for root or a raised maxbytes, it could be handled by a small separate
hardening, e.g. having key_payload_reserve() reject a reservation larger
than USHRT_MAX (or widening key->quotalen to unsigned int).  I left that out
of this patch on purpose: it changes core quota semantics beyond the
reported bug and is not needed to close the unprivileged quota-accounting
bypass this series fixes.  Happy to send it as a follow-up if the
maintainers want it.

Thanks,
Yuqi Xu