diff options
author | Martin Polden <mpolden@mpolden.no> | 2022-05-13 11:29:51 +0200 |
---|---|---|
committer | Martin Polden <mpolden@mpolden.no> | 2022-05-13 12:14:55 +0200 |
commit | b6bb92cc71074a55a1bb3441951ee37d946ce620 (patch) | |
tree | 2ebcbd3a03614af23beaf5e51fb2effc91419fca /controller-server | |
parent | e0e2af53a7d6df0291e416e3ad56a5132569014c (diff) |
Fix request body sanitizing
Diffstat (limited to 'controller-server')
4 files changed, 35 insertions, 21 deletions
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLog.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLog.java index 19fb1b7e1bb..6d011438b10 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLog.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLog.java @@ -1,7 +1,6 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.auditlog; -import com.google.common.base.CharMatcher; import com.google.common.collect.Ordering; import java.time.Instant; @@ -10,7 +9,6 @@ import java.util.Comparator; import java.util.List; import java.util.Objects; import java.util.Optional; -import java.util.function.Predicate; /** * This represents the audit log of a hosted Vespa system. The audit log contains manual actions performed through @@ -66,7 +64,7 @@ public class AuditLog { private final String resource; private final Optional<String> data; - public Entry(Instant at, String principal, Method method, String resource, Optional<String> data) { + public Entry(Instant at, String principal, Method method, String resource, byte[] data) { this.at = Objects.requireNonNull(at, "at must be non-null"); this.principal = Objects.requireNonNull(principal, "principal must be non-null"); this.method = Objects.requireNonNull(method, "method must be non-null"); @@ -112,16 +110,27 @@ public class AuditLog { DELETE } - private static Optional<String> sanitize(Optional<String> data) { - Objects.requireNonNull(data, "data must be non-null"); - return data.filter(Predicate.not(String::isBlank)) - .filter(CharMatcher.ascii()::matchesAllOf) - .map(v -> { - if (v.length() > maxDataLength) { - return v.substring(0, maxDataLength); - } - return v; - }); + private static Optional<String> sanitize(byte[] data) { + StringBuilder sb = new StringBuilder(); + for (byte b : data) { + char c = (char) b; + if (!printableAscii(c) && !tabOrLineBreak(c)) { + return Optional.empty(); + } + sb.append(c); + if (sb.length() == maxDataLength) { + break; + } + } + return Optional.of(sb.toString()).filter(s -> !s.isEmpty()); + } + + private static boolean printableAscii(char c) { + return c >= 32 && c <= 126; + } + + private static boolean tabOrLineBreak(char c) { + return c == 9 || c == 10 || c == 13; } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLogger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLogger.java index 34e7955e02a..b6782767386 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLogger.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/auditlog/AuditLogger.java @@ -3,14 +3,12 @@ package com.yahoo.vespa.hosted.controller.auditlog; import com.yahoo.container.jdisc.HttpRequest; import com.yahoo.transaction.Mutex; -import com.yahoo.vespa.curator.Lock; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.UncheckedIOException; import java.net.URI; -import java.nio.charset.StandardCharsets; import java.security.Principal; import java.time.Clock; import java.time.Duration; @@ -70,7 +68,7 @@ public class AuditLogger { Instant now = clock.instant(); AuditLog.Entry entry = new AuditLog.Entry(now, principal.getName(), method.get(), pathAndQueryOf(request.getUri()), - Optional.of(new String(data, StandardCharsets.UTF_8))); + data); try (Mutex lock = db.lockAuditLog()) { AuditLog auditLog = db.readAuditLog() .pruneBefore(now.minus(entryTtl)) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java index c94c04fc244..38b9d994a6d 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java @@ -8,6 +8,7 @@ import com.yahoo.slime.Slime; import com.yahoo.slime.SlimeUtils; import com.yahoo.vespa.hosted.controller.auditlog.AuditLog; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; @@ -57,6 +58,8 @@ public class AuditLogSerializer { methodFrom(entryObject.field(methodField)), entryObject.field(resourceField).asString(), SlimeUtils.optionalString(entryObject.field(dataField)) + .map(s -> s.getBytes(StandardCharsets.UTF_8)) + .orElseGet(() -> new byte[0]) )); }); return new AuditLog(entries); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializerTest.java index d9e2e61b868..c047f31e171 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializerTest.java @@ -4,10 +4,10 @@ package com.yahoo.vespa.hosted.controller.persistence; import com.yahoo.vespa.hosted.controller.auditlog.AuditLog; import org.junit.Test; +import java.nio.charset.StandardCharsets; import java.time.Duration; import java.time.Instant; import java.util.List; -import java.util.Optional; import static java.time.temporal.ChronoUnit.MILLIS; import static org.junit.Assert.assertEquals; @@ -28,16 +28,19 @@ public class AuditLogSerializerTest { AuditLog log = new AuditLog(List.of( new AuditLog.Entry(i1, "bar", AuditLog.Entry.Method.POST, "/bar/baz/", - Optional.of("0".repeat(2048))), + "0".repeat(2048).getBytes(StandardCharsets.UTF_8)), new AuditLog.Entry(i2, "foo", AuditLog.Entry.Method.POST, "/foo/bar/", - Optional.of("{\"foo\":\"bar\"}")), + "{\"foo\":\"bar\"}".getBytes(StandardCharsets.UTF_8)), new AuditLog.Entry(i3, "baz", AuditLog.Entry.Method.POST, "/foo/baz/", - Optional.of("")), + new byte[0]), new AuditLog.Entry(i4, "baz", AuditLog.Entry.Method.POST, "/foo/baz/", - Optional.of("\ufdff\ufeff\uffff")) // non-ascii + "000\ufdff\ufeff\uffff000".getBytes(StandardCharsets.UTF_8)), // non-ascii + new AuditLog.Entry(i4, "quux", AuditLog.Entry.Method.POST, + "/foo/quux/", + new byte[]{(byte) 0xDE, (byte) 0xAD, (byte) 0xBE, (byte) 0xEF}) // garbage )); AuditLogSerializer serializer = new AuditLogSerializer(); @@ -58,6 +61,7 @@ public class AuditLogSerializerTest { assertEquals(1024, log.entries().get(0).data().get().length()); assertTrue(log.entries().get(2).data().isEmpty()); assertTrue(log.entries().get(3).data().isEmpty()); + assertTrue(log.entries().get(4).data().isEmpty()); } } |