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

debugger: avoid off-by-one overflow in debugger socket path #1753

Merged
merged 2 commits into from May 4, 2018

Conversation

Projects
None yet
5 participants
@avsm
Copy link
Member

avsm commented May 2, 2018

Modern gcc have a -Werror=stringop-truncation warning that tells us that the strncpy used to copy the CAML_DEBUG_SOCKET value from the environment misses out the space for a terminating NUL.

Rather than silently truncate the path, this patch raises an exception if the path is too long, and also fixes the off-by-one in the strncpy invocation.

This (along with #1752) fix compilation on Fedora 28 with trunk.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 2, 2018

@avsm

This comment has been minimized.

Copy link
Member Author

avsm commented May 2, 2018

@shindere yes, it would be good to add some more of these options to CFLAGS, but I'd rather do that as a separate PR. A quick glance through gcc8's docs show quite a lot of new warnings, and clang also has a different set.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 2, 2018

strncpy(sock_addr.s_unix.sun_path, address,
sizeof(sock_addr.s_unix.sun_path));
sizeof(sock_addr.s_unix.sun_path) - 1);

This comment has been minimized.

@stedolan

stedolan May 2, 2018

Contributor

There is still an off-by-one error here: if strlen(address) == sizeof(sock_addr.s_unix.sun_path) - 1, then the fatal error won't trigger and strncpy will fail to write a null terminator. I think the check should be a_len >= sizeof(sock_addr.s_unix.sun_path) - 1.

This comment has been minimized.

@avsm

avsm May 2, 2018

Author Member

But strlen(address) == sizeof(sock_addr.s_unix.sun_path) - 1 should be a valid length since it fits into the buffer. Perhaps we should just explicitly set the final byte to 0 -- I'd assumed that the buffer was zeroed already and so the strncpy didn't need to set the final byte to 0.

This comment has been minimized.

@stedolan

stedolan May 2, 2018

Contributor

Yeah, the issue is that strncpy won't write the final zero. Setting it to 0 explicitly works.

This comment has been minimized.

@avsm

avsm May 2, 2018

Author Member

Thanks, pushed that change and rebased

@avsm avsm force-pushed the avsm:debug-socket-len branch from 483e4c4 to 268aa90 May 2, 2018

debugger: avoid off-by-one overflow in debugger socket path
Modern gcc have a `-Werror=stringop-truncation` warning that tells
us that the strncpy used to copy the CAML_DEBUG_SOCKET value from
the environment misses out the space for a terminating NUL.

Rather than silently truncate the path, this patch raises an
exception if the path is too long, and also fixes the off-by-one
in the strncpy invocation.

with feedback from Stephen Dolan

@avsm avsm force-pushed the avsm:debug-socket-len branch from 268aa90 to 8cea98c May 2, 2018

@avsm

This comment has been minimized.

Copy link
Member Author

avsm commented May 3, 2018

I'm just working on the Fedora 28 CI tests for opam -- I think this is the last changeset that would be useful in 4.07.0 in order to not require local patches to compile OCaml for the opam build infrastructure.

@damiendoligez damiendoligez added the bug label May 3, 2018

@damiendoligez damiendoligez added this to the 4.07 milestone May 3, 2018

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

A couple of comments below showing that I still don't understand socket programming after all those years...

strncpy(sock_addr.s_unix.sun_path, address,
sizeof(sock_addr.s_unix.sun_path));
sizeof(sock_addr.s_unix.sun_path) - 1);
sock_addr.s_unix.sun_path[sizeof(sock_addr.s_unix.sun_path) - 1] = '\0';
sock_addr_len =

This comment has been minimized.

@xavierleroy

xavierleroy May 3, 2018

Contributor

Many authors just memset the whole sockaddr_un structure to zero before initializing the fields of interest. I don't know if it's just superstition or a more robust way of having the path zero-terminated.

This comment has been minimized.

@avsm

avsm May 4, 2018

Author Member

Agreed -- the sock_addr value here should be initialised to 0 already as its in the BSS, so this zero termination is to be doubly sure (and in case the debugger can somehow be initialised twice, although that should never happen in normal use).

sock_addr_len =
((char *)&(sock_addr.s_unix.sun_path) - (char *)&(sock_addr.s_unix))
+ strlen(address);
+ a_len;
#else

This comment has been minimized.

@xavierleroy

xavierleroy May 3, 2018

Contributor

man 7 unix on Linux suggests to simply use sizeof(struct sockaddr_un) as the length parameter. Maybe the original code was just wrong trying to track the actual length of the path.

This comment has been minimized.

@avsm

avsm May 4, 2018

Author Member

On Linux yes, but it wasn't clear to me what the behaviour is elsewhere. That man page says:

The addrlen argument that describes the enclosing sockaddr_un
          structure should have a value of at least:

              offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1

          or, more simply, addrlen can be specified as sizeof(struct sock‐
          addr_un).

which led me to conclude that the existing length calculation is more precise and possibly what used to happen before it became acceptable to just pass the whole (presumably NUL padded) struct.

I think I'd prefer to merge this minimal patch to fix the overflow, and then peer more closely at the use of globals and various socket options and perhaps add some test cases in a future post-4.07 PR :-)

This comment has been minimized.

@xavierleroy

xavierleroy May 4, 2018

Contributor

I fully agree with your conclusions. I'm merging in trunk and in 4.07 right now.

@xavierleroy xavierleroy merged commit 698f648 into ocaml:trunk May 4, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

xavierleroy added a commit that referenced this pull request May 4, 2018

debugger: avoid off-by-one overflow in debugger socket path (#1753)
Modern gcc have a `-Werror=stringop-truncation` warning that tells
us that the strncpy used to copy the CAML_DEBUG_SOCKET value from
the environment misses out the space for a terminating NUL.

Rather than silently truncate the path, this patch raises an
exception if the path is too long, and also fixes the off-by-one
in the strncpy invocation.

with feedback from Stephen Dolan
@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented May 4, 2018

698f648 on trunk, 8121e51 on 4.07 branch

@avsm avsm deleted the avsm:debug-socket-len branch May 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.