commit 71ade013f46410187c9d981504ec1a713f20d1b7
author Paul Lawrence <paullawrence@google.com> Tue Jul 18 08:49:44 2023 -0700
committer Paul Lawrence <paullawrence@google.com> Tue Jul 18 08:51:19 2023 -0700
tree 6ee51ea9769a367a13379ef1e3f033e4080d3886
parent b1c1cf7c86a8f58d567c933ba3cb20b1061355fd

Listen on property_service_for_system socket for powerctl messages

It is easy to dos the property_service socket, since it will wait for a
complete data packet from one command before moving on to the next one.

To prevent low privilege apps interfering with system and root apps,
add a second property_service socket that only they can use.

However, since writes to properties are not thread-safe, limit use of
this second socket to just sys.powerctl messages. These are the messages
that this security issue is concerned about, and they do not actually
write to the properties, rather they are acted upon immediately.

Bug: 262208935
Test: Builds, boots
Ignore-AOSP-First: Security fix
Change-Id: I1e96444115de4cc0b021c6864922845de331f6a7

libc/bionic/system_property_set.cpp

diff --git a/libc/bionic/system_property_set.cpp b/libc/bionic/system_property_set.cpp
index f7999db..6e49bce 100644
--- a/libc/bionic/system_property_set.cpp
+++ b/libc/bionic/system_property_set.cpp
@@ -49,21 +49,34 @@
 #include "private/ScopedFd.h"
 
 static const char property_service_socket[] = "/dev/socket/" PROP_SERVICE_NAME;
+static const char property_service_for_system_socket[] =
+    "/dev/socket/" PROP_SERVICE_FOR_SYSTEM_NAME;
 static const char* kServiceVersionPropertyName = "ro.property_service.version";
 
 class PropertyServiceConnection {
  public:
-  PropertyServiceConnection() : last_error_(0) {
+  PropertyServiceConnection(const char* name) : last_error_(0) {
     socket_.reset(::socket(AF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0));
     if (socket_.get() == -1) {
       last_error_ = errno;
       return;
     }
 
-    const size_t namelen = strlen(property_service_socket);
+    // If we're trying to set "sys.powerctl" from a privileged process, use the special
+    // socket. Because this socket is only accessible to privileged processes, it can't
+    // be DoSed directly by malicious apps. (The shell user should be able to reboot,
+    // though, so we don't just always use the special socket for "sys.powerctl".)
+    // See b/262237198 for context
+    const char* socket = property_service_socket;
+    if (strcmp(name, "sys.powerctl") == 0 &&
+        access(property_service_for_system_socket, W_OK) == 0) {
+      socket = property_service_for_system_socket;
+    }
+
+    const size_t namelen = strlen(socket);
     sockaddr_un addr;
     memset(&addr, 0, sizeof(addr));
-    strlcpy(addr.sun_path, property_service_socket, sizeof(addr.sun_path));
+    strlcpy(addr.sun_path, socket, sizeof(addr.sun_path));
     addr.sun_family = AF_LOCAL;
     socklen_t alen = namelen + offsetof(sockaddr_un, sun_path) + 1;
 
@@ -176,7 +189,7 @@
 };
 
 static int send_prop_msg(const prop_msg* msg) {
-  PropertyServiceConnection connection;
+  PropertyServiceConnection connection(msg->name);
   if (!connection.IsValid()) {
     return connection.GetLastError();
   }
@@ -269,7 +282,7 @@
     // New protocol only allows long values for ro. properties only.
     if (strlen(value) >= PROP_VALUE_MAX && strncmp(key, "ro.", 3) != 0) return -1;
     // Use proper protocol
-    PropertyServiceConnection connection;
+    PropertyServiceConnection connection(key);
     if (!connection.IsValid()) {
       errno = connection.GetLastError();
       async_safe_format_log(ANDROID_LOG_WARN, "libc",