ng_parse: disallow negative length for malloc

[emaste]

Summary

PR:             267334
Reported by:    Robert Morris <[e-mail]>
Sponsored by:   The FreeBSD Foundation

[emaste]

just return, foff is not yet set

[glebius]

Can we return unsigned value from ng_get_composite_len() instead?

[glebius]

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.

[emaste]

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

[emaste]

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.

[emaste]

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.

[des]

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.

[des]

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.