From a8a7a84ae55dd9b70cce8d7eb1dfbfd43600893a Mon Sep 17 00:00:00 2001 From: Ardo Septama Date: Mon, 4 Dec 2017 20:30:53 +0100 Subject: [PATCH 1/3] 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/3] 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); From bf5cfebda81b2e1e1b10801b46e7686f0aef4cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 29 Jan 2018 16:41:01 -0500 Subject: [PATCH 3/3] Fix error dialog not honoring line breaks Creating ErrorDialog from Throwable was already formatting error message nicely because it is setting the css attribute 'whiteSpace' to 'pre'. Creating ErrorDialog from an error message string was not setting this css attribute resulting in line break being removed. Fix this by making both use a private method to create error message label. Also change the attribute from 'whiteSpace' to 'white-space' which is the real name. Change-Id: I5932c3d547b8ab232de2f866d0ad20d968804ee8 --- .../java/com/google/gerrit/client/ErrorDialog.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ErrorDialog.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ErrorDialog.java index e8807126c4..8e12575e89 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ErrorDialog.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ErrorDialog.java @@ -88,7 +88,7 @@ public class ErrorDialog extends PopupPanel { /** Create a dialog box to show a single message string. */ public ErrorDialog(final String message) { this(); - body.add(new Label(message)); + body.add(createErrorMsgLabel(message)); } /** Create a dialog box to show a single message string. */ @@ -145,12 +145,16 @@ public class ErrorDialog extends PopupPanel { } if (msg != null) { - final Label m = new Label(msg); - m.getElement().getStyle().setProperty("whiteSpace", "pre"); - body.add(m); + body.add(createErrorMsgLabel(msg)); } } + private Label createErrorMsgLabel(String message) { + Label m = new Label(message); + m.getElement().getStyle().setProperty("white-space", "pre"); + return m; + } + public ErrorDialog setText(final String t) { text.setText(t); return this;