Skip to content
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

Buffer overflows #13

Closed
ryandesign opened this issue Oct 2, 2019 · 2 comments · Fixed by #14
Closed

Buffer overflows #13

ryandesign opened this issue Oct 2, 2019 · 2 comments · Fixed by #14
Assignees

Comments

@ryandesign
Copy link
Contributor

When compiling the latest master on macOS High Sierra, clang points out a couple buffer overflows in your code:

tcp_subr.c:341:4: warning: '__builtin___strncpy_chk' will always overflow destination buffer [-Wbuiltin-memcpy-chk-size]
                        strncpy(addr.sun_path,path,UNIX_PATH_MAX);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/secure/_string.h:124:3: note: expanded from macro 'strncpy'
                __builtin___strncpy_chk (dest, __VA_ARGS__, __darwin_obsz (dest))
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Here, it looks like you're copying UNIX_PATH_MAX bytes to the sun_path field of a struct sockaddr_un, and in tcp2unix.h you've defined UNIX_PATH_MAX as 108, which as far as I can tell is correct for Linux systems, but based on cursory research, it seems that different operating systems use different sizes for this field, down to as small as 92 bytes. On my macOS system, the sun_path field's length is 104 bytes.

vder_packet.c:207:5: warning: '__builtin___memcpy_chk' will always overflow destination buffer [-Wbuiltin-memcpy-chk-size]
                                memcpy(foot, footprint(packet), sizeof(struct iphdr) + 8);
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/secure/_string.h:62:3: note: expanded from macro 'memcpy'
                __builtin___memcpy_chk (dest, __VA_ARGS__, __darwin_obsz0 (dest))
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

In this case, I think the problem is that you're allocating the variable foot based on the size of a pointer rather than the size of the struct that the pointer points to.

danielinux added a commit that referenced this issue Oct 6, 2019
danielinux added a commit that referenced this issue Oct 6, 2019
@rd235 rd235 closed this as completed in #14 Oct 8, 2019
@ryandesign
Copy link
Contributor Author

What about the sun_path length issue? One solution would be to reduce UNIX_PATH_MAX to 92, even if Linux and some other systems can use larger values. I don't know how inconvenient that would be for users of those systems. I don't know if there's a way to detect what the maximum length of sun_path is.

@danielinux danielinux reopened this Oct 8, 2019
@danielinux
Copy link
Member

@ryandesign I see the problem, but I don't have a Mac to properly test the fix. Would you be so kind to review my PR that aims to fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants