jeltz/hostap
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Android: Harden wpa_ctrl_open2() against potential race conditions

Browse source 
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 <j@w1.fi>
This commit is contained in:
Jouni Malinen 2019-01-06 20:01:09 +02:00
parent 9aac73121b
commit b686745c76
1 changed files with 17 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
src/common/wpa_ctrl.c
View file

@ -11,6 +11,8 @@
#ifdef CONFIG_CTRL_IFACE #ifdef CONFIG_CTRL_IFACE
#ifdef CONFIG_CTRL_IFACE_UNIX #ifdef CONFIG_CTRL_IFACE_UNIX
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/un.h> #include <sys/un.h>
#include <unistd.h> #include <unistd.h>
#include <fcntl.h> #include <fcntl.h>
@ -133,6 +135,19 @@ try_again:
return NULL; return NULL;
} }
tries++; 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, if (bind(ctrl->s, (struct sockaddr *) &ctrl->local,
sizeof(ctrl->local)) < 0) { sizeof(ctrl->local)) < 0) {
if (errno == EADDRINUSE && tries < 2) { if (errno == EADDRINUSE && tries < 2) {
@ -151,10 +166,9 @@ try_again:
} }
#ifdef ANDROID #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 */ /* Set group even if we do not have privileges to change owner */
chown(ctrl->local.sun_path, -1, AID_WIFI); lchown(ctrl->local.sun_path, -1, AID_WIFI);
chown(ctrl->local.sun_path, AID_SYSTEM, AID_WIFI); lchown(ctrl->local.sun_path, AID_SYSTEM, AID_WIFI);
if (os_strncmp(ctrl_path, "@android:", 9) == 0) { if (os_strncmp(ctrl_path, "@android:", 9) == 0) {
if (socket_local_client_connect( if (socket_local_client_connect(