Add configuration to disable signing (Fixes #817) (#848)

Signed-off-by: Jeroen van Erp <[email protected]>
hierynomus · Dec 9, 2024 · 9df23a1 · 9df23a1
1 parent 9fdc5b4
commit 9df23a1
Show file tree

Hide file tree

Showing 11 changed files with 168 additions and 16 deletions.
diff --git a/src/it/java/com/hierynomus/smbj/IntegrationTest.java b/src/it/java/com/hierynomus/smbj/IntegrationTest.java
@@ -30,6 +30,7 @@
 import org.testcontainers.junit.jupiter.Testcontainers;
 
 import com.hierynomus.msfscc.fileinformation.FileIdBothDirectoryInformation;
+import com.hierynomus.mssmb2.SMB2Dialect;
 import com.hierynomus.smbj.share.DiskShare;
 import com.hierynomus.smbj.share.Share;
 import com.hierynomus.smbj.testcontainers.SambaContainer;
@@ -89,6 +90,17 @@ public void shouldListContentsOfShareWithNullPath() throws Exception {
         });
     }
 
+    @Test
+    public void shouldConnectAndListContentsOfShareWithDisabledSigning() throws Exception {
+        SmbConfig c = SmbConfig.builder().withDialects(SMB2Dialect.SMB_2_0_2).withMultiProtocolNegotiate(true).withSigningEnabled(false).withEncryptData(false).build();
+        samba.withAuthenticatedClient(c, DEFAULT_AUTHENTICATION_CONTEXT, session -> {
+            try (DiskShare share = (DiskShare) session.connectShare("user")) {
+                List<FileIdBothDirectoryInformation> items = share.list("");
+                assertThat(items).hasSize(2).map(FileIdBothDirectoryInformation::getFileName).contains(".", "..");
+            }
+        });
+    }
+
     @ParameterizedTest(name = "should not fail closing connection twice")
     @MethodSource("com.hierynomus.smbj.testing.TestingUtils#defaultTestingConfig")
     public void shouldNotFailClosingConnectionTwice(SmbConfig c) throws Exception {

diff --git a/src/it/java/com/hierynomus/smbj/testing/TestingUtils.java b/src/it/java/com/hierynomus/smbj/testing/TestingUtils.java
@@ -66,6 +66,7 @@ public static Stream<Arguments> validConfigs() {
     public static Stream<Arguments> defaultTestingConfig() {
         return Stream.of(Arguments.of(config(SMB2Dialect.SMB_3_1_1, true, true)));
     }
+
     public static Stream<Arguments> dfsConfig() {
         return Stream.of(Arguments.of(config(SMB2Dialect.SMB_2_1, false, true)));
     }

diff --git a/src/main/java/com/hierynomus/smbj/SmbConfig.java b/src/main/java/com/hierynomus/smbj/SmbConfig.java
@@ -21,6 +21,7 @@
 import static com.hierynomus.mssmb2.SMB2Dialect.SMB_3_0_2;
 import static com.hierynomus.mssmb2.SMB2Dialect.SMB_3_1_1;
 
+import java.lang.reflect.InvocationTargetException;
 import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -77,6 +78,7 @@ public final class SmbConfig {
     private Random random;
     private UUID clientGuid;
     private boolean signingRequired;
+    private boolean signingEnabled;
     private boolean dfsEnabled;
     private boolean useMultiProtocolNegotiate;
     private SecurityProvider securityProvider;
@@ -103,6 +105,7 @@ public static Builder builder() {
                 .withSecurityProvider(getDefaultSecurityProvider())
                 .withSocketFactory(new ProxySocketFactory())
                 .withSigningRequired(false)
+                .withSigningEnabled(true)
                 .withDfsEnabled(false)
                 .withMultiProtocolNegotiate(false)
                 .withBufferSize(DEFAULT_BUFFER_SIZE)
@@ -131,9 +134,9 @@ private static List<Factory.Named<Authenticator>> getDefaultAuthenticators() {
 
         if (!ANDROID) {
             try {
-                Object spnegoFactory = Class.forName("com.hierynomus.smbj.auth.SpnegoAuthenticator$Factory").newInstance();
+                Object spnegoFactory = Class.forName("com.hierynomus.smbj.auth.SpnegoAuthenticator$Factory").getDeclaredConstructor().newInstance();
                 authenticators.add((Factory.Named<Authenticator>)spnegoFactory);
-            } catch (InstantiationException | IllegalAccessException | ClassNotFoundException | ClassCastException e) {
+            } catch (InstantiationException | IllegalAccessException | ClassNotFoundException | ClassCastException | NoSuchMethodException | InvocationTargetException e) {
                 throw new SMBRuntimeException(e);
             }
         }
@@ -156,6 +159,7 @@ private SmbConfig(SmbConfig other) {
         random = other.random;
         clientGuid = other.clientGuid;
         signingRequired = other.signingRequired;
+        signingEnabled = other.signingEnabled;
         dfsEnabled = other.dfsEnabled;
         securityProvider = other.securityProvider;
         readBufferSize = other.readBufferSize;
@@ -200,6 +204,16 @@ public boolean isSigningRequired() {
         return signingRequired;
     }
 
+    /**
+     * Whether the client should sign messages to the server.  When message signing is enabled the client will sign messages to the server.
+     */
+    public boolean isSigningEnabled() {
+        return signingEnabled;
+    }
+
+    /**
+     * Whether the client should use the DFS protocol.
+     */
     public boolean isDfsEnabled() {
         return dfsEnabled;
     }
@@ -369,6 +383,11 @@ public Builder withSigningRequired(boolean signingRequired) {
             return this;
         }
 
+        public Builder withSigningEnabled(boolean signingEnabled) {
+            config.signingEnabled = signingEnabled;
+            return this;
+        }
+
         public Builder withReadBufferSize(int readBufferSize) {
             if (readBufferSize <= 0) {
                 throw new IllegalArgumentException("Read buffer size must be greater than zero");
@@ -453,6 +472,14 @@ public SmbConfig build() {
                 throw new IllegalStateException("At least one SMB dialect should be specified");
             }
 
+            if (config.signingRequired && !config.signingEnabled) {
+                throw new IllegalStateException("If signing is required, it should also be enabled");
+            }
+
+            if (!config.signingEnabled && SMB2Dialect.supportsSmb3x(config.dialects)) {
+                throw new IllegalStateException("Signing cannot be disabled when using SMB3.x dialects");
+            }
+
             if (config.encryptData && !SMB2Dialect.supportsSmb3x(config.dialects)) {
                 throw new IllegalStateException("If encryption is enabled, at least one dialect should be SMB3.x compatible");
             }

diff --git a/src/main/java/com/hierynomus/smbj/connection/Connection.java b/src/main/java/com/hierynomus/smbj/connection/Connection.java
@@ -86,7 +86,7 @@ public class Connection extends Pooled<Connection> implements Closeable, PacketR
     private final SMBClient client;
     final ServerList serverList;
 
-    private PacketSignatory signatory;
+    private Signatory signatory;
     private PacketEncryptor encryptor;
 
     public SMBClient getClient() {
@@ -110,7 +110,12 @@ public Connection(SmbConfig config, SMBClient client, SMBEventBus bus, ServerLis
     private void init() {
         bus.subscribe(this);
         this.sequenceWindow = new SequenceWindow();
-        this.signatory = new PacketSignatory(config.getSecurityProvider());
+        if (config.isSigningEnabled()) {
+            this.signatory = new PacketSignatory(config.getSecurityProvider());
+        } else {
+            logger.warn("Signing is disabled for this connection.");
+            this.signatory = new NoSignatory();
+        }
         this.encryptor = new PacketEncryptor(config.getSecurityProvider());
 
         this.packetHandlerChain = new SMB3DecryptingPacketHandler(sessionTable, encryptor).setNext(
@@ -139,7 +144,6 @@ public void connect(String hostname, int port) throws IOException {
         transport.connect(new InetSocketAddress(hostname, port));
         this.connectionContext = new ConnectionContext(config.getClientGuid(), hostname, port, config);
         new SMBProtocolNegotiator(this, config, connectionContext).negotiateDialect();
-        this.signatory.init();
         this.encryptor.init(connectionContext);
 
         this.pathResolver = new SymlinkPathResolver(PathResolver.LOCAL);

diff --git a/src/main/java/com/hierynomus/smbj/connection/NoSignatory.java b/src/main/java/com/hierynomus/smbj/connection/NoSignatory.java
@@ -0,0 +1,36 @@
+/*
+ * Copyright (C)2016 - SMBJ Contributors
+ *
+ * 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.hierynomus.smbj.connection;
+
+import javax.crypto.SecretKey;
+
+import com.hierynomus.mssmb2.SMB2Packet;
+import com.hierynomus.mssmb2.SMB2PacketData;
+
+/**
+ * When signing is disabled, this class is used to sign and verify packets so that the code does not need to take the lack of signing into account.
+ */
+public class NoSignatory implements Signatory {
+    @Override
+    public SMB2Packet sign(SMB2Packet packet, SecretKey secretKey) {
+        return packet;
+    }
+
+    @Override
+    public boolean verify(SMB2PacketData packet, SecretKey secretKey) {
+        return true;
+    }
+}
diff --git a/src/main/java/com/hierynomus/smbj/connection/PacketSignatory.java b/src/main/java/com/hierynomus/smbj/connection/PacketSignatory.java
@@ -37,7 +37,7 @@
 import com.hierynomus.security.SecurityProvider;
 import com.hierynomus.smb.SMBBuffer;
 
-public class PacketSignatory {
+public class PacketSignatory implements Signatory {
     private static final Logger logger = LoggerFactory.getLogger(PacketSignatory.class);
 
     private SecurityProvider securityProvider;
@@ -46,9 +46,7 @@ public class PacketSignatory {
         this.securityProvider = securityProvider;
     }
 
-    void init() {
-    }
-
+    @Override
     public SMB2Packet sign(SMB2Packet packet, SecretKey secretKey) {
         if (secretKey != null) {
             return new SignedPacketWrapper(packet, secretKey);
@@ -59,6 +57,7 @@ public SMB2Packet sign(SMB2Packet packet, SecretKey secretKey) {
     }
 
     // TODO make session a packet handler which wraps the incoming packets
+    @Override
     public boolean verify(SMB2PacketData packet, SecretKey secretKey) {
         try {
             SMBBuffer buffer = packet.getDataBuffer();

diff --git a/src/main/java/com/hierynomus/smbj/connection/Signatory.java b/src/main/java/com/hierynomus/smbj/connection/Signatory.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C)2016 - SMBJ Contributors
+ *
+ * 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.hierynomus.smbj.connection;
+
+import javax.crypto.SecretKey;
+
+import com.hierynomus.mssmb2.SMB2Packet;
+import com.hierynomus.mssmb2.SMB2PacketData;
+
+public interface Signatory {
+    public SMB2Packet sign(SMB2Packet packet, SecretKey secretKey);
+
+    public boolean verify(SMB2PacketData packet, SecretKey secretKey);
+}
diff --git a/...in/java/com/hierynomus/smbj/connection/packet/SMB2SignatureVerificationPacketHandler.java b/...in/java/com/hierynomus/smbj/connection/packet/SMB2SignatureVerificationPacketHandler.java
@@ -18,8 +18,8 @@
 import com.hierynomus.mssmb2.DeadLetterPacketData;
 import com.hierynomus.mssmb2.SMB2PacketData;
 import com.hierynomus.protocol.transport.TransportException;
-import com.hierynomus.smbj.connection.PacketSignatory;
 import com.hierynomus.smbj.connection.SessionTable;
+import com.hierynomus.smbj.connection.Signatory;
 import com.hierynomus.smbj.session.Session;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -67,9 +67,9 @@
 public class SMB2SignatureVerificationPacketHandler extends SMB2PacketHandler {
     private static final Logger logger = LoggerFactory.getLogger(SMB2SignatureVerificationPacketHandler.class);
     private SessionTable sessionTable;
-    private PacketSignatory signatory;
+    private Signatory signatory;
 
-    public SMB2SignatureVerificationPacketHandler(SessionTable sessionTable, PacketSignatory signatory) {
+    public SMB2SignatureVerificationPacketHandler(SessionTable sessionTable, Signatory signatory) {
         this.sessionTable = sessionTable;
         this.signatory = signatory;
     }

diff --git a/src/main/java/com/hierynomus/smbj/session/Session.java b/src/main/java/com/hierynomus/smbj/session/Session.java
@@ -29,7 +29,7 @@
 import com.hierynomus.smbj.common.SmbPath;
 import com.hierynomus.smbj.connection.Connection;
 import com.hierynomus.smbj.connection.PacketEncryptor;
-import com.hierynomus.smbj.connection.PacketSignatory;
+import com.hierynomus.smbj.connection.Signatory;
 import com.hierynomus.smbj.event.SMBEventBus;
 import com.hierynomus.smbj.event.SessionLoggedOff;
 import com.hierynomus.smbj.event.TreeDisconnected;
@@ -61,15 +61,15 @@ public class Session implements AutoCloseable {
     private final SmbConfig config;
     private SMBEventBus bus;
     private final PathResolver pathResolver;
-    private PacketSignatory signatory;
+    private Signatory signatory;
     private PacketEncryptor encryptor;
     private TreeConnectTable treeConnectTable = new TreeConnectTable();
     private Map<String, Session> nestedSessionsByHost = new HashMap<>();
     private ReentrantReadWriteLock nestedSessionsRwLock = new ReentrantReadWriteLock();
     private AuthenticationContext userCredentials;
     private SessionContext sessionContext;
 
-    public Session(Connection connection, SmbConfig config, AuthenticationContext userCredentials, SMBEventBus bus, PathResolver pathResolver, PacketSignatory signatory, PacketEncryptor encryptor) {
+    public Session(Connection connection, SmbConfig config, AuthenticationContext userCredentials, SMBEventBus bus, PathResolver pathResolver, Signatory signatory, PacketEncryptor encryptor) {
         this.connection = connection;
         this.config = config;
         this.userCredentials = userCredentials;

diff --git a/src/test/java/com/hierynomus/smbj/SmbConfigTest.java b/src/test/java/com/hierynomus/smbj/SmbConfigTest.java
@@ -0,0 +1,42 @@
+/*
+ * Copyright (C)2016 - SMBJ Contributors
+ *
+ * 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.hierynomus.smbj;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
+import org.junit.jupiter.api.Test;
+
+import com.hierynomus.mssmb2.SMB2Dialect;
+
+public class SmbConfigTest {
+    @Test
+    public void testCreateDefaultConfig() {
+        assertDoesNotThrow(() -> SmbConfig.createDefaultConfig());
+    }
+
+    @Test
+    public void shouldNotBuildConfigWithRequiredAndDisabledSigning() {
+        assertThrows(IllegalStateException.class,
+                () -> SmbConfig.builder().withDialects(SMB2Dialect.SMB_2_0_2).withSigningRequired(true).withSigningEnabled(false).build());
+    }
+
+    @Test
+    public void shouldNotBuildConfigWithDisabledSigningAndSmb3xDialect() {
+        assertThrows(IllegalStateException.class,
+                () -> SmbConfig.builder().withDialects(SMB2Dialect.SMB_3_0).withSigningEnabled(false).build());
+    }
+}
diff --git a/src/test/java/com/hierynomus/smbj/testing/Utils.java b/src/test/java/com/hierynomus/smbj/testing/Utils.java
@@ -20,8 +20,12 @@
 
 public class Utils {
     public static SmbConfig config(PacketProcessor processor) {
+        return configBuilder(processor).build();
+    }
+
+    public static SmbConfig.Builder configBuilder(PacketProcessor processor) {
         return SmbConfig.builder()
                 .withTransportLayerFactory(new StubTransportLayerFactory<>(new DefaultPacketProcessor().wrap(processor)))
-                .withAuthenticators(new StubAuthenticator.Factory()).build();
+                .withAuthenticators(new StubAuthenticator.Factory());
     }
 }