Page MenuHomeFreeBSD

ng_parse: disallow negative length for malloc
ClosedPublic

Authored by emaste on Nov 1 2022, 2:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 10, 5:38 PM
Unknown Object (File)
Fri, Oct 10, 5:38 PM
Unknown Object (File)
Fri, Oct 10, 5:38 PM
Unknown Object (File)
Fri, Oct 10, 5:38 PM
Unknown Object (File)
Fri, Oct 10, 12:32 PM
Unknown Object (File)
Fri, Sep 19, 6:02 AM
Unknown Object (File)
Sep 4 2025, 1:40 PM
Unknown Object (File)
Sep 3 2025, 10:39 PM

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emaste requested review of this revision.Nov 1 2022, 2:12 PM

just return, foff is not yet set

glebius requested changes to this revision.Nov 2 2022, 6:14 PM

Can we return unsigned value from ng_get_composite_len() instead?

This revision now requires changes to proceed.Nov 2 2022, 6:14 PM

Looking at 267334. IMHO, we should return the error for invalid message as close as possible to the userland. Don't let the invalid data travel this down.

Of course we should also check for num nonnegative and too large; there's probably more to be covered here too.

Can we return unsigned value from ng_get_composite_len() instead?

We should, but I think there are ABI considerations to take into account and we really need someone who understands netgraph well.

I'm going to commit this as an interim improvement. We can revisit ng_get_composite_len's return type and error handling later on.

This revision was not accepted when it landed; it landed in state Needs Revision.Nov 21 2024, 8:56 PM
This revision was automatically updated to reflect the committed changes.

I agree in principle with “as close as possible to userland”, but I don't see how you could move this check any closer to userland without either duplicating a non-trivial amount of code in several places or completely refactoring ng_parse.c.

More importantly though, I'm concerned by the lack of an upper bound on num. I wouldn't be surprised if it's possible to pass in a value that causes the multiplication in the malloc() call to overflow to a small positive number, allowing the allocation to succeed but leading to a buffer overflow further down the line.

I also dislike hiding the call to ng_get_composite_len() in the initialization of num. I would prefer to see it immediately before the range check.