Details

Reviewers

philip
asomers

Group Reviewers

Contributor Reviews (src)

Commits

rGf284bed200b0: geom_gate: ensure readprov is null-terminated

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 requested review of this revision.Aug 30 2021, 1:48 PM

jo_bruelltuete.com created this revision.

jo_bruelltuete.com added inline comments.Aug 30 2021, 1:54 PM

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.

jo_bruelltuete.com edited the summary of this revision. (Show Details)Aug 30 2021, 1:57 PM

jo_bruelltuete.com added a reviewer: philip.

oshogbo added a subscriber: oshogbo.Aug 30 2021, 2:27 PM

oshogbo added inline comments.

sys/geom/gate/g_gate.c
673	Shouldn't we verify this?

jo_bruelltuete.com added inline comments.Aug 30 2021, 3:21 PM

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.

jo_bruelltuete.com added inline comments.Aug 30 2021, 5:43 PM

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.

jo_bruelltuete.com added inline comments.Sep 1 2021, 11:50 PM

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...?

jmg added a subscriber: jmg.Sep 8 2021, 11:53 PM

In D31727#717074, @philip wrote:

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?

jo_bruelltuete.com added a reviewer: Contributor Reviews (src).Oct 27 2021, 11:59 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 27 2021, 11:59 PM

jo_bruelltuete.com added inline comments.Dec 28 2021, 2:24 AM