xfrm/sa: bounds-check algo key length and ctx/bmp lengths in parser#456
Open
rawrmonster17 wants to merge 1 commit into
Open
xfrm/sa: bounds-check algo key length and ctx/bmp lengths in parser#456rawrmonster17 wants to merge 1 commit into
rawrmonster17 wants to merge 1 commit into
Conversation
xfrmnl_sa_parse() computes allocation and copy sizes from length fields
embedded in the netlink attribute payload (alg_key_len, ctx_len,
bmp_len) without comparing them against the actual attribute length
returned by nla_len(). The nla_policy for these attributes sets only
.minlen, not .maxlen, so nlmsg_parse() cannot enforce an upper bound.
This means a caller supplying an attribute whose payload is shorter
than the claimed key/context/bitmap length causes:
- An OOB heap read: the calloc destination is sized to the attacker-
controlled value, and the subsequent memcpy() reads that many bytes
past the end of the (shorter) source attribute, dragging adjacent
heap contents into sa->aead/auth/crypt/comp/sec_ctx/replay_state_esn.
- An OOB heap write on the XFRMA_ALG_AUTH path (line 905): the old
strcpy(sa->auth->alg_name, auth->alg_name) had no NUL-termination
guarantee on the 64-byte source field, allowing a write past the
alg_name destination.
- An integer overflow on the XFRMA_REPLAY_ESN_VAL path: bmp_len is
a uint32_t, and sizeof(uint32_t)*bmp_len can wrap to a small value,
producing an undersized allocation followed by a large copy.
Fix all seven affected attribute blocks by validating the payload-
embedded length field against nla_len() before computing the allocation
size:
key_bytes = (alg_key_len + 7) / 8;
if (key_bytes > nla_len(tb[...]) - sizeof(header struct))
return -NLE_INVAL;
For XFRMA_ALG_AUTH, replace strcpy with a bounded memcpy into the
calloc'd (zero-initialised) alg_name field.
For XFRMA_REPLAY_ESN_VAL, add an explicit overflow guard before the
bmp_len multiply.
Normal well-formed SA messages from the kernel are unaffected: the
validated key_bytes / bmp_bytes values are identical to what the old
code computed whenever the attribute is correctly sized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
xfrmnl_sa_parse()computes allocation and copy sizes from length fieldsembedded in the netlink attribute payload (
alg_key_len,ctx_len,bmp_len)without comparing them against the actual attribute length returned by
nla_len().The
nla_policyfor these attributes sets only.minlen, never.maxlen, sonlmsg_parse()cannot enforce an upper bound on attribute payload size. Thismeans a caller supplying an attribute whose payload is shorter than the claimed
key/context/bitmap length causes:
OOB heap read (all algo/sec_ctx/ESN paths): the
callocdestination issized to the attacker-controlled value, and
memcpy()reads that many bytespast the end of the (shorter) source attribute, dragging adjacent heap contents
into
sa->aead/auth/crypt/comp/sec_ctx/replay_state_esn.OOB heap write (XFRMA_ALG_AUTH, line 905): the old
strcpy(sa->auth->alg_name, auth->alg_name)had no NUL-terminationguarantee on the 64-byte source field, allowing a write past the
alg_namedestination in the freshly allocated
sa->auth.Integer overflow (XFRMA_REPLAY_ESN_VAL):
bmp_lenisuint32_t, andsizeof(uint32_t) * bmp_lencan wrap to a small value, producing anundersized allocation followed by a large
memcpy.Fix
For every variable-length attribute block, validate the payload-embedded length
field against
nla_len()before computing the allocation size:This pattern is applied to all seven affected blocks:
XFRMA_ALG_AEAD,XFRMA_ALG_AUTH_TRUNC,XFRMA_ALG_AUTH,XFRMA_ALG_CRYPT,XFRMA_ALG_COMP,XFRMA_SEC_CTX,XFRMA_REPLAY_ESN_VAL.For
XFRMA_ALG_AUTH,strcpyis replaced with a boundedmemcpyinto thecalloc'd (zero-initialised)alg_namefield:For
XFRMA_REPLAY_ESN_VAL, an explicit overflow guard is added before thebmp_lenmultiply:Normal well-formed SA messages from the kernel are unaffected: the validated
key_bytes/bmp_bytesvalues are identical to what the old code computedwhenever the attribute is correctly sized.
Context
This was noted in issue #453, which identified the
strcpyon line 905. Thispatch addresses the full family of unbounded copies in the same function.
The setter paths were hardened in an earlier PR; this hardens the corresponding
parser/receive path.