Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 56cd646a authored by Jeff Sharkey's avatar Jeff Sharkey
Browse files

Avoid logging sensitive data. 

When building commands to send across NativeDaemonConnector, scrub
sensitive arguments to prevent them from being logged.

Bug: 8609800
Change-Id: I84b16791749264a010f7e59f9918f68d71bac6b9
parent 00d17f7a

services/java/com/android/server/MountService.java

+10 −9
Original line number Diff line number Diff line
@@ -64,6 +64,7 @@ import com.android.internal.app.IMediaContainerService; 
import com.android.internal.util.Preconditions;

import com.android.internal.util.XmlUtils;

import com.android.server.NativeDaemonConnector.Command;

import com.android.server.NativeDaemonConnector.SensitiveArg;

import com.android.server.am.ActivityManagerService;

import com.android.server.pm.PackageManagerService;

import com.android.server.pm.UserManagerService;
@@ -1642,8 +1643,8 @@ class MountService extends IMountService.Stub 


        int rc = StorageResultCode.OperationSucceeded;

        try {

            mConnector.execute("asec", "create", id, sizeMb, fstype, key, ownerUid,

                    external ? "1" : "0");

            mConnector.execute("asec", "create", id, sizeMb, fstype, new SensitiveArg(key),

                    ownerUid, external ? "1" : "0");

        } catch (NativeDaemonConnectorException e) {

            rc = StorageResultCode.OperationFailedInternalError;

        }
@@ -1743,7 +1744,7 @@ class MountService extends IMountService.Stub 


        int rc = StorageResultCode.OperationSucceeded;

        try {

            mConnector.execute("asec", "mount", id, key, ownerUid);

            mConnector.execute("asec", "mount", id, new SensitiveArg(key), ownerUid);

        } catch (NativeDaemonConnectorException e) {

            int code = e.getCode();

            if (code != VoldResponseCode.OpFailedStorageBusy) {
@@ -2019,7 +2020,7 @@ class MountService extends IMountService.Stub 


        final NativeDaemonEvent event;

        try {

            event = mConnector.execute("cryptfs", "checkpw", password);

            event = mConnector.execute("cryptfs", "checkpw", new SensitiveArg(password));



            final int code = Integer.parseInt(event.getMessage());

            if (code == 0) {
@@ -2058,7 +2059,7 @@ class MountService extends IMountService.Stub 
        }



        try {

            mConnector.execute("cryptfs", "enablecrypto", "inplace", password);

            mConnector.execute("cryptfs", "enablecrypto", "inplace", new SensitiveArg(password));

        } catch (NativeDaemonConnectorException e) {

            // Encryption failed

            return e.getCode();
@@ -2083,7 +2084,7 @@ class MountService extends IMountService.Stub 


        final NativeDaemonEvent event;

        try {

            event = mConnector.execute("cryptfs", "changepw", password);

            event = mConnector.execute("cryptfs", "changepw", new SensitiveArg(password));

            return Integer.parseInt(event.getMessage());

        } catch (NativeDaemonConnectorException e) {

            // Encryption failed
@@ -2116,7 +2117,7 @@ class MountService extends IMountService.Stub 


        final NativeDaemonEvent event;

        try {

            event = mConnector.execute("cryptfs", "verifypw", password);

            event = mConnector.execute("cryptfs", "verifypw", new SensitiveArg(password));

            Slog.i(TAG, "cryptfs verifypw => " + event.getMessage());

            return Integer.parseInt(event.getMessage());

        } catch (NativeDaemonConnectorException e) {
@@ -2482,8 +2483,8 @@ class MountService extends IMountService.Stub 


            int rc = StorageResultCode.OperationSucceeded;

            try {

                mConnector.execute(

                        "obb", "mount", mObbState.voldPath, hashedKey, mObbState.ownerGid);

                mConnector.execute("obb", "mount", mObbState.voldPath, new SensitiveArg(hashedKey),

                        mObbState.ownerGid);

            } catch (NativeDaemonConnectorException e) {

                int code = e.getCode();

                if (code != VoldResponseCode.OpFailedStorageBusy) {

services/java/com/android/server/NativeDaemonConnector.java

+55 −21
Original line number Diff line number Diff line
@@ -203,10 +203,29 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 
        }

    }



    /**

     * Wrapper around argument that indicates it's sensitive and shouldn't be

     * logged.

     */

    public static class SensitiveArg {

        private final Object mArg;



        public SensitiveArg(Object arg) {

            mArg = arg;

        }



        @Override

        public String toString() {

            return String.valueOf(mArg);

        }

    }



    /**

     * Make command for daemon, escaping arguments as needed.

     */

    private void makeCommand(StringBuilder builder, String cmd, Object... args) {

    @VisibleForTesting

    static void makeCommand(StringBuilder rawBuilder, StringBuilder logBuilder, int sequenceNumber,

            String cmd, Object... args) {

        if (cmd.indexOf('\0') >= 0) {

            throw new IllegalArgumentException("Unexpected command: " + cmd);

        }
@@ -214,16 +233,26 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 
            throw new IllegalArgumentException("Arguments must be separate from command");

        }



        builder.append(cmd);

        rawBuilder.append(sequenceNumber).append(' ').append(cmd);

        logBuilder.append(sequenceNumber).append(' ').append(cmd);

        for (Object arg : args) {

            final String argString = String.valueOf(arg);

            if (argString.indexOf('\0') >= 0) {

                throw new IllegalArgumentException("Unexpected argument: " + arg);

            }



            builder.append(' ');

            appendEscaped(builder, argString);

            rawBuilder.append(' ');

            logBuilder.append(' ');



            appendEscaped(rawBuilder, argString);

            if (arg instanceof SensitiveArg) {

                logBuilder.append("[scrubbed]");

            } else {

                appendEscaped(logBuilder, argString);

            }

        }



        rawBuilder.append('\0');

    }



    /**
@@ -303,27 +332,27 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 
     */

    public NativeDaemonEvent[] execute(int timeout, String cmd, Object... args)

            throws NativeDaemonConnectorException {

        final long startTime = SystemClock.elapsedRealtime();



        final ArrayList<NativeDaemonEvent> events = Lists.newArrayList();



        final StringBuilder rawBuilder = new StringBuilder();

        final StringBuilder logBuilder = new StringBuilder();

        final int sequenceNumber = mSequenceNumber.incrementAndGet();

        final StringBuilder cmdBuilder =

                new StringBuilder(Integer.toString(sequenceNumber)).append(' ');

        final long startTime = SystemClock.elapsedRealtime();



        makeCommand(cmdBuilder, cmd, args);

        makeCommand(rawBuilder, logBuilder, sequenceNumber, cmd, args);



        final String logCmd = cmdBuilder.toString(); /* includes cmdNum, cmd, args */

        log("SND -> {" + logCmd + "}");

        final String rawCmd = rawBuilder.toString();

        final String logCmd = logBuilder.toString();



        cmdBuilder.append('\0');

        final String sentCmd = cmdBuilder.toString(); /* logCmd + \0 */

        log("SND -> {" + logCmd + "}");



        synchronized (mDaemonLock) {

            if (mOutputStream == null) {

                throw new NativeDaemonConnectorException("missing output stream");

            } else {

                try {

                    mOutputStream.write(sentCmd.getBytes(Charsets.UTF_8));

                    mOutputStream.write(rawCmd.getBytes(Charsets.UTF_8));

                } catch (IOException e) {

                    throw new NativeDaemonConnectorException("problem sending command", e);

                }
@@ -332,7 +361,7 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 


        NativeDaemonEvent event = null;

        do {

            event = mResponseQueue.remove(sequenceNumber, timeout, sentCmd);

            event = mResponseQueue.remove(sequenceNumber, timeout, logCmd);

            if (event == null) {

                loge("timed-out waiting for response to " + logCmd);

                throw new NativeDaemonFailureException(logCmd, event);
@@ -447,10 +476,11 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 
    private static class ResponseQueue {



        private static class PendingCmd {

            public int cmdNum;

            public final int cmdNum;

            public final String logCmd;



            public BlockingQueue<NativeDaemonEvent> responses =

                    new ArrayBlockingQueue<NativeDaemonEvent>(10);

            public String request;



            // The availableResponseCount member is used to track when we can remove this

            // instance from the ResponseQueue.
@@ -468,7 +498,11 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 
            // hold references to this instance already so it can be removed from

            // mPendingCmds queue.

            public int availableResponseCount;

            public PendingCmd(int c, String r) {cmdNum = c; request = r;}



            public PendingCmd(int cmdNum, String logCmd) {

                this.cmdNum = cmdNum;

                this.logCmd = logCmd;

            }

        }



        private final LinkedList<PendingCmd> mPendingCmds;
@@ -497,7 +531,7 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 
                        // let any waiter timeout waiting for this

                        PendingCmd pendingCmd = mPendingCmds.remove();

                        Slog.e("NativeDaemonConnector.ResponseQueue",

                                "Removing request: " + pendingCmd.request + " (" +

                                "Removing request: " + pendingCmd.logCmd + " (" +

                                pendingCmd.cmdNum + ")");

                    }

                    found = new PendingCmd(cmdNum, null);
@@ -515,7 +549,7 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 


        // note that the timeout does not count time in deep sleep.  If you don't want

        // the device to sleep, hold a wakelock

        public NativeDaemonEvent remove(int cmdNum, int timeoutMs, String origCmd) {

        public NativeDaemonEvent remove(int cmdNum, int timeoutMs, String logCmd) {

            PendingCmd found = null;

            synchronized (mPendingCmds) {

                for (PendingCmd pendingCmd : mPendingCmds) {
@@ -525,7 +559,7 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 
                    }

                }

                if (found == null) {

                    found = new PendingCmd(cmdNum, origCmd);

                    found = new PendingCmd(cmdNum, logCmd);

                    mPendingCmds.add(found);

                }

                found.availableResponseCount--;
@@ -547,7 +581,7 @@ final class NativeDaemonConnector implements Runnable, Handler.Callback, Watchdo 
            pw.println("Pending requests:");

            synchronized (mPendingCmds) {

                for (PendingCmd pendingCmd : mPendingCmds) {

                    pw.println("  Cmd " + pendingCmd.cmdNum + " - " + pendingCmd.request);

                    pw.println("  Cmd " + pendingCmd.cmdNum + " - " + pendingCmd.logCmd);

                }

            }

        }

services/java/com/android/server/NetworkManagementService.java

+3 −3
Original line number Diff line number Diff line
@@ -34,7 +34,6 @@ import static com.android.server.NetworkManagementService.NetdResponseCode.Tethe 
import static com.android.server.NetworkManagementService.NetdResponseCode.TtyListResult;

import static com.android.server.NetworkManagementSocketTagger.PROP_QTAGUID_ENABLED;



import android.bluetooth.BluetoothTetheringDataTracker;

import android.content.Context;

import android.net.INetworkManagementEventObserver;

import android.net.InterfaceConfiguration;
@@ -59,6 +58,7 @@ import android.util.SparseBooleanArray; 
import com.android.internal.net.NetworkStatsFactory;

import com.android.internal.util.Preconditions;

import com.android.server.NativeDaemonConnector.Command;

import com.android.server.NativeDaemonConnector.SensitiveArg;

import com.android.server.net.LockdownVpnTracker;

import com.google.android.collect.Maps;
@@ -990,7 +990,7 @@ public class NetworkManagementService extends INetworkManagementService.Stub 
                mConnector.execute("softap", "set", wlanIface);

            } else {

                mConnector.execute("softap", "set", wlanIface, wifiConfig.SSID,

                        getSecurityType(wifiConfig), wifiConfig.preSharedKey);

                        getSecurityType(wifiConfig), new SensitiveArg(wifiConfig.preSharedKey));

            }

            mConnector.execute("softap", "startap");

        } catch (NativeDaemonConnectorException e) {
@@ -1039,7 +1039,7 @@ public class NetworkManagementService extends INetworkManagementService.Stub 
                mConnector.execute("softap", "set", wlanIface);

            } else {

                mConnector.execute("softap", "set", wlanIface, wifiConfig.SSID,

                        getSecurityType(wifiConfig), wifiConfig.preSharedKey);

                        getSecurityType(wifiConfig), new SensitiveArg(wifiConfig.preSharedKey));

            }

        } catch (NativeDaemonConnectorException e) {

            throw e.rethrowAsParcelableException();

services/tests/servicestests/src/com/android/server/NativeDaemonConnectorTest.java

+27 −0
Original line number Diff line number Diff line
@@ -17,10 +17,13 @@ 
package com.android.server;



import static com.android.server.NativeDaemonConnector.appendEscaped;

import static com.android.server.NativeDaemonConnector.makeCommand;



import android.test.AndroidTestCase;

import android.test.suitebuilder.annotation.MediumTest;



import com.android.server.NativeDaemonConnector.SensitiveArg;



/**

 * Tests for {@link NativeDaemonConnector}.

 */
@@ -67,4 +70,28 @@ public class NativeDaemonConnectorTest extends AndroidTestCase { 
        appendEscaped(builder, "caf\u00E9 c\u00F6ffee");

        assertEquals("\"caf\u00E9 c\u00F6ffee\"", builder.toString());

    }



    public void testSensitiveArgs() throws Exception {

        final StringBuilder rawBuilder = new StringBuilder();

        final StringBuilder logBuilder = new StringBuilder();



        rawBuilder.setLength(0);

        logBuilder.setLength(0);

        makeCommand(rawBuilder, logBuilder, 1, "foo", "bar", "baz");

        assertEquals("1 foo bar baz\0", rawBuilder.toString());

        assertEquals("1 foo bar baz", logBuilder.toString());



        rawBuilder.setLength(0);

        logBuilder.setLength(0);

        makeCommand(rawBuilder, logBuilder, 1, "foo", new SensitiveArg("bar"), "baz");

        assertEquals("1 foo bar baz\0", rawBuilder.toString());

        assertEquals("1 foo [scrubbed] baz", logBuilder.toString());



        rawBuilder.setLength(0);

        logBuilder.setLength(0);

        makeCommand(rawBuilder, logBuilder, 1, "foo", new SensitiveArg("foo bar"), "baz baz",

                new SensitiveArg("wat"));

        assertEquals("1 foo \"foo bar\" \"baz baz\" wat\0", rawBuilder.toString());

        assertEquals("1 foo [scrubbed] \"baz baz\" [scrubbed]", logBuilder.toString());

    }

}