Skip to content
Permalink
Browse files Browse the repository at this point in the history
fix strncpy call to avoid ASAN violation
Ensure we're only reading to the size of the smallest buffer, since
they're both on the stack and could potentially overlap. Overlapping is
defined as ... undefined behavior. I've looked through all available
implementations of strncpy and they still only copy from the first \0
found.

We'll also never read past the end of sun_path since we _supply_
sun_path with a proper null terminator.
  • Loading branch information
dormando committed Aug 29, 2019
1 parent 938154d commit 554b566
Showing 1 changed file with 20 additions and 2 deletions.
22 changes: 20 additions & 2 deletions memcached.c
Expand Up @@ -3463,6 +3463,7 @@ static inline void get_conn_text(const conn *c, const int af,
addr_text[0] = '\0';
const char *protoname = "?";
unsigned short port = 0;
size_t pathlen = 0;

switch (af) {
case AF_INET:
Expand All @@ -3488,10 +3489,27 @@ static inline void get_conn_text(const conn *c, const int af,
break;

case AF_UNIX:
// this strncpy call originally could piss off an address
// sanitizer; we supplied the size of the dest buf as a limiter,
// but optimized versions of strncpy could read past the end of
// *src while looking for a null terminator. Since buf and
// sun_path here are both on the stack they could even overlap,
// which is "undefined". In all OSS versions of strncpy I could
// find this has no effect; it'll still only copy until the first null
// terminator is found. Thus it's possible to get the OS to
// examine past the end of sun_path but it's unclear to me if this
// can cause any actual problem.
//
// We need a safe_strncpy util function but I'll punt on figuring
// that out for now.
pathlen = sizeof(((struct sockaddr_un *)sock_addr)->sun_path);
if (MAXPATHLEN <= pathlen) {
pathlen = MAXPATHLEN - 1;
}
strncpy(addr_text,
((struct sockaddr_un *)sock_addr)->sun_path,
sizeof(addr_text) - 1);
addr_text[sizeof(addr_text)-1] = '\0';
pathlen);
addr_text[pathlen] = '\0';
protoname = "unix";
break;
}
Expand Down

4 comments on commit 554b566

@setharnold
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unix(7) sockets have three different types of names:

  • names in the filesystem, normal nul termination
  • abstract names, the first byte is a nul
  • unnamed sockets, no name at all, and sun_path shouldn't be inspected at all

Are unix sockets of the other two types reachable by this code? (and, perhaps, was the asan warning about writing a nul before the buffer starts?)

Thanks

@dormando
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just path type. So far as I could tell the ASAN violation is due to optimized forms of strncpy examining chunks of bytes at a time while scanning for a null. I wasn't able to personally reproduce.

@setharnold
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reply; I'm not too surprised that strncpy might search eight or sixteen bytes at a go. UB is an interesting twist I didn't expect here.

Thanks

@dormando
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup... just to clarify for any future readers: we only read back sun_path from instances where a string has been copied to it, and we insert the null byte before copying. So the OS would have to lose the byte to cause any kind of direct bug.

It also requires that memc be clamped to localhost via a unix socket and the sun_path input is controlled via a commandline argument. Further, on linux at least long paths are truncated to significantly less than sun_path (though I didn't track down the detail here).

The problem is we had two (overlapping in the stack) buffers and strncpy was given the len restriction of the larger buffer. Hence original comment; they'll read 8 bytes at a time up until < 8 bytes remain in the len argument.

Please sign in to comment.