Fix root permission issues (fixes #812)

* Provided fix for issue #812

Check permissions of config.xml before reading or writing from/to it.
If the permissions are wrong, assume root magic is going on, and fix it.

* Various fixes regarding coding guideline

- Fixed whitespaces
- Moved aux method to bottom of file
- Added Javadoc-like comments

* SettingsActivity: Use onClickListener for the useRoot Checkbox

* Moved fixAppDataPermissions() to class Util

* Removed obsolete shell commands

* Implemented Nutomic's changes

src/main/java/com/nutomic/syncthingandroid/activities/SettingsActivity.java

@@ -24,6 +24,7 @@ import com.nutomic.syncthingandroid.model.Options;
 import com.nutomic.syncthingandroid.service.RestApi;
 import com.nutomic.syncthingandroid.service.SyncthingService;
 import com.nutomic.syncthingandroid.util.Languages;
+import com.nutomic.syncthingandroid.util.Util;
 import com.nutomic.syncthingandroid.views.WifiSsidPreference;
 
 import java.security.InvalidParameterException;
@@ -155,7 +156,7 @@ public class SettingsActivity extends SyncthingActivity {
             environmentVariables.setOnPreferenceChangeListener(this);
             stReset.setOnPreferenceClickListener(this);
 
-            mUseRoot.setOnPreferenceChangeListener(this);
+            mUseRoot.setOnPreferenceClickListener(this);
             useWakelock.setOnPreferenceChangeListener(this::onRequireRestart);
             foregroundService.setOnPreferenceChangeListener(this::onRequireRestart);
             useTor.setOnPreferenceChangeListener(this::onRequireRestart);
@@ -406,9 +407,7 @@ public class SettingsActivity extends SyncthingActivity {
         private class ChownFilesRunnable implements Runnable {
             @Override
             public void run() {
-                String f = getActivity().getFilesDir().getAbsolutePath();
-                List<String> out = Shell.SU.run("chown -R --reference=" + f + " " + f);
-                Log.i(TAG, "Changed owner of syncthing files, output: " + out);
+                Util.fixAppDataPermissions(getActivity());
             }
         }
 

src/main/java/com/nutomic/syncthingandroid/util/ConfigXml.java

@@ -1,6 +1,9 @@
 package com.nutomic.syncthingandroid.util;
 
 import android.content.Context;
+import android.content.SharedPreferences;
+import android.content.pm.ApplicationInfo;
+import android.content.pm.PackageManager.NameNotFoundException;
 import android.os.Build;
 import android.os.Environment;
 import android.text.TextUtils;
@@ -8,6 +11,8 @@ import android.util.Log;
 
 import com.nutomic.syncthingandroid.R;
 import com.nutomic.syncthingandroid.service.SyncthingRunnable;
+import com.nutomic.syncthingandroid.service.SyncthingService;
+import com.nutomic.syncthingandroid.util.Util;
 
 import org.mindrot.jbcrypt.BCrypt;
 import org.w3c.dom.Document;
@@ -18,6 +23,7 @@ import org.xml.sax.SAXException;
 
 import java.io.File;
 import java.io.IOException;
+import java.io.DataOutputStream;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.util.Locale;
@@ -31,6 +37,7 @@ import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
 
+import eu.chainfire.libsuperuser.Shell;
 /**
  * Provides direct access to the config.xml file in the file system.
  *
@@ -63,12 +70,7 @@ public class ConfigXml {
             generateKeysConfig(context);
         }
 
-        try {
-            DocumentBuilder db = DocumentBuilderFactory.newInstance().newDocumentBuilder();
-            mConfig = db.parse(mConfigFile);
-        } catch (SAXException | ParserConfigurationException | IOException e) {
-            throw new OpenConfigException();
-        }
+        readConfig();
 
         if (isFirstStart) {
             changeLocalDeviceName();
@@ -76,6 +78,20 @@ public class ConfigXml {
         }
     }
 
+    private void readConfig() {
+        if (!mConfigFile.canRead() && !Util.fixAppDataPermissions(mContext)) {
+            throw new OpenConfigException();
+        }
+        try {
+            DocumentBuilder db = DocumentBuilderFactory.newInstance().newDocumentBuilder();
+            Log.d(TAG, "Trying to read '" + mConfigFile + "'");
+            mConfig = db.parse(mConfigFile);
+        } catch (SAXException | ParserConfigurationException | IOException e) {
+            Log.w(TAG, "Cannot read '" + mConfigFile + "'", e);
+            throw new OpenConfigException();
+        }
+    }
+
     private void generateKeysConfig(Context context) {
         new SyncthingRunnable(context, SyncthingRunnable.Command.generate).run();
     }
@@ -216,6 +232,10 @@ public class ConfigXml {
      * Writes updated mConfig back to file.
      */
     private void saveChanges() {
+        if (!mConfigFile.canWrite() && !Util.fixAppDataPermissions(mContext)) {
+            Log.w(TAG, "Failed to save updated config. Cannot change the owner of the config file.");
+            return;
+        }
         try {
             Log.i(TAG, "Writing updated config back to file");
             TransformerFactory transformerFactory = TransformerFactory.newInstance();
@@ -227,5 +247,4 @@ public class ConfigXml {
             Log.w(TAG, "Failed to save updated config", e);
         }
     }
-
 }

src/main/java/com/nutomic/syncthingandroid/util/Util.java

@@ -4,13 +4,23 @@ import android.content.ClipData;
 import android.content.ClipboardManager;
 import android.content.Context;
 import android.widget.Toast;
+import android.util.Log;
+import android.content.Context;
+import android.content.pm.ApplicationInfo;
+import android.content.pm.PackageManager.NameNotFoundException;
+
+import java.io.IOException;
+import java.io.DataOutputStream;
 
 import com.nutomic.syncthingandroid.R;
 
 import java.text.DecimalFormat;
+import eu.chainfire.libsuperuser.Shell;
 
 public class Util {
 
+    private static final String TAG = "SyncthingUtil";
+
     private Util() {
     }
 
@@ -55,4 +65,56 @@ public class Util {
         return new DecimalFormat("#,##0.#")
                 .format(bytes / Math.pow(1024, digitGroups)) + " " + units[digitGroups];
     }
+
+    /**
+     * Normally an application's data directory is only accessible by the corresponding application.
+     * Therefore, every file and directory is owned by an application's user and group. When running Syncthing as root,
+     * it writes to the application's data directory. This leaves files and directories behind which are owned by root having 0600.
+     * Moreover, those acitons performed as root changes a file's type in terms of SELinux.
+     * A subsequent start of Syncthing will fail due to insufficient permissions.
+     * Hence, this method fixes the owner, group and the files' type of the data directory.
+     * 
+     * @return true if the operation was successfully performed. False otherwise.
+     */
+    public static boolean fixAppDataPermissions(Context context) {
+        // We can safely assume that root magic is somehow available, because readConfig and saveChanges check for
+        // read and write access before calling us.
+        // Be paranoid :) and check if root is available.
+        // Ignore the 'use_root' preference, because we might want to fix ther permission
+        // just after the root option has been disabled.
+        if (!Shell.SU.available()) {
+            Log.e(TAG, "Root is not available. Cannot fix permssions.");
+            return false;
+        }
+
+        try {
+            ApplicationInfo appInfo = context.getPackageManager().getApplicationInfo(context.getPackageName(), 0);
+            Log.d(TAG, "Uid of '" + context.getPackageName() + "' is " + appInfo.uid);
+            Process fixPerm = Runtime.getRuntime().exec("su");
+            DataOutputStream fixPermOut = new DataOutputStream(fixPerm.getOutputStream());
+            String dir = context.getFilesDir().getAbsolutePath();
+            String cmd = "chown -R " + appInfo.uid + ":" + appInfo.uid + " " + dir + "\n";
+            Log.d(TAG, "Running: '" + cmd);
+            fixPermOut.writeBytes(cmd);
+            // Running Syncthing as root might change a file's or directories type in terms of SELinux.
+            // Leaving them as they are, the Android service won't be able to access them.
+            // At least for those files residing in an application's data folder.
+            // Simply reverting the type to its default should do the trick.
+            cmd = "restorecon -R " + dir + "\n";
+            Log.d(TAG, "Running: '" + cmd);
+            fixPermOut.writeBytes(cmd);
+            fixPermOut.flush();
+            fixPermOut.close();
+            int ret = fixPerm.waitFor();
+            Log.i(TAG, "Changed the owner, the group and the SELinux context of '" + dir + "'. Result: " + ret);
+            return ret == 0;
+        } catch (IOException | InterruptedException e) {
+            Log.w(TAG, "Cannot chown data directory", e);
+        } catch (NameNotFoundException e) {
+            // This should not happen!
+            // One should always be able to retrieve the application info for its own package.
+            Log.w(TAG, "This should not happen", e);
+        }
+        return false;
+    }
 }