sensors: harden string parsing from BPF events #1276
Conversation
| // If no size then path walk was not possible and file was | ||
| // either a mount point or not a "file" at all which can | ||
| // happen if running without any filters and kernel opens an | ||
| // anonymous inode. For this lets just report its on "/" all | ||
| // though pid filtering will mostly catch this. |
There was a problem hiding this comment.
I'm not sure I get this comment so maybe we should not apply this for gt.GenericFileType, gt.GenericFdType, gt.GenericKiocb: and case gt.GenericPathType:. I just reestablished it to have exactly the same behavior.
There was a problem hiding this comment.
It looks like the case I'm trying to handle is the anonymous file case where the value returned here is empty. I guess its fine to do for all the other cases as well. What we need to be sure is we report something so the logs at least have an entry.
There was a problem hiding this comment.
ok I see, I just have one last question, in the patched that introduced that you wrote:
When using fd_install hook to monitor program open* if the open is against
the mount then our path walking logic will return the size=0. The user
side then interprets this as an error and prints an ugly warning.
And the thing I wonder is, if the size is indeed 0 and it's correctly writing a 4 bytes 0, decoding should be okay and not error:
tetragon/bpf/process/types/basic.h
Line 412 in b2e3858
But I guess you encountered the error? Because the only situation I see that could trigger this would be that BPF sends a message less than 4 bytes in total for that arg, thus we would get an EOF.
There was a problem hiding this comment.
hmm that patch is from some long ago time I don't recall what I did. If sending a 4B zerod arg is the right thing lets do that. A lot of code has been fixed/moved around probably from when I wrote that.
There was a problem hiding this comment.
Ok maybe this is never triggered anymore
mtardy
force-pushed
the
pr/mtardy/fuzz-handle-string
branch
from
cf55f07
to
f51ab2d
Compare
Initially found this issue by trying to fuzz parts of the handler. You can run the fuzzer with the following command: `go test -fuzz=Fuzz_parseString -run=Fuzz_parseString ./pkg/sensors/tracing/` Also added some unit test cases under `Test_parseString`. Previous parsing had flaws that could make Tetragon panic or be OOM killed in certain circumstances. In reality, the string size was already bound by BPF copy_string function to 1024 but it could have theoretically returned a size of -2 as an error code which would have made it panic. Also remove some code duplication that was rewriting the string parsing instead of using the common function. Signed-off-by: Mahe Tardy <[email protected]>
mtardy
force-pushed
the
pr/mtardy/fuzz-handle-string
branch
from
f51ab2d
to
94faed1
Compare
| t.Errorf("unexpected error %v", err) | ||
| } | ||
| if out != "pizza" { | ||
| t.Errorf("got %q, want %q", out, "pizza") |
| err = binary.Read(r, binary.LittleEndian, &outputStr) | ||
|
|
||
| // limit the size of the string to avoid huge memory allocation and OOM kill in case of issue | ||
| if size > maxStringSize { |
There was a problem hiding this comment.
This check is defensive right? We should not get such a string size from the BPF kernel program?
There was a problem hiding this comment.
Yes see MAX_STRING here
tetragon/bpf/process/types/basic.h
Line 447 in 3f0e05b
|
I'm happy with this. One question though just to be sure I understood this correctly. |
Initially found this issue by trying to fuzz parts of the handler. You can run the fuzzer with the following command:
go test -fuzz=Fuzz_parseString -run=Fuzz_parseString ./pkg/sensors/tracing/Also added some unit test cases underTest_parseString.Previous parsing had flaws that could make Tetragon panic or be OOM killed in certain circumstances. In reality, the string size was already bound by BPF copy_string function to 1024 but it could have theoretically returned a size of -2 as an error code which would have made it panic.
Also remove some code duplication that was rewriting the string parsing instead of using the common function.