fix(grpc): sanitize invalid UTF-8 in sockaddr conversion#5325
Open
arpitjain099 wants to merge 1 commit into
Open
fix(grpc): sanitize invalid UTF-8 in sockaddr conversion#5325arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
getSockaddr assigned raw map[string]string values straight into the proto3 string fields sun_path, sin_addr and sin6_addr without UTF-8 sanitization. A getsockname / sockaddr_t argument can carry non-UTF-8 bytes (for example an abstract AF_UNIX sun_path that begins with a NUL byte and holds binary data, or address fields with raw bytes). proto3 string fields require valid UTF-8, so proto.Marshal failed with "string field contains invalid UTF-8", which killed the StreamEvents gRPC stream and disconnected clients. The earlier UTF-8 hardening covered the other string paths in this file but missed getSockaddr. Route the three affected fields through the existing sanitizeStringForProtobuf helper so the message marshals cleanly. Valid UTF-8 values are returned unchanged. Add Test_getSockaddr_InvalidUTF8 feeding AF_UNIX, AF_INET and AF_INET6 sockaddr arguments with invalid UTF-8 bytes through the conversion and asserting proto.Marshal succeeds while a valid sun_path is preserved. Fixes aquasecurity#5292 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Member
|
@arpitjain099 thank you for contributing. @trvll do you mind reviewing this? Cheers. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. Explain what the PR does
Fixes #5292.
getSockaddrwas the one place left in the gRPC event-to-proto conversion that assigned rawmap[string]stringvalues straight into proto3stringfields without UTF-8 sanitization. Agetsockname/sockaddr_targument can carry non-UTF-8 bytes:sun_pathof an abstractAF_UNIXsocket begins with a NUL byte and can hold arbitrary binary bytes (uninitialized tail of the 108-bytesun_pathbuffer, abstract namespace names, etc.).sin_addr/sin6_addrcan likewise contain raw bytes that are not valid UTF-8.proto3
stringfields must be valid UTF-8, soproto.Marshalrejected these values withstring field contains invalid UTF-8. That error propagated out ofStreamEvents, killing the stream and disconnecting clients (grpcurl and others). The reporter confirmed via bisection that simply havinggetsocknamein the event list triggered the crash.The earlier UTF-8 hardening (#4894, #4916) sanitized nearly every other string path in this file but missed
getSockaddr. This change closes that gap by routing the three affected fields through the existingsanitizeStringForProtobufhelper, matching the convention already used everywhere else inevent_data.go.2. Explain how to test it
Unit test added in
pkg/server/grpc/event_data_test.go(Test_getSockaddr_InvalidUTF8). It feedsAF_UNIX,AF_INET, andAF_INET6sockaddr arguments whose address/path fields contain invalid UTF-8 bytes throughgetEventData, then callsproto.Marshalon the resultingpb.Event.Before this change the marshal fails:
After it, marshal succeeds, the sanitized fields are valid UTF-8, and a valid
sun_pathlike/tmp/socketis returned byte-for-byte unchanged.3. Other comments
The sanitization is lossy only for already-malformed input: invalid byte sequences are dropped (the same behavior
sanitizeStringForProtobufalready applies to every other string field), while valid UTF-8 is preserved exactly. An alternative would be to expose the raw bytes through a dedicatedbytesfield onSockAddr, but that is an API/proto change; string sanitization keeps the wire format and existing consumers unchanged and is consistent with the rest of this file.