-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Fixed Heap-buffer-overflow in parse_unix via clusterfuzz #9833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Do you know if something needs to be done to mark the clusterfuzz entry as fixed? |
|
|
||
| un->sun_family = AF_UNIX; | ||
| strcpy(un->sun_path, uri->path); | ||
| strncpy(un->sun_path, uri->path, sizeof(un->sun_path) - 1 /* null term'd */); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strncpy() is notoriously difficult to use in a safe way, because if the string is longer than the specified length, it does not copy the trailing NUL byte to the destination string. strlcpy() is a much safer alternative to strncpy(), but it's unfortunately not portable. :(
I assume that this misfeature of strncpy() is why you're using memset() above to initialize the whole struct. However, that seems a bit fragile, because a future developer looking at this code might think "the memset() isn't really necessary, because we're explicitly setting all of the fields below". An alternative would be to do something like this right after this line:
un->sun_path[sizeof(un->sun_path) - 1] = '\0';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was exactly why I was zeroing the memory. Good point though, setting the terminator explicitly is more robust.
Done.
| @@ -51,7 +51,8 @@ int parse_unix(grpc_uri *uri, grpc_resolved_address *resolved_addr) { | |||
| struct sockaddr_un *un = (struct sockaddr_un *)resolved_addr->addr; | |||
|
|
|||
| un->sun_family = AF_UNIX; | |||
| strcpy(un->sun_path, uri->path); | |||
| strncpy(un->sun_path, uri->path, sizeof(un->sun_path) - 1 /* null term'd */); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just return 0 if uri->path is too long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. Done.
a40662e to
d6d86b0
Compare
| @@ -49,12 +49,11 @@ | |||
|
|
|||
| int parse_unix(grpc_uri *uri, grpc_resolved_address *resolved_addr) { | |||
| struct sockaddr_un *un = (struct sockaddr_un *)resolved_addr->addr; | |||
|
|
|||
| const size_t maxlen = sizeof(un->sun_path); | |||
| if (strnlen(uri->path, maxlen) == maxlen) return 0; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this fail if the size is exactly maxlen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the size is exactly maxlen, we have a problem: the \0 would be one-past the end
| resolved_addr->len = strlen(un->sun_path) + sizeof(un->sun_family) + 1; | ||
|
|
||
| strcpy(un->sun_path, uri->path); | ||
| resolved_addr->len = strlen(un->sun_path) + maxlen + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you put maxlen here instead of sizeof(un->sun_family), where you probably actually meant to replace sizeof(un->sun_path).
Shouldn't this actually just be set to sizeof(struct sockaddr_un)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are too quick :) I noticed right after pushing, took me ~1 min to fix and re-push but that was already too late!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agh! hold on, push didn't go through...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, NOW. PTAL.
|
Yes, I noticed that seconds after sending that commit. Refresh :)
…On 23 February 2017 at 09:20, Mark D. Roth ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/core/ext/client_channel/parse_address.c
<#9833 (comment)>:
> @@ -49,12 +49,11 @@
int parse_unix(grpc_uri *uri, grpc_resolved_address *resolved_addr) {
struct sockaddr_un *un = (struct sockaddr_un *)resolved_addr->addr;
-
+ const size_t maxlen = sizeof(un->sun_path);
+ if (strnlen(uri->path, maxlen) == maxlen) return 0;
Won't this fail if the size is exactly maxlen?
------------------------------
In src/core/ext/client_channel/parse_address.c
<#9833 (comment)>:
> un->sun_family = AF_UNIX;
- strncpy(un->sun_path, uri->path, sizeof(un->sun_path) - 1 /* null term'd */);
- un->sun_path[sizeof(un->sun_path) - 1] = '\0';
- resolved_addr->len = strlen(un->sun_path) + sizeof(un->sun_family) + 1;
-
+ strcpy(un->sun_path, uri->path);
+ resolved_addr->len = strlen(un->sun_path) + maxlen + 1;
Looks like you put maxlen here instead of sizeof(un->sun_family), where
you probably actually meant to replace sizeof(un->sun_path).
Shouldn't this actually just be set to sizeof(struct sockaddr_un)?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9833 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAHVmaY5-bcFtL2PSPPYrZgVL2z5Fsr7ks5rfb_6gaJpZM4MJSIN>
.
|
d6d86b0 to
e2869fe
Compare
| resolved_addr->len = strlen(un->sun_path) + sizeof(un->sun_family) + 1; | ||
|
|
||
| strcpy(un->sun_path, uri->path); | ||
| resolved_addr->len = path_len + sizeof(un->sun_family) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think it's appropriate to use path_len here, since the size of the address is the size of the buffer, not the size of the string in the buffer (which may be smaller than the full size of the buffer).
Why are we not simply saying resolved_addr->len = sizeof(struct sockaddr_un)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the previously existing logic, but I think you are right: all other types of addresses populate len with that way. Changed.
| @@ -49,12 +49,12 @@ | |||
|
|
|||
| int parse_unix(grpc_uri *uri, grpc_resolved_address *resolved_addr) { | |||
| struct sockaddr_un *un = (struct sockaddr_un *)resolved_addr->addr; | |||
|
|
|||
| const size_t maxlen = sizeof(un->sun_path); | |||
| const size_t path_len = strnlen(uri->path, maxlen); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like the logic actually changed here. But upon further review, I think this is correct: if the NUL byte is at maxlen, strnlen() will return maxlen - 1, which is fine, and if the NUL byte is at maxlen + 1, we don't have enough room in the buffer, so we should fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
Fixes