Page MenuHomeFreeBSD

geom_gate: ensure readprov is null-terminated
ClosedPublic

Authored by jo_bruelltuete.com on Aug 30 2021, 1:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 24 2024, 7:58 PM
Unknown Object (File)
Feb 19 2024, 7:02 PM
Unknown Object (File)
Feb 5 2024, 9:18 AM
Unknown Object (File)
Feb 2 2024, 5:23 PM
Unknown Object (File)
Jan 30 2024, 5:12 PM
Unknown Object (File)
Jan 30 2024, 5:12 PM
Unknown Object (File)
Jan 30 2024, 5:12 PM
Unknown Object (File)
Jan 17 2024, 11:44 PM
Subscribers

Details

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
rG FreeBSD src repository
Lint
Lint Skipped
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?

sys/geom/gate/g_gate.c
542

this looks fishy at first glance: assigning a stack pointer. but it's temporarily only.
will add a comment.

LGTM

sys/geom/gate/g_gate.c
471

NAME_MAX does not include the trailing nul.

679

Style: wrap it to 80 cols.

This revision is now accepted and ready to land.Jan 2 2022, 3:57 AM
This revision now requires review to proceed.Jan 3 2022, 12:27 AM
This revision is now accepted and ready to land.Jan 3 2022, 12:43 AM