Merge pull request #15550 from dashevchenko/credentialValidation

Hardened device credentials validation

common/data/src/main/java/org/thingsboard/server/common/data/StringUtils.java

@@ -24,6 +24,7 @@ import java.util.Arrays;
 import java.util.Base64;
 import java.util.List;
 import java.util.function.Function;
+import java.util.regex.Pattern;
 
 import static org.apache.commons.lang3.StringUtils.repeat;
 
@@ -37,6 +38,12 @@ public class StringUtils {
 
     public static final int INDEX_NOT_FOUND = -1;
 
+    public static final Pattern CONTROL_CHARS = Pattern.compile("[\\x00-\\x1F\\x7F]");
+
+    public static boolean containsControlChars(String source) {
+        return source != null && CONTROL_CHARS.matcher(source).find();
+    }
+
     public static boolean isEmpty(String source) {
         return source == null || source.isEmpty();
     }

dao/src/main/java/org/thingsboard/server/dao/service/validator/DeviceCredentialsDataValidator.java

@@ -18,10 +18,13 @@ package org.thingsboard.server.dao.service.validator;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.annotation.Lazy;
 import org.springframework.stereotype.Component;
+import org.thingsboard.common.util.JacksonUtil;
 import org.thingsboard.server.common.data.Device;
 import org.thingsboard.server.common.data.StringUtils;
+import org.thingsboard.server.common.data.device.credentials.BasicMqttCredentials;
 import org.thingsboard.server.common.data.id.TenantId;
 import org.thingsboard.server.common.data.security.DeviceCredentials;
+import org.thingsboard.server.common.data.security.DeviceCredentialsType;
 import org.thingsboard.server.dao.device.DeviceCredentialsDao;
 import org.thingsboard.server.dao.device.DeviceService;
 import org.thingsboard.server.dao.exception.DeviceCredentialsValidationException;
@@ -69,9 +72,24 @@ public class DeviceCredentialsDataValidator extends DataValidator<DeviceCredenti
         if (StringUtils.isEmpty(deviceCredentials.getCredentialsId())) {
             throw new DeviceCredentialsValidationException("Device credentials id should be specified!");
         }
+        rejectControlChars(deviceCredentials.getCredentialsId(), "credentialsId");
+        if (deviceCredentials.getCredentialsType() == DeviceCredentialsType.MQTT_BASIC) {
+            BasicMqttCredentials mqtt = JacksonUtil.fromString(deviceCredentials.getCredentialsValue(), BasicMqttCredentials.class);
+            if (mqtt != null) {
+                rejectControlChars(mqtt.getClientId(), "clientId");
+                rejectControlChars(mqtt.getUserName(), "userName");
+                rejectControlChars(mqtt.getPassword(), "password");
+            }
+        }
         Device device = deviceService.findDeviceById(tenantId, deviceCredentials.getDeviceId());
         if (device == null) {
             throw new DeviceCredentialsValidationException("Can't assign device credentials to non-existent device!");
         }
     }
+
+    private static void rejectControlChars(String value, String fieldName) {
+        if (StringUtils.containsControlChars(value)) {
+            throw new DeviceCredentialsValidationException(fieldName + " must not contain control characters!");
+        }
+    }
 }

dao/src/main/java/org/thingsboard/server/dao/util/DeviceConnectivityUtil.java

@@ -51,9 +51,27 @@ public class DeviceConnectivityUtil {
     public static final String COAP_IMAGE = "thingsboard/coap-clients ";
     private final static Pattern VALID_URL_PATTERN = Pattern.compile("^(https?)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]");
 
+    private static String stripControlChars(String value) {
+        return value == null ? null : StringUtils.CONTROL_CHARS.matcher(value).replaceAll("_");
+    }
+
+    // Escapes a value that is interpolated inside a double-quoted shell argument (e.g. -u "...") in the
+    // publish commands shown to the operator, so it cannot break out of the quotes or trigger command/
+    // variable substitution. Control chars are stripped first to keep the command on a single line.
+    private static String escapeShellArg(String value) {
+        if (value == null) {
+            return null;
+        }
+        return stripControlChars(value)
+                .replace("\\", "\\\\")
+                .replace("\"", "\\\"")
+                .replace("$", "\\$")
+                .replace("`", "\\`");
+    }
+
     public static String getHttpPublishCommand(String protocol, String host, String port, DeviceCredentials deviceCredentials) {
         return String.format("curl -v -X POST %s://%s%s/api/v1/%s/telemetry --header Content-Type:application/json --data " + JSON_EXAMPLE_PAYLOAD,
-                protocol, host, port, deviceCredentials.getCredentialsId());
+                protocol, host, port, stripControlChars(deviceCredentials.getCredentialsId()));
     }
 
     public static String getMqttPublishCommand(String protocol, String host, String port, String deviceTelemetryTopic, DeviceCredentials deviceCredentials) {
@@ -66,20 +84,20 @@ public class DeviceConnectivityUtil {
 
         switch (deviceCredentials.getCredentialsType()) {
             case ACCESS_TOKEN:
-                command.append(" -u \"").append(deviceCredentials.getCredentialsId()).append("\"");
+                command.append(" -u \"").append(escapeShellArg(deviceCredentials.getCredentialsId())).append("\"");
                 break;
             case MQTT_BASIC:
                 BasicMqttCredentials credentials = JacksonUtil.fromString(deviceCredentials.getCredentialsValue(),
                         BasicMqttCredentials.class);
                 if (credentials != null) {
                     if (StringUtils.isNotEmpty(credentials.getClientId())) {
-                        command.append(" -i \"").append(credentials.getClientId()).append("\"");
+                        command.append(" -i \"").append(escapeShellArg(credentials.getClientId())).append("\"");
                     }
                     if (StringUtils.isNotEmpty(credentials.getUserName())) {
-                        command.append(" -u \"").append(credentials.getUserName()).append("\"");
+                        command.append(" -u \"").append(escapeShellArg(credentials.getUserName())).append("\"");
                     }
                     if (StringUtils.isNotEmpty(credentials.getPassword())) {
-                        command.append(" -P \"").append(credentials.getPassword()).append("\"");
+                        command.append(" -P \"").append(escapeShellArg(credentials.getPassword())).append("\"");
                     }
                 } else {
                     return null;
@@ -117,12 +135,12 @@ public class DeviceConnectivityUtil {
         dockerComposeBuilder.append("\n");
         dockerComposeBuilder.append("    # Environment variables\n");
         dockerComposeBuilder.append("    environment:\n");
-        dockerComposeBuilder.append("      - TB_GW_HOST=").append(isLocalhost(host) ? HOST_DOCKER_INTERNAL : host).append("\n");
+        dockerComposeBuilder.append("      - TB_GW_HOST=").append(stripControlChars(isLocalhost(host) ? HOST_DOCKER_INTERNAL : host)).append("\n");
         dockerComposeBuilder.append("      - TB_GW_PORT=1883\n");
         switch (deviceCredentials.getCredentialsType()) {
             case ACCESS_TOKEN:
                 dockerComposeBuilder.append("      - TB_GW_SECURITY_TYPE=accessToken\n");
-                dockerComposeBuilder.append("      - TB_GW_ACCESS_TOKEN=").append(deviceCredentials.getCredentialsId()).append("\n");
+                dockerComposeBuilder.append("      - TB_GW_ACCESS_TOKEN=").append(stripControlChars(deviceCredentials.getCredentialsId())).append("\n");
                 break;
             case MQTT_BASIC:
                 dockerComposeBuilder.append("      - TB_GW_SECURITY_TYPE=usernamePassword\n");
@@ -130,13 +148,13 @@ public class DeviceConnectivityUtil {
                         BasicMqttCredentials.class);
                 if (credentials != null) {
                     if (StringUtils.isNotEmpty(credentials.getClientId())) {
-                        dockerComposeBuilder.append("      - TB_GW_CLIENT_ID=").append(credentials.getClientId()).append("\n");
+                        dockerComposeBuilder.append("      - TB_GW_CLIENT_ID=").append(stripControlChars(credentials.getClientId())).append("\n");
                     }
                     if (StringUtils.isNotEmpty(credentials.getUserName())) {
-                        dockerComposeBuilder.append("      - TB_GW_USERNAME=").append(credentials.getUserName()).append("\n");
+                        dockerComposeBuilder.append("      - TB_GW_USERNAME=").append(stripControlChars(credentials.getUserName())).append("\n");
                     }
                     if (StringUtils.isNotEmpty(credentials.getPassword())) {
-                        dockerComposeBuilder.append("      - TB_GW_PASSWORD=").append(credentials.getPassword()).append("\n");
+                        dockerComposeBuilder.append("      - TB_GW_PASSWORD=").append(stripControlChars(credentials.getPassword())).append("\n");
                     }
                 }
                 break;
@@ -201,7 +219,7 @@ public class DeviceConnectivityUtil {
                 String client = COAPS.equals(protocol) ? "coap-client-openssl" : "coap-client";
                 String certificate = COAPS.equals(protocol) ? " -R " + CA_ROOT_CERT_PEM : "";
                 return String.format("%s -v 6 -m POST%s -t \"application/json\" -e %s %s://%s%s/api/v1/%s/telemetry",
-                        client, certificate, JSON_EXAMPLE_PAYLOAD, protocol, host, port, deviceCredentials.getCredentialsId());
+                        client, certificate, JSON_EXAMPLE_PAYLOAD, protocol, host, port, stripControlChars(deviceCredentials.getCredentialsId()));
             default:
                 return null;
         }

dao/src/test/java/org/thingsboard/server/dao/service/validator/DeviceCredentialsDataValidatorTest.java

@@ -0,0 +1,138 @@
+/**
+ * Copyright © 2016-2026 The Thingsboard Authors
+ *
+ * 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 org.thingsboard.server.dao.service.validator;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.thingsboard.common.util.JacksonUtil;
+import org.thingsboard.server.common.data.Device;
+import org.thingsboard.server.common.data.device.credentials.BasicMqttCredentials;
+import org.thingsboard.server.common.data.id.DeviceId;
+import org.thingsboard.server.common.data.id.TenantId;
+import org.thingsboard.server.common.data.security.DeviceCredentials;
+import org.thingsboard.server.common.data.security.DeviceCredentialsType;
+import org.thingsboard.server.dao.device.DeviceCredentialsDao;
+import org.thingsboard.server.dao.device.DeviceService;
+import org.thingsboard.server.dao.exception.DeviceCredentialsValidationException;
+
+import java.util.UUID;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.BDDMockito.willReturn;
+
+@ExtendWith(MockitoExtension.class)
+class DeviceCredentialsDataValidatorTest {
+
+    @Mock
+    DeviceCredentialsDao deviceCredentialsDao;
+    @Mock
+    DeviceService deviceService;
+    @InjectMocks
+    DeviceCredentialsDataValidator validator;
+
+    final TenantId tenantId = TenantId.fromUUID(UUID.fromString("9ef79cdf-37a8-4119-b682-2e7ed4e018da"));
+    final DeviceId deviceId = new DeviceId(UUID.fromString("11111111-1111-1111-1111-111111111111"));
+
+    @Test
+    void rejectsNewlineInAccessToken() {
+        DeviceCredentials creds = accessToken("safe_token\nentrypoint: [\"/bin/sh\"]");
+
+        assertThatThrownBy(() -> validator.validateDataImpl(tenantId, creds))
+                .isInstanceOf(DeviceCredentialsValidationException.class)
+                .hasMessageContaining("credentialsId")
+                .hasMessageContaining("control characters");
+    }
+
+    @Test
+    void rejectsCarriageReturnInAccessToken() {
+        DeviceCredentials creds = accessToken("token\rprivileged: true");
+
+        assertThatThrownBy(() -> validator.validateDataImpl(tenantId, creds))
+                .isInstanceOf(DeviceCredentialsValidationException.class)
+                .hasMessageContaining("control characters");
+    }
+
+    @Test
+    void rejectsNewlineInMqttClientId() {
+        DeviceCredentials creds = mqttBasic("cid\nentrypoint: x", "user", "pwd");
+
+        assertThatThrownBy(() -> validator.validateDataImpl(tenantId, creds))
+                .isInstanceOf(DeviceCredentialsValidationException.class)
+                .hasMessageContaining("clientId");
+    }
+
+    @Test
+    void rejectsNewlineInMqttUserName() {
+        DeviceCredentials creds = mqttBasic("cid", "user\nprivileged: true", "pwd");
+
+        assertThatThrownBy(() -> validator.validateDataImpl(tenantId, creds))
+                .isInstanceOf(DeviceCredentialsValidationException.class)
+                .hasMessageContaining("userName");
+    }
+
+    @Test
+    void rejectsNewlineInMqttPassword() {
+        DeviceCredentials creds = mqttBasic("cid", "user", "pwd\nentrypoint: x");
+
+        assertThatThrownBy(() -> validator.validateDataImpl(tenantId, creds))
+                .isInstanceOf(DeviceCredentialsValidationException.class)
+                .hasMessageContaining("password");
+    }
+
+    @Test
+    void acceptsValidCredentials() {
+        willReturn(new Device()).given(deviceService).findDeviceById(tenantId, deviceId);
+        DeviceCredentials creds = accessToken("safe_token_123");
+
+        assertThatCode(() -> validator.validateDataImpl(tenantId, creds))
+                .doesNotThrowAnyException();
+    }
+
+    @Test
+    void acceptsValidMqttBasicCredentials() {
+        willReturn(new Device()).given(deviceService).findDeviceById(tenantId, deviceId);
+        DeviceCredentials creds = mqttBasic("client-1", "user-1", "pwd-1");
+
+        assertThatCode(() -> validator.validateDataImpl(tenantId, creds))
+                .doesNotThrowAnyException();
+    }
+
+    private DeviceCredentials accessToken(String token) {
+        DeviceCredentials c = new DeviceCredentials();
+        c.setDeviceId(deviceId);
+        c.setCredentialsType(DeviceCredentialsType.ACCESS_TOKEN);
+        c.setCredentialsId(token);
+        return c;
+    }
+
+    private DeviceCredentials mqttBasic(String clientId, String userName, String password) {
+        BasicMqttCredentials inner = new BasicMqttCredentials();
+        inner.setClientId(clientId);
+        inner.setUserName(userName);
+        inner.setPassword(password);
+        DeviceCredentials c = new DeviceCredentials();
+        c.setDeviceId(deviceId);
+        c.setCredentialsType(DeviceCredentialsType.MQTT_BASIC);
+        c.setCredentialsId("mqtt-credentials-id");
+        c.setCredentialsValue(JacksonUtil.toString(inner));
+        return c;
+    }
+
+}

dao/src/test/java/org/thingsboard/server/dao/util/DeviceConnectivityUtilTest.java

@@ -16,6 +16,13 @@
 package org.thingsboard.server.dao.util;
 
 import org.junit.jupiter.api.Test;
+import org.thingsboard.common.util.JacksonUtil;
+import org.thingsboard.server.common.data.device.credentials.BasicMqttCredentials;
+import org.thingsboard.server.common.data.security.DeviceCredentials;
+import org.thingsboard.server.common.data.security.DeviceCredentialsType;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -29,4 +36,120 @@ class DeviceConnectivityUtilTest {
         assertThat(DeviceConnectivityUtil.CA_ROOT_CERT_PEM).doesNotContainAnyWhitespaces();
     }
 
+    @Test
+    void validAccessTokenIsRenderedAsIs() throws Exception {
+        String yaml = renderCompose(accessToken("safe_token_123"));
+
+        assertThat(yaml).contains("- TB_GW_ACCESS_TOKEN=safe_token_123\n");
+        assertNoInjectedSiblingKeys(yaml);
+    }
+
+    @Test
+    void newlineInAccessTokenIsSanitized() throws Exception {
+        String malicious = "safe_token\n    entrypoint: [\"/bin/bash\",\"-c\",\"id\"]";
+
+        String yaml = renderCompose(accessToken(malicious));
+
+        assertNoInjectedSiblingKeys(yaml);
+    }
+
+    @Test
+    void carriageReturnInAccessTokenIsSanitized() throws Exception {
+        String yaml = renderCompose(accessToken("token\rprivileged: true"));
+
+        assertNoInjectedSiblingKeys(yaml);
+    }
+
+    @Test
+    void newlineInMqttClientIdIsSanitized() throws Exception {
+        String yaml = renderCompose(mqttBasic("cid\n    entrypoint: [\"/bin/sh\"]", "user", "pwd"));
+
+        assertNoInjectedSiblingKeys(yaml);
+    }
+
+    @Test
+    void newlineInMqttUserNameIsSanitized() throws Exception {
+        String yaml = renderCompose(mqttBasic("cid", "user\n    privileged: true", "pwd"));
+
+        assertNoInjectedSiblingKeys(yaml);
+    }
+
+    @Test
+    void newlineInMqttPasswordIsSanitized() throws Exception {
+        String yaml = renderCompose(mqttBasic("cid", "user", "pwd\n    entrypoint: [\"/bin/sh\"]"));
+
+        assertNoInjectedSiblingKeys(yaml);
+    }
+
+    @Test
+    void mqttBasicQuoteInUserNameIsEscapedInPublishCommand() {
+        String command = DeviceConnectivityUtil.getMqttPublishCommand(
+                "mqtt", "localhost", "1883", "v1/devices/me/telemetry",
+                mqttBasic("cid", "u\";touch pwned;echo \"", "pwd"));
+
+        // the double quote must be backslash-escaped so it cannot terminate the -u "..." argument
+        assertThat(command).contains("-u \"u\\\";touch pwned;echo \\\"\"");
+        assertThat(command).doesNotContain("-u \"u\";");
+    }
+
+    @Test
+    void controlCharsInMqttClientIdAreStrippedInPublishCommand() {
+        String command = DeviceConnectivityUtil.getMqttPublishCommand(
+                "mqtt", "localhost", "1883", "v1/devices/me/telemetry",
+                mqttBasic("c\nid", "user", "pwd"));
+
+        assertThat(command).doesNotContain("\n");
+        assertThat(command).contains("-i \"c_id\"");
+    }
+
+    @Test
+    void controlCharsInAccessTokenAreStrippedInHttpAndCoapCommands() {
+        DeviceCredentials creds = accessToken("tok\nen");
+
+        assertThat(DeviceConnectivityUtil.getHttpPublishCommand("http", "localhost", ":8080", creds))
+                .doesNotContain("\n")
+                .contains("/api/v1/tok_en/telemetry");
+        assertThat(DeviceConnectivityUtil.getCoapPublishCommand("coap", "localhost", ":5683", creds))
+                .doesNotContain("\n")
+                .contains("/api/v1/tok_en/telemetry");
+    }
+
+    private static String renderCompose(DeviceCredentials credentials) throws Exception {
+        var resource = DeviceConnectivityUtil.getGatewayDockerComposeFile(
+                "host.docker.internal", "3.8-stable", credentials);
+        try (var in = resource.getInputStream()) {
+            return new String(in.readAllBytes(), StandardCharsets.UTF_8);
+        }
+    }
+
+    private static DeviceCredentials accessToken(String token) {
+        DeviceCredentials c = new DeviceCredentials();
+        c.setCredentialsType(DeviceCredentialsType.ACCESS_TOKEN);
+        c.setCredentialsId(token);
+        return c;
+    }
+
+    private static DeviceCredentials mqttBasic(String clientId, String userName, String password) {
+        BasicMqttCredentials inner = new BasicMqttCredentials();
+        inner.setClientId(clientId);
+        inner.setUserName(userName);
+        inner.setPassword(password);
+        DeviceCredentials c = new DeviceCredentials();
+        c.setCredentialsType(DeviceCredentialsType.MQTT_BASIC);
+        c.setCredentialsId("mqtt-credentials-id");
+        c.setCredentialsValue(JacksonUtil.toString(inner));
+        return c;
+    }
+
+    private static void assertNoInjectedSiblingKeys(String yaml) throws IOException {
+        for (String line : yaml.split("\n")) {
+            String trimmed = line.replaceFirst("^\\s+", "");
+            assertThat(trimmed)
+                    .as("unexpected sibling key — possible YAML injection: %s", line)
+                    .doesNotStartWith("entrypoint:")
+                    .doesNotStartWith("privileged:")
+                    .doesNotStartWith("command:");
+        }
+    }
+
 }