Bug #2673
closede1_line socket has no/wrong path length check
0%
Description
From recent commit https://gerrit.osmocom.org/#/c/4213/ it starts checking with strlcpy() that file path is at least PATH_MAX. Even though that fixes a possible overflow, it is still wrong because a unix socket patch is at most 108 characters, which means if a larger path is passed, it will be truncated, and it can create problems (such as the truncated file finished truncated in "/" or a directory already existing in the path).
For more info see https://stackoverflow.com/questions/34829600/why-is-the-maximal-path-length-allowed-for-unix-sockets-on-linux-108 and "man 7 unix":
A UNIX domain socket address is represented in the following structure:
struct sockaddr_un {
sa_family_t sun_family; /* AF_UNIX */
char sun_path[108]; /* pathname */
};
It can also be checked using the following define:
/usr/include/linux/un.h:6:#define UNIX_PATH_MAX 108
/usr/include/linux/un.h:10: char sun_path[UNIX_PATH_MAX]; /* pathname */
Several points to improve:
- libosmo-abis: e1_input_vty.c: 1 is at most 107 chars (+1 '\0'">DEFUN, otherwise return warning.
- libosmo-abis: unixsocket.c: unixsocket_line_update: Use UNIX_PATH_MAX instead of PATH_MAX, which is too big.
Bonus: Grep in all projects which use "osmo_sock_unix_init" function, and make sure the same validations are applied during vty parsing.
Updated by laforge over 5 years ago
- Assignee set to stsp
- Priority changed from Normal to Low
Updated by stsp over 5 years ago
Proposed patch for libosmo-abis: https://gerrit.osmocom.org/c/libosmo-abis/+/10647
Updated by stsp over 5 years ago
Proposed patch for libosmocore: https://gerrit.osmocom.org/#/c/libosmocore/+/10648
Updated by stsp over 5 years ago
Bonus: Grep in all projects which use "osmo_sock_unix_init" function, and make sure the same validations are applied during vty parsing.
I believe the above two patches are sufficient for this issue.
I just checked all existing callers of osmo_sock_unix_init and they will all detect failure when trying to create a unix socket with an overlong path.
Adding length checks in VTY-specific code is definitely user-friendly but should not be necessary.
Updated by stsp over 5 years ago
My patches contained a small mistake:
The case where a non-NULL-terminated socket path must be expected is where the
path of an accepted UNIX domain socket is obtained e.g. with getsockname().
When creating UNIX sockets for bind/connect, it is best practice to ensure
that the path is NUL-terminated. My previous patches allowed for a missing
NUL terminator during bind/connect.
Fixes for this are at:
https://gerrit.osmocom.org/c/libosmocore/+/11044
https://gerrit.osmocom.org/c/libosmo-abis/+/11045
Updated by stsp over 5 years ago
grep reveals several missing checks for overlong socket paths in other components.
A fix for one particular instance in osmo-bsc is at https://gerrit.osmocom.org/c/osmo-bsc/+/11046
Updated by stsp over 5 years ago
The case where a non-NULL-terminated socket path must be expected is where the path of an accepted UNIX domain socket is obtained e.g. with getsockname()
I have reviewed the following files and found no instance where we care about the
path of an accepted socket, so it looks like we're good in this respect.
libosmo-abis/src/input/unixsocket.c
libosmo-abis/src/e1_input_vty.c
libosmocore/src/socket.c
osmo-bsc/src/osmo-bsc/pcu_sock.c
osmo-bsc/src/osmo-bsc/bsc_rf_ctrl.c
osmo-bts/src/common/pcu_sock.c
osmocom-bb/src/host/trxcon/l1ctl_link.c
osmocom-bb/src/host/layer23/src/mobile/mncc_sock.c
osmocom-bb/src/host/osmocon/osmocon.c
osmo-msc/src/libmsc/mncc_sock.c
osmo-pcu/src/osmobts_sock.cpp
Updated by stsp over 5 years ago
Another overlong path check, this time for osmo-pcu, is here: https://gerrit.osmocom.org/c/osmo-pcu/+/11048
Updated by stsp over 5 years ago
- Status changed from In Progress to Resolved
All above patches have been merged. Closing this issue.
