Don't trip up when deleting strict iptables rules.

Currently, when applying a cleartext policy to a UID,
StrictController will attempt to delete all possible policies
that might previously have applied to this UID. Because only
two of these rules can exist at any given time, at least one
of these deletes is guaranteed to fail, causing the whole
operation to fail.

Instead of adding a log or reject rule for every UID, add a
rule that sends that UID to its own chain which then contains
the log or reject rule. That way, deleting the previous policy
only requires deleting the chain, which is something we know
exists.

Bug: 64988066
Test: netd_{unit,integration}_test pass
Test: android.os.cts.StrictModeTest passes
Change-Id: Ic9d66220a65f2ce9510c4194e7b874d3d5dca5d7

2 files changed, 26 insertions, 11 deletions

diff --git a/server/StrictController.cpp b/server/StrictController.cpp
index ea25b2ea..04c1bfec 100644
--- a/server/StrictController.cpp
+++ b/server/StrictController.cpp
@@ -157,6 +157,13 @@ int StrictController::disableStrict(void) {
 }
 
 int StrictController::setUidCleartextPenalty(uid_t uid, StrictPenalty penalty) {
+    // When a penalty is set, we don't know what penalty the UID previously had. In order to be able
+    // to clear the previous penalty without causing an iptables error by deleting rules that don't
+    // exist, put each UID's rules in a chain specific to that UID. That way, the commands we need
+    // to run to clear the previous penalty don't depend on what the penalty actually was - all we
+    // need to do is clear the chain.
+    std::string perUidChain = StringPrintf("st_clear_caught_%u", uid);
+
     std::vector<std::string> commands;
     if (penalty == ACCEPT) {
         // Clean up any old rules
@@ -165,21 +172,24 @@ int StrictController::setUidCleartextPenalty(uid_t uid, StrictPenalty penalty) {
             StringPrintf("-D %s -m owner --uid-owner %d -j %s",
                          LOCAL_OUTPUT, uid, LOCAL_CLEAR_DETECT),
             StringPrintf("-D %s -m owner --uid-owner %d -j %s",
-                         LOCAL_CLEAR_CAUGHT, uid, LOCAL_PENALTY_LOG),
-            StringPrintf("-D %s -m owner --uid-owner %d -j %s",
-                         LOCAL_CLEAR_CAUGHT, uid, LOCAL_PENALTY_REJECT),
+                         LOCAL_CLEAR_CAUGHT, uid, perUidChain.c_str()),
+            StringPrintf("-F %s", perUidChain.c_str()),
+            StringPrintf("-X %s", perUidChain.c_str()),
         };
     } else {
         // Always take a detour to investigate this UID
         commands.push_back("*filter");
+        commands.push_back(StringPrintf(":%s -", perUidChain.c_str()));
         commands.push_back(StringPrintf("-I %s -m owner --uid-owner %d -j %s",
                                         LOCAL_OUTPUT, uid, LOCAL_CLEAR_DETECT));
+        commands.push_back(StringPrintf("-I %s -m owner --uid-owner %d -j %s",
+                                        LOCAL_CLEAR_CAUGHT, uid, perUidChain.c_str()));
+
         if (penalty == LOG) {
-            commands.push_back(StringPrintf("-I %s -m owner --uid-owner %d -j %s",
-                                            LOCAL_CLEAR_CAUGHT, uid, LOCAL_PENALTY_LOG));
+            commands.push_back(StringPrintf("-A %s -j %s", perUidChain.c_str(), LOCAL_PENALTY_LOG));
         } else if (penalty == REJECT) {
-            commands.push_back(StringPrintf("-I %s -m owner --uid-owner %d -j %s",
-                                            LOCAL_CLEAR_CAUGHT, uid, LOCAL_PENALTY_REJECT));
+            commands.push_back(StringPrintf("-A %s -j %s", perUidChain.c_str(),
+                                            LOCAL_PENALTY_REJECT));
         }
     }
     commands.push_back("COMMIT\n");
diff --git a/server/StrictControllerTest.cpp b/server/StrictControllerTest.cpp
index 82d0cdaf..9770352c 100644
--- a/server/StrictControllerTest.cpp
+++ b/server/StrictControllerTest.cpp
@@ -126,20 +126,25 @@ TEST_F(StrictControllerTest, TestSetUidCleartextPenalty) {
     std::vector<std::string> acceptCommands = {
         "*filter\n"
         "-D st_OUTPUT -m owner --uid-owner 12345 -j st_clear_detect\n"
-        "-D st_clear_caught -m owner --uid-owner 12345 -j st_penalty_log\n"
-        "-D st_clear_caught -m owner --uid-owner 12345 -j st_penalty_reject\n"
+        "-D st_clear_caught -m owner --uid-owner 12345 -j st_clear_caught_12345\n"
+        "-F st_clear_caught_12345\n"
+        "-X st_clear_caught_12345\n"
         "COMMIT\n"
     };
     std::vector<std::string> logCommands = {
         "*filter\n"
+        ":st_clear_caught_12345 -\n"
         "-I st_OUTPUT -m owner --uid-owner 12345 -j st_clear_detect\n"
-        "-I st_clear_caught -m owner --uid-owner 12345 -j st_penalty_log\n"
+        "-I st_clear_caught -m owner --uid-owner 12345 -j st_clear_caught_12345\n"
+        "-A st_clear_caught_12345 -j st_penalty_log\n"
         "COMMIT\n"
     };
     std::vector<std::string> rejectCommands = {
         "*filter\n"
+        ":st_clear_caught_12345 -\n"
         "-I st_OUTPUT -m owner --uid-owner 12345 -j st_clear_detect\n"
-        "-I st_clear_caught -m owner --uid-owner 12345 -j st_penalty_reject\n"
+        "-I st_clear_caught -m owner --uid-owner 12345 -j st_clear_caught_12345\n"
+        "-A st_clear_caught_12345 -j st_penalty_reject\n"
         "COMMIT\n"
     };