Project

General

Profile

Bug #2673

e1_line socket has no/wrong path length check

Added by pespin about 1 year ago. Updated 2 months ago.

Status:
Resolved
Priority:
Low
Assignee:
Target version:
-
Start date:
11/22/2017
Due date:
% Done:

0%

Spec Reference:
Tags:
E1

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.

History

#1 Updated by pespin about 1 year ago

  • Description updated (diff)

#2 Updated by laforge 6 months ago

  • Tags set to E1

#3 Updated by laforge 4 months ago

  • Assignee set to stsp
  • Priority changed from Normal to Low

#4 Updated by stsp 3 months ago

Proposed patch for libosmo-abis: https://gerrit.osmocom.org/c/libosmo-abis/+/10647

#5 Updated by stsp 3 months ago

#6 Updated by stsp 3 months 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.

#7 Updated by stsp 3 months ago

  • Status changed from New to In Progress

#8 Updated by stsp 3 months 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

#9 Updated by stsp 3 months 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

#10 Updated by stsp 3 months 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

#11 Updated by stsp 3 months ago

Another overlong path check, this time for osmo-pcu, is here: https://gerrit.osmocom.org/c/osmo-pcu/+/11048

#12 Updated by stsp 2 months ago

  • Status changed from In Progress to Resolved

All above patches have been merged. Closing this issue.

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)