From b686745c76b8fe88a6d4adda1be136d2d74f094c Mon Sep 17 00:00:00 2001 From: Jouni Malinen Date: Sun, 6 Jan 2019 20:01:09 +0200 Subject: [PATCH] Android: Harden wpa_ctrl_open2() against potential race conditions The Android-specific chmod and chown operations on the client socket (for communication with wpa_supplicant) did not protect against file replacement between the bind() and chmod()/chown() calls. If the directory in which the client socket is created (depends a bit on the version and platform, but /data/misc/wifi/sockets is commonly used) allows write access to processes that are different (less privileged) compared to the process calling wpa_ctrl_open2(), it might be possible to delete the socket file and replace it with something else (mainly, a symlink) before the chmod/chown operations occur. This could have resulted in the owner or permissions of the target of that symlink being modified. In general, it would be safest to use a directory which has more limited write privileges (/data/misc/wifi/sockets normally has 'wifi' group (AID_WIFI) with write access), but if that cannot be easily changed due to other constraints, it is better to make wpa_ctrl_open2() less likely to enable this type of race condition between the operations. Replace chown() with lchown() (i.e., a version that does not dereference symlinks) and chmod() with fchmod() on the socket before the bind() call which is also not going to dereference a symlink (whereas chmod() would). lchown() is a standard operation, but the fchmod() on the socket is less so (unspecified behavior in some systems). However, it seems to work on Linux and in particular, on Android, where this code is executed. Signed-off-by: Jouni Malinen --- src/common/wpa_ctrl.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/common/wpa_ctrl.c b/src/common/wpa_ctrl.c index 1f1c9c422..c9890a0e4 100644 --- a/src/common/wpa_ctrl.c +++ b/src/common/wpa_ctrl.c @@ -11,6 +11,8 @@ #ifdef CONFIG_CTRL_IFACE #ifdef CONFIG_CTRL_IFACE_UNIX +#include +#include #include #include #include @@ -133,6 +135,19 @@ try_again: return NULL; } tries++; +#ifdef ANDROID + /* Set client socket file permissions so that bind() creates the client + * socket with these permissions and there is no need to try to change + * them with chmod() after bind() which would have potential issues with + * race conditions. These permissions are needed to make sure the server + * side (wpa_supplicant or hostapd) can reply to the control interface + * messages. + * + * The lchown() calls below after bind() are also part of the needed + * operations to allow the response to go through. Those are using the + * no-deference-symlinks version to avoid races. */ + fchmod(ctrl->s, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP); +#endif /* ANDROID */ if (bind(ctrl->s, (struct sockaddr *) &ctrl->local, sizeof(ctrl->local)) < 0) { if (errno == EADDRINUSE && tries < 2) { @@ -151,10 +166,9 @@ try_again: } #ifdef ANDROID - chmod(ctrl->local.sun_path, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP); /* Set group even if we do not have privileges to change owner */ - chown(ctrl->local.sun_path, -1, AID_WIFI); - chown(ctrl->local.sun_path, AID_SYSTEM, AID_WIFI); + lchown(ctrl->local.sun_path, -1, AID_WIFI); + lchown(ctrl->local.sun_path, AID_SYSTEM, AID_WIFI); if (os_strncmp(ctrl_path, "@android:", 9) == 0) { if (socket_local_client_connect(