Page MenuHomeFreeBSD

geom_gate: ensure readprov is null-terminated
Needs ReviewPublic

Authored by jo_bruelltuete.com on Aug 30 2021, 1:48 PM.

Details

Reviewers
philip
Summary

With crafted input to the G_GATE_CMD_CREATE ioctl, geom_gate can be made to print kernel memory to the system console, potentially revealing sensitive data from whatever was previously in that memory page.

But but but: this is a case of the sys admin misconfiguring, and you'd need root privileges to do this.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

jo_bruelltuete.com created this revision.
sys/geom/gate/g_gate.c
471

should this be NAME_MAX+1 instead?
Does NAME_MAX include the null-terminator or not? https://www.gnu.org/software/libc/manual/html_node/Limits-for-Files.html says it does not include it but then that page is for linux libc... so not sure.

551

see above. if NAME_MAX does include the null-terminator then a simpler version is to just write \0 to the last byte, no need to copy.

oshogbo added inline comments.
sys/geom/gate/g_gate.c
673

Shouldn't we verify this?

sys/geom/gate/g_gate.c
673

Do you mean to check the return value of snprintf?
The number of characters that would have been printed into output doesn't really tell us much at this point. We know the input is either 1 char long, or 255 legitimate chars, or unbounded potentially-nasty.

Actually thats a good point! snprintf is not a good solution here!
At least snprintf(3) man page suggests that it will keep scanning for a null-terminator. Which means we still have a kernel out of bounds read. Bad.

I suggest a straight forward fixed size memcopy with a single write of \0 to the last byte. if there was an earlier \0 great, if there was not then we'd have done that now.

My preferred fix would be to make all string buffers in https://github.com/freebsd/freebsd-src/blob/main/sys/geom/gate/g_gate.h#L118 NAME_MAX+1 and enforce the last byte to be the null terminator.
But that'd be a ioctl interface change and an ABI change? Frowned upon?
Also this mainly affects hastd, which I cannot test at the moment.

Try to avoid out-of-bounds reads that can happen with other str*cpy functions.

sys/geom/gate/g_gate.c
527

this is prob the simplest and safest.
stpncpy may have helped avoid writing unnecessary zeros but that doesnt seem to be available. also this is not a perf critical path, so play it safe.

sys/geom/gate/g_gate.c
509

missed one, dang.
same problem: untrusted input from user mode triggering out-of-bounds read (and crash?).

I wonder if this wants some abstraction in the form of a function rather than chasing every str*cpy...?

I wonder if this wants some abstraction in the form of a function rather than chasing every str*cpy...?

I'd feel better about adding a new function if there's a way to properly unit test it (and other functions here). Is there some equivalent of gtest for kernel? Or compile all this stuff into a user mode gtest binary?