Opened 10 years ago

Closed 6 years ago

#3255 closed bug (fixed)

malformed unix socket filename

Reported by: kaliber Owned by: pdziepak
Priority: normal Milestone: R1
Component: Network & Internet Version: R1/pre-alpha1
Keywords: Cc: michaelvoliveira@…
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Attached code creates 'socket_test_filenam\302K\200\C' instead of 'socket_test_filename'. It works on linux.

gcc socket.c -lnetwork

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/un.h>

#define SOCK_PATH "socket_test_filename"

int main(void)
{
    int s, len;
    struct sockaddr_un local;
    char str[100];

    if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
        perror("socket");
        exit(1);
    }

    local.sun_family = AF_UNIX;
    strcpy(local.sun_path, SOCK_PATH);
    unlink(local.sun_path);
    len = strlen(local.sun_path) + sizeof(local.sun_family);
    if (bind(s, (struct sockaddr *)&local, len) == -1) {
        perror("bind");
        exit(1);
    }
    return 0;
}



Attachments (2)

0001-Add-length-parameter-to-bind-and-connect.patch (18.5 KB) - added by jscipione 6 years ago.
NUL terminate unix socket address in UnixEndpoint.cpp
0001-vfs-Allow-non-null-terminated-UNIX-socket-pathnames.patch (1.7 KB) - added by pdziepak 6 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by axeld

Resolution: invalid
Status: newclosed

The code is incorrect anyway, as you make assumptions about the contents of sockaddr_un - on Haiku, it also has a length field that you miss on the length computation, causing the terminating null byte to be cut off.

Please always just use sizeof(sockaddr_un) for the length.

comment:2 Changed 10 years ago by kaliber

Resolution: invalid
Status: closedreopened

Oh, my mistake, sorry. The correct length should be: len = offsetof(struct sockaddr_un, sun_path) + strlen(local.sun_path)

But seems there is a bug in haiku too. Here is more information: http://bugs.freedesktop.org/show_bug.cgi?id=19445

comment:3 Changed 10 years ago by axeld

Component: - GeneralNetwork & Internet
Owner: changed from axeld to bonefish
Status: reopenednew

Indeed, that looks like a bug, thanks for the note and persistence! :-) BTW it's called "Haiku", not "HaikuOS".

comment:4 Changed 9 years ago by michaelvoliveira

Cc: michaelvoliveira@… added

Maybe our sys/un.h implementation differs from Posix one?

Here http://www.opengroup.org/onlinepubs/009695399/basedefs/sys/un.h.html

here is the opensolaris implementation http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/common/sys/un.h

comment:5 Changed 9 years ago by bonefish

I guess I would still consider the sample code invalid. bind()'s address_len parameter "Specifies the length of the sockaddr structure pointed to by the address argument." I would interpret that to mean sizeof(sockaddr_un) in this case. The sun_len field doesn't have to be set. It will be ignored anyway (not doing so would break POSIX compliant programs).

That being said it certainly wouldn't harm to consider the address size passed as a limit. Currently the address is copied into a a sockaddr_storage and strnlen(..., sizeof(sockaddr_un)) is used to determine the actual length of the address. Furthermore the value the unix module writes to the sun_len field actually includes the terminating null, which other OSs don't include. And finally the SUN_LEN() macro could be added to <sys/un.h> (several other OSs have it).

To sum it up, while I think the reported problem is invalid I'll leave the ticket open for sake of the related issues/features.

comment:6 Changed 6 years ago by akira

I found this issue while I compare various Unix domain socket implementation. https://github.com/akr/socket-test

I agree that the sample code is not POSIX compliant because POSIX defines sun_path field as a pathname and pathname is NUL-terminated by definition.

However, specifying a length without NUL is a traditional way to use Unix domain socket address.

For example, a 4.3BSD document describes that the length doesn't contains NUL.

http://www.tuhs.org/Archive/4BSD/Distributions/4.3BSD/usr.tar.gz doc/ps1/08.ipc/2.t

       #include <sys/un.h>
        ...
       struct sockaddr_un addr;
        ...
       strcpy(addr.sun_path, "/tmp/foo");
       addr.sun_family = AF_UNIX;
       bind(s, (struct sockaddr *) &addr, strlen(addr.sun_path) +
           sizeof (addr.sun_family));

  Note  that  in determining the size of a UNIX domain address
  null bytes are not counted, which is why strlen is used.

4.4BSD document also has similar description. (It adds sizeof(addr.sun_len) though) http://docs.freebsd.org/44doc/psd/21.ipc/paper.pdf

So Haiku is not friendly for traditional programs.

Also, out-of-bounds read is not a good programming practice. CWE-125: Out-of-bounds Read http://cwe.mitre.org/data/definitions/125.html

What interesting about Haiku is Out-of-bounds Read is happen on copied socket address. This means NUL just after specified length cannot prevent the problem.

Changed 6 years ago by jscipione

NUL terminate unix socket address in UnixEndpoint.cpp

comment:7 Changed 6 years ago by jscipione

Has a Patch: set

comment:8 Changed 6 years ago by pdziepak

The patch I've just attached solves the compatibility problem with applications expecting bind() and connect() to accept non-null-terminated strings. Since it affects only system calls anyone using UNIX sockets in kernel mode (probably an unlikely scenario) would still need to pass a null-terminated string.

The example code in this ticket still truncates the last 'e' in the path since it incorrectly assumes that sockaddr_un contains no other fields than sun_family and sun_path. I don't think there is much we can do about it.

comment:9 Changed 6 years ago by pdziepak

Owner: changed from bonefish to pdziepak
Status: newassigned

Patch commited in hrev46000.

Since the issue with null-termination is now solved and the problems resulting from the presence of sa_len are not really Haiku fault I guess this ticket may be closed now.

comment:10 Changed 6 years ago by pdziepak

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.