Bug 267334 - ng_parse_composite() passes length to malloc() without check
Summary: ng_parse_composite() passes length to malloc() without check
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Ed Maste
URL: https://reviews.freebsd.org/D37229
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-25 13:10 UTC by Robert Morris
Modified: 2024-11-21 20:56 UTC (History)
3 users (show)

See Also:


Attachments
provoke a crash in netgraph ng_parse_composite() (1.24 KB, text/plain)
2022-10-25 13:10 UTC, Robert Morris
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2022-10-25 13:10:36 UTC
Created attachment 237614 [details]
provoke a crash in netgraph ng_parse_composite()

In netgraph/ng_parse.c's ng_parse_composite():

        const int num = ng_get_composite_len(type, start, buf, ctype);
        ...;
        foff = malloc(num * sizeof(*foff), M_NETGRAPH_PARSE, M_NOWAIT | M_ZERO);

ng_get_composite_len() reads num from the message, and it's used
without a sanity check. If it's negative, malloc() either page
faults or (with INVARIANTS) fails MPASS(size > 0).

I've attached a demo that triggers the crash with an NGM_BINARY2ASCII
NGM_LISTNAMES control message:

# cc ng12a.c -lnetgraph
# ./a.out
panic: Fatal page fault at 0xffffffc000340f48: 0x27fffd000807460
panic() at panic+0x2a
page_fault_handler() at page_fault_handler+0x1d6
do_trap_supervisor() at do_trap_supervisor+0x74
cpu_exception_handler_supervisor() at cpu_exception_handler_supervisor+0x70
--- exception 13, tval = 0x27fffd000807460
vmem_alloc() at vmem_alloc+0x76
kmem_malloc_domain() at kmem_malloc_domain+0x52
kmem_malloc_domainset() at kmem_malloc_domainset+0x36
malloc_large() at malloc_large+0x2a
malloc() at malloc+0xf8
ng_parse_composite() at ng_parse_composite+0x64
ng_array_getDefault() at ng_array_getDefault+0x24
ng_get_composite_elem_default() at ng_get_composite_elem_default+0x74
ng_unparse_composite() at ng_unparse_composite+0x17e
ng_struct_unparse() at ng_struct_unparse+0x42
ng_unparse() at ng_unparse+0x30
ng_generic_msg() at ng_generic_msg+0x96e
ng_apply_item() at ng_apply_item+0xf6
ng_snd_item() at ng_snd_item+0x1bc
ngc_send() at ngc_send+0x260
sosend_generic() at sosend_generic+0x384
sosend() at sosend+0x68
kern_sendit() at kern_sendit+0x170
sendit() at sendit+0x9c
sys_sendto() at sys_sendto+0x40
syscallenter() at syscallenter+0xec
ecall_handler() at ecall_handler+0x18
do_trap_user() at do_trap_user+0xf6
cpu_exception_handler_user() at cpu_exception_handler_user+0x72
--- syscall (133, FreeBSD ELF64, sys_sendto)
Comment 1 commit-hook freebsd_committer freebsd_triage 2024-11-21 20:56:19 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=ae4f39464c61182c2bdfd3cc61594f8707b3daf0

commit ae4f39464c61182c2bdfd3cc61594f8707b3daf0
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2022-11-01 14:01:29 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2024-11-21 20:53:04 +0000

    ng_parse: disallow negative length for malloc

    This is an interim robustness improvement; further improvements as
    described in the PR and/or Phabricator review are still needed.

    PR:             267334
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D37229

 sys/netgraph/ng_parse.c | 2 ++
 1 file changed, 2 insertions(+)
Comment 2 Ed Maste freebsd_committer freebsd_triage 2024-11-21 20:56:35 UTC
I've committed an interim improvement. Gleb raises valid points in the linked Phabricator review, so don't consider this issue closed.