From a8a7a84ae55dd9b70cce8d7eb1dfbfd43600893a Mon Sep 17 00:00:00 2001 From: Ardo Septama Date: Mon, 4 Dec 2017 20:30:53 +0100 Subject: [PATCH 1/2] Hide sensitive data from audit and gerrit logs If there is password in ssh command parameters, it will be logged by Gerrit. This patch introduce @SensitiveData annotation to mark parameters that contain sensitive data and hide them from logger and audit. Audit masking must be enabled at gerrit.config: audit.maskSensitiveData=true Change-Id: I1f0e8e91bf971ed58b2362f89ce9f02cd8fb2ec7 Signed-off-by: Ardo Septama --- Documentation/config-gerrit.txt | 11 +++ .../com/google/gerrit/sshd/BaseCommand.java | 66 +++++++++++++++- .../gerrit/sshd/CommandFactoryProvider.java | 7 +- .../google/gerrit/sshd/DispatchCommand.java | 4 + .../com/google/gerrit/sshd/SensitiveData.java | 28 +++++++ .../sshd/SshCommandSensitiveFieldsCache.java | 24 ++++++ .../SshCommandSensitiveFieldsCacheImpl.java | 76 +++++++++++++++++++ .../java/com/google/gerrit/sshd/SshLog.java | 33 +++++--- .../com/google/gerrit/sshd/SshModule.java | 1 + .../gerrit/sshd/SshPluginStarterCallback.java | 7 +- 10 files changed, 242 insertions(+), 15 deletions(-) create mode 100644 gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java create mode 100644 gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java create mode 100644 gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 0ee3fd7f1d..072dcb8091 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -144,6 +144,17 @@ be ordered as ranked by the plugins (if there are any). + By default 1. +[[audit]] +=== Section audit + +[[audit.maskSensitiveData]]audit.maskSensitiveData:: ++ +If true, command parameters marked as sensitive are masked in audit logs. ++ +This option only affects audit. Other means of logging will always be masked. ++ +By default `false`. + [[auth]] === Section auth diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java index e38c0802f5..30fc8199ec 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java @@ -42,6 +42,10 @@ import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.io.StringWriter; import java.nio.charset.Charset; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicReference; import org.apache.sshd.common.SshException; @@ -63,6 +67,8 @@ public abstract class BaseCommand implements Command { static final int STATUS_NOT_FOUND = PRIVATE_STATUS | 2; public static final int STATUS_NOT_ADMIN = PRIVATE_STATUS | 3; + private static final String MASK = "***"; + @Option(name = "--", usage = "end of options", handler = EndOfOptionsHandler.class) private boolean endOfOptions; @@ -84,6 +90,8 @@ public abstract class BaseCommand implements Command { @Inject private SshScope.Context context; + @Inject private SshCommandSensitiveFieldsCache cache; + /** Commands declared by a plugin can be scoped by the plugin name. */ @Inject(optional = true) @PluginName @@ -98,6 +106,10 @@ public abstract class BaseCommand implements Command { /** Unparsed command line options. */ private String[] argv; + private List maskedArgv = new ArrayList<>(); + + private Set sensitiveParameters = new HashSet<>(); + public BaseCommand() { task = Atomics.newReference(); } @@ -143,6 +155,22 @@ public abstract class BaseCommand implements Command { this.argv = argv; } + public List getMaskedArguments() { + return maskedArgv; + } + + public String getFormattedMaskedArguments(String delimiter) { + return String.join(delimiter, maskedArgv); + } + + public void setMaskedArguments(List argv) { + this.maskedArgv = argv; + } + + public boolean isSensitiveParameter(String param) { + return sensitiveParameters.contains(param); + } + @Override public void destroy() { Future future = task.getAndSet(null); @@ -325,7 +353,7 @@ public abstract class BaseCommand implements Command { m.append(")"); } m.append(" during "); - m.append(context.getCommandLine()); + m.append(getFormattedMaskedArguments(" ")); log.error(m.toString(), e); } @@ -371,7 +399,7 @@ public abstract class BaseCommand implements Command { protected String getTaskDescription() { StringBuilder m = new StringBuilder(); - m.append(context.getCommandLine()); + m.append(getFormattedMaskedArguments(" ")); return m.toString(); } @@ -385,12 +413,46 @@ public abstract class BaseCommand implements Command { return m.toString(); } + private void maskSensitiveParameters() { + sensitiveParameters = cache.get(this.getClass()); + maskedArgv = new ArrayList<>(); + maskedArgv.add(commandName); + boolean maskNext = false; + for (int i = 0; i < argv.length; i++) { + if (maskNext) { + maskedArgv.add(MASK); + maskNext = false; + continue; + } + String arg = argv[i]; + String key = extractKey(arg); + if (isSensitiveParameter(key)) { + maskNext = arg.equals(key); + // When arg != key then parameter contains '=' sign and we mask them right away. + // Otherwise we mask the next parameter as indicated by maskNext. + if (!maskNext) { + arg = key + "=" + MASK; + } + } + maskedArgv.add(arg); + } + } + + private String extractKey(String arg) { + int eqPos = arg.indexOf('='); + if (eqPos > 0) { + return arg.substring(0, eqPos); + } + return arg; + } + private final class TaskThunk implements CancelableRunnable, ProjectRunnable { private final CommandRunnable thunk; private final String taskName; private Project.NameKey projectName; private TaskThunk(final CommandRunnable thunk) { + maskSensitiveParameters(); this.thunk = thunk; this.taskName = getTaskName(); } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java index c6d750c438..e1f7a65194 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java @@ -29,6 +29,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -159,7 +160,7 @@ class CommandFactoryProvider implements Provider, LifecycleListe } catch (Exception e) { logger.warn( "Cannot start command \"" - + ctx.getCommandLine() + + cmd.getFormattedMaskedArguments(" ") + "\" for user " + ctx.getSession().getUsername(), e); @@ -179,6 +180,10 @@ class CommandFactoryProvider implements Provider, LifecycleListe try { cmd = dispatcher.get(); cmd.setArguments(argv); + cmd.setMaskedArguments( + argv.length > 0 + ? Arrays.asList(argv[0]) + : Arrays.asList(ctx.getCommandLine().split(" ")[0])); cmd.setInputStream(in); cmd.setOutputStream(out); cmd.setErrorStream(err); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java index 2f3d10f61b..5e23cdb479 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java @@ -100,6 +100,10 @@ final class DispatchCommand extends BaseCommand { atomicCmd.set(cmd); cmd.start(env); + if (cmd instanceof BaseCommand) { + setMaskedArguments(((BaseCommand) cmd).getMaskedArguments()); + } + } catch (UnloggedFailure e) { String msg = e.getMessage(); if (!msg.endsWith("\n")) { diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java new file mode 100644 index 0000000000..1dd7896013 --- /dev/null +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java @@ -0,0 +1,28 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.sshd; + +import static java.lang.annotation.ElementType.FIELD; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +/** + * Annotation tagged on a field of an ssh command to indicate the value must be hidden from logs. + */ +@Target({FIELD}) +@Retention(RUNTIME) +public @interface SensitiveData {} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java new file mode 100644 index 0000000000..8c79299394 --- /dev/null +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java @@ -0,0 +1,24 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.sshd; + +import java.util.Set; + +/** Keeps data about ssh commands' parameters that have extra secure annotation. */ +public interface SshCommandSensitiveFieldsCache { + Set get(Class command); + + void evictAll(); +} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java new file mode 100644 index 0000000000..b5933886b4 --- /dev/null +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java @@ -0,0 +1,76 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.sshd; + +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; +import com.google.gerrit.server.cache.CacheModule; +import com.google.inject.Inject; +import com.google.inject.Module; +import com.google.inject.TypeLiteral; +import com.google.inject.name.Named; +import java.lang.reflect.Field; +import java.util.HashSet; +import java.util.Set; +import org.kohsuke.args4j.Option; + +public class SshCommandSensitiveFieldsCacheImpl implements SshCommandSensitiveFieldsCache { + private static final String CACHE_NAME = "sshd_sensitive_command_params"; + private final LoadingCache, Set> sshdCommandsCache; + + static Module module() { + return new CacheModule() { + @Override + protected void configure() { + cache(CACHE_NAME, new TypeLiteral>() {}, new TypeLiteral>() {}) + .loader(Loader.class); + bind(SshCommandSensitiveFieldsCache.class).to(SshCommandSensitiveFieldsCacheImpl.class); + } + }; + } + + @Inject + SshCommandSensitiveFieldsCacheImpl(@Named(CACHE_NAME) LoadingCache, Set> cache) { + sshdCommandsCache = cache; + } + + @Override + public Set get(Class cmd) { + return sshdCommandsCache.getUnchecked(cmd); + } + + @Override + public void evictAll() { + sshdCommandsCache.invalidateAll(); + } + + static class Loader extends CacheLoader, Set> { + + @Override + public Set load(Class cmd) throws Exception { + Set datas = new HashSet<>(); + for (Field field : cmd.getDeclaredFields()) { + if (field.isAnnotationPresent(SensitiveData.class)) { + Option option = field.getAnnotation(Option.class); + datas.add(option.name()); + for (String opt : option.aliases()) { + datas.add(opt); + } + } + } + return datas; + } + } +} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java index dfd56f1f88..aacd8bc458 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java @@ -48,9 +48,12 @@ class SshLog implements LifecycleListener { private static final String P_STATUS = "status"; private static final String P_AGENT = "agent"; + private static final String MASK = "***"; + private final Provider session; private final Provider context; private final AsyncAppender async; + private final boolean auditMask; private final AuditService auditService; @Inject @@ -64,6 +67,7 @@ class SshLog implements LifecycleListener { this.context = context; this.auditService = auditService; + auditMask = config.getBoolean("audit", "maskSensitiveData", false); if (!config.getBoolean("sshd", "requestLog", true)) { async = null; return; @@ -121,8 +125,7 @@ class SshLog implements LifecycleListener { final Context ctx = context.get(); ctx.finished = TimeUtil.nowMs(); - String cmd = extractWhat(dcmd); - + String cmd = extractWhat(dcmd, true); final LoggingEvent event = log(cmd); event.setProperty(P_WAIT, (ctx.started - ctx.created) + "ms"); event.setProperty(P_EXEC, (ctx.finished - ctx.started) + "ms"); @@ -154,7 +157,11 @@ class SshLog implements LifecycleListener { if (async != null) { async.append(event); } - audit(context.get(), status, dcmd); + + if (!auditMask) { + cmd = extractWhat(dcmd, false); + } + audit(ctx, status, cmd, extractParameters(dcmd)); } private ListMultimap extractParameters(DispatchCommand dcmd) { @@ -177,7 +184,10 @@ class SshLog implements LifecycleListener { // --param=value int eqPos = arg.indexOf('='); if (arg.startsWith("--") && eqPos > 0) { - parms.put(arg.substring(0, eqPos), arg.substring(eqPos + 1)); + String param = arg.substring(0, eqPos); + String value = + auditMask && dcmd.isSensitiveParameter(param) ? MASK : arg.substring(eqPos + 1); + parms.put(param, value); continue; } // -p value or --param value @@ -192,7 +202,7 @@ class SshLog implements LifecycleListener { if (paramName == null) { parms.put("$" + argPos++, arg); } else { - parms.put(paramName, arg); + parms.put(paramName, auditMask && dcmd.isSensitiveParameter(paramName) ? MASK : arg); paramName = null; } } @@ -256,10 +266,6 @@ class SshLog implements LifecycleListener { audit(ctx, result, cmd, null); } - void audit(Context ctx, Object result, DispatchCommand cmd) { - audit(ctx, result, extractWhat(cmd), extractParameters(cmd)); - } - private void audit(Context ctx, Object result, String cmd, ListMultimap params) { String sessionId; CurrentUser currentUser; @@ -277,11 +283,16 @@ class SshLog implements LifecycleListener { auditService.dispatch(new SshAuditEvent(sessionId, currentUser, cmd, created, params, result)); } - private String extractWhat(DispatchCommand dcmd) { + private String extractWhat(DispatchCommand dcmd, boolean hideSensitive) { if (dcmd == null) { return "Command was already destroyed"; } - StringBuilder commandName = new StringBuilder(dcmd.getCommandName()); + return hideSensitive ? dcmd.getFormattedMaskedArguments(".") : extractWhat(dcmd); + } + + private String extractWhat(DispatchCommand dcmd) { + String name = dcmd.getCommandName(); + StringBuilder commandName = new StringBuilder(name == null ? "" : name); String[] args = dcmd.getArguments(); for (int i = 1; i < args.length; i++) { commandName.append(".").append(args[i]); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java index d25c58be3a..4108cabb2a 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java @@ -63,6 +63,7 @@ public class SshModule extends LifecycleModule { configureRequestScope(); install(new AsyncReceiveCommits.Module()); configureAliases(); + install(SshCommandSensitiveFieldsCacheImpl.module()); bind(SshLog.class); bind(SshInfo.class).to(SshDaemon.class).in(SINGLETON); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java index 7f76ec6c83..7932290907 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java @@ -30,10 +30,14 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe private static final Logger log = LoggerFactory.getLogger(SshPluginStarterCallback.class); private final DispatchCommandProvider root; + private final SshCommandSensitiveFieldsCache cache; @Inject - SshPluginStarterCallback(@CommandName(Commands.ROOT) DispatchCommandProvider root) { + SshPluginStarterCallback( + @CommandName(Commands.ROOT) DispatchCommandProvider root, + SshCommandSensitiveFieldsCache cache) { this.root = root; + this.cache = cache; } @Override @@ -50,6 +54,7 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe if (cmd != null) { newPlugin.add(root.replace(Commands.named(newPlugin.getName()), cmd)); } + cache.evictAll(); } private Provider load(Plugin plugin) { From dfa74b835f451461a33c8a93c909fef11e04ff86 Mon Sep 17 00:00:00 2001 From: Ardo Septama Date: Mon, 29 Jan 2018 11:44:17 +0100 Subject: [PATCH 2/2] Protect against null pointer for BaseCommand Subclasses of BaseCommand may not initialize argv field (e.g. ScpCommand). Recent change [1] uses argv field which may result in NPE. This change will guard the code from such error. [1]: https://gerrit-review.googlesource.com/c/gerrit/+/146191 Change-Id: I6dd3afa08fd696d25741b5bdec8530deac422261 Signed-off-by: Ardo Septama --- .../src/main/java/com/google/gerrit/sshd/BaseCommand.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java index 30fc8199ec..e3e367c428 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java @@ -414,6 +414,9 @@ public abstract class BaseCommand implements Command { } private void maskSensitiveParameters() { + if (argv == null) { + return; + } sensitiveParameters = cache.get(this.getClass()); maskedArgv = new ArrayList<>(); maskedArgv.add(commandName);