From: Konrad Lipinski <k.lipinski2@samsung.com>
Date: Tue, 20 Oct 2020 13:35:20 +0000 (+0200)
Subject: Apply private sharing rules before relabeling
X-Git-Tag: submit/tizen/20201022.113306~1
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=05371c02402016f290cf4a1ce6a1ce830020fcc9;p=platform%2Fcore%2Fsecurity%2Fsecurity-manager.git

Apply private sharing rules before relabeling

Prior to this commit, applyPrivatePathSharing does this:
1. Relabel a privately shared file.
2. Enable the package to rwxat the file's label.

Thus, there's a window between steps 1 & 2 where the package is unable
to access the file. This can be remedied by changing the order to:
1. Enable the package to rwxat the file's label.
2. Relabel the privately shared file.

The change preserves current semantics post-return but eliminates the
window.

The context:

Reportedly, the utc_rpc_port_set_private_sharing_array_p TCT test has
revealed a possibility of a race condition where a package owner would
get a smack access error when trying to unlink one of its own privately
shared files. This has reportedly happened on TM1 and some unspecified
TV product.

HQ inserted a 10ms sleep into ServiceImpl::applyPrivatePathSharing right
before return and, reportedly, it seems to have fixed the issue. They
seem partial to the assumption that the root cause is related to a race
condition in the kernel (as in: smack rules are being applied with a
delay). Thus, an idea for a possible solution involved checking smack
access client-side to make sure all is well before private sharing is
considered applied.

Given the fact that smack has been in place for quite some time now, I
find the possibility of a race condition unlikely. Unfortunately, I
haven't been able to prove anything. I couldn't reproduce the problem
and failed to find any obvious faults in the TCT test.

If there is a race condition, checking smack access client-side may not
be enough (it would only guarantee the client process or thread to be
race-free, TCT tests or the platform may need stronger guarantees). I'm
not inclined to do that unless there's proof. Such messy defensive code
tends to do more harm then good, especially if the race condition is
elsewhere.

Change-Id: I0a57edd6535eb1889d9bb8e5aaa6ddab58ca7009
---

diff --git a/src/common/service_impl.cpp b/src/common/service_impl.cpp
index 7575ab12..a7e15c09 100644
--- a/src/common/service_impl.cpp
+++ b/src/common/service_impl.cpp
@@ -1778,13 +1778,13 @@ int ServiceImpl::applyPrivatePathSharing(
                 //Nothing to do, only counter needed incrementing
                 continue;
             }
-            if (pathCount <= 0) {
-                SmackLabels::setupSharedPrivatePath(ownerPkgName, path);
-            }
             m_smackRules.applyPrivateSharingRules(ownerPkgName, pkgsLabels,
                                                 targetAppLabel, pathLabel,
                                                 (pathCount > 0),
                                                 (ownerTargetCount > 0));
+            if (pathCount <= 0) {
+                SmackLabels::setupSharedPrivatePath(ownerPkgName, path);
+            }
         }
         trans.commit();
         return SECURITY_MANAGER_SUCCESS;