From 060837348200306e617645b11f9c1ff1c8b60e09 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Fri, 7 Jun 2013 15:09:15 -0700 Subject: [PATCH] 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 --- .../android/server/NativeDaemonConnector.java | 80 +++++++++++++------ .../server/NativeDaemonConnectorTest.java | 27 +++++++ 2 files changed, 84 insertions(+), 23 deletions(-) diff --git a/services/java/com/android/server/NativeDaemonConnector.java b/services/java/com/android/server/NativeDaemonConnector.java index c3f2afa04b..47840e0882 100644 --- a/services/java/com/android/server/NativeDaemonConnector.java +++ b/services/java/com/android/server/NativeDaemonConnector.java @@ -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 ArrayList events = Lists.newArrayList(); - - final int sequenceNumber = mSequenceNumber.incrementAndGet(); - final StringBuilder cmdBuilder = - new StringBuilder(Integer.toString(sequenceNumber)).append(' '); final long startTime = SystemClock.elapsedRealtime(); - makeCommand(cmdBuilder, cmd, args); + final ArrayList events = Lists.newArrayList(); + + final StringBuilder rawBuilder = new StringBuilder(); + final StringBuilder logBuilder = new StringBuilder(); + final int sequenceNumber = mSequenceNumber.incrementAndGet(); + + makeCommand(rawBuilder, logBuilder, sequenceNumber, cmd, args); + + final String rawCmd = rawBuilder.toString(); + final String logCmd = logBuilder.toString(); - final String logCmd = cmdBuilder.toString(); /* includes cmdNum, cmd, args */ log("SND -> {" + logCmd + "}"); - cmdBuilder.append('\0'); - final String sentCmd = cmdBuilder.toString(); /* logCmd + \0 */ - 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 responses = new ArrayBlockingQueue(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 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); } } } diff --git a/services/tests/servicestests/src/com/android/server/NativeDaemonConnectorTest.java b/services/tests/servicestests/src/com/android/server/NativeDaemonConnectorTest.java index 275d807645..e2253a2151 100644 --- a/services/tests/servicestests/src/com/android/server/NativeDaemonConnectorTest.java +++ b/services/tests/servicestests/src/com/android/server/NativeDaemonConnectorTest.java @@ -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()); + } }