Add validations of variable byte integers (#2214)

* Add validations and add docs

* Update release notes

* Fix build

Samples/Server/Server_Intercepting_Samples.cs

@@ -9,6 +9,7 @@
 // ReSharper disable MemberCanBeMadeStatic.Local
 
 using MQTTnet.Internal;
+using MQTTnet.Protocol;
 using MQTTnet.Server;
 
 namespace MQTTnet.Samples.Server;
@@ -42,4 +43,32 @@ public static class Server_Intercepting_Samples
         Console.ReadLine();
         await mqttServer.StopAsync();
     }
+
+    public static async Task Intercept_Subscription()
+    {
+        /*
+         * This sample starts a simple MQTT server which does not allow to subscribe to '#'
+         * Start the server and try to subscribe to '#' with a client of choice.
+         */
+
+        var mqttServerFactory = new MqttServerFactory();
+        var mqttServerOptions = new MqttServerOptionsBuilder().WithDefaultEndpoint().Build();
+
+        using var mqttServer = mqttServerFactory.CreateMqttServer(mqttServerOptions);
+        mqttServer.InterceptingSubscriptionAsync += args =>
+        {
+            if (args.TopicFilter.Topic.Equals("#", StringComparison.Ordinal))
+            {
+                args.Response.ReasonCode = MqttSubscribeReasonCode.NotAuthorized;
+            }
+
+            return CompletedTask.Instance;
+        };
+
+        await mqttServer.StartAsync();
+
+        Console.WriteLine("Press Enter to exit.");
+        Console.ReadLine();
+        await mqttServer.StopAsync();
+    }
 }

Source/MQTTnet.Server/Options/MqttServerOptionsBuilder.cs

@@ -5,8 +5,8 @@
 using System.Net;
 using System.Net.Security;
 using System.Security.Authentication;
-using MQTTnet.Certificates;
 using System.Security.Cryptography.X509Certificates;
+using MQTTnet.Certificates;
 
 // ReSharper disable UnusedMember.Global
 namespace MQTTnet.Server;
@@ -47,12 +47,22 @@ public class MqttServerOptionsBuilder
         return this;
     }
 
+    /// <summary>
+    ///     Sets the IPv4 network address to bind.
+    ///     Defaults to any available IPv4 address of the machine.
+    ///     Set this to `IPAddress.None` to disable the IPv4 support entirely.
+    /// </summary>
     public MqttServerOptionsBuilder WithDefaultEndpointBoundIPAddress(IPAddress value)
     {
         _options.DefaultEndpointOptions.BoundInterNetworkAddress = value ?? IPAddress.Any;
         return this;
     }
 
+    /// <summary>
+    ///     Sets the IPv6 network address to bind.
+    ///     Defaults to any available IPv6 address of the machine.
+    ///     Set this to `IPAddress.None` to disable the IPv6 support entirely.
+    /// </summary>
     public MqttServerOptionsBuilder WithDefaultEndpointBoundIPV6Address(IPAddress value)
     {
         _options.DefaultEndpointOptions.BoundInterNetworkV6Address = value ?? IPAddress.Any;
@@ -77,12 +87,22 @@ public class MqttServerOptionsBuilder
         return this;
     }
 
+    /// <summary>
+    ///     Sets the IPv4 network address to bind.
+    ///     Defaults to any available IPv4 address of the machine.
+    ///     Set this to `IPAddress.None` to disable the IPv4 support entirely.
+    /// </summary>
     public MqttServerOptionsBuilder WithEncryptedEndpointBoundIPAddress(IPAddress value)
     {
         _options.TlsEndpointOptions.BoundInterNetworkAddress = value;
         return this;
     }
 
+    /// <summary>
+    ///     Sets the IPv6 network address to bind.
+    ///     Defaults to any available IPv6 address of the machine.
+    ///     Set this to `IPAddress.None` to disable the IPv6 support entirely.
+    /// </summary>
     public MqttServerOptionsBuilder WithEncryptedEndpointBoundIPV6Address(IPAddress value)
     {
         _options.TlsEndpointOptions.BoundInterNetworkV6Address = value;
@@ -95,6 +115,35 @@ public class MqttServerOptionsBuilder
         return this;
     }
 
+    public MqttServerOptionsBuilder WithEncryptionCertificate(byte[] value, IMqttServerCertificateCredentials credentials = null)
+    {
+        ArgumentNullException.ThrowIfNull(value);
+
+        _options.TlsEndpointOptions.CertificateProvider = new BlobCertificateProvider(value)
+        {
+            Password = credentials?.Password
+        };
+
+        return this;
+    }
+
+    public MqttServerOptionsBuilder WithEncryptionCertificate(X509Certificate2 certificate)
+    {
+        ArgumentNullException.ThrowIfNull(certificate);
+
+        _options.TlsEndpointOptions.CertificateProvider = new X509CertificateProvider(certificate);
+        return this;
+    }
+
+    public MqttServerOptionsBuilder WithEncryptionCertificate(ICertificateProvider certificateProvider)
+    {
+        ArgumentNullException.ThrowIfNull(certificateProvider);
+
+        _options.TlsEndpointOptions.CertificateProvider = certificateProvider;
+
+        return this;
+    }
+
     public MqttServerOptionsBuilder WithEncryptionSslProtocol(SslProtocols value)
     {
         _options.TlsEndpointOptions.SslProtocol = value;
@@ -114,12 +163,6 @@ public class MqttServerOptionsBuilder
         return this;
     }
 
-    public MqttServerOptionsBuilder WithPendingMessagesOverflowStrategy(MqttPendingMessagesOverflowStrategy value)
-    {
-        _options.PendingMessagesOverflowStrategy = value;
-        return this;
-    }
-
     public MqttServerOptionsBuilder WithoutDefaultEndpoint()
     {
         _options.DefaultEndpointOptions.IsEnabled = false;
@@ -144,6 +187,12 @@ public class MqttServerOptionsBuilder
         return this;
     }
 
+    public MqttServerOptionsBuilder WithPendingMessagesOverflowStrategy(MqttPendingMessagesOverflowStrategy value)
+    {
+        _options.PendingMessagesOverflowStrategy = value;
+        return this;
+    }
+
     public MqttServerOptionsBuilder WithPersistentSessions(bool value = true)
     {
         _options.EnablePersistentSessions = value;
@@ -182,33 +231,4 @@ public class MqttServerOptionsBuilder
         _options.TlsEndpointOptions.ReuseAddress = true;
         return this;
     }
-
-    public MqttServerOptionsBuilder WithEncryptionCertificate(byte[] value, IMqttServerCertificateCredentials credentials = null)
-    {
-        ArgumentNullException.ThrowIfNull(value);
-
-        _options.TlsEndpointOptions.CertificateProvider = new BlobCertificateProvider(value)
-        {
-            Password = credentials?.Password
-        };
-
-        return this;
-    }
-
-    public MqttServerOptionsBuilder WithEncryptionCertificate(X509Certificate2 certificate)
-    {
-        ArgumentNullException.ThrowIfNull(certificate);
-
-        _options.TlsEndpointOptions.CertificateProvider = new X509CertificateProvider(certificate);
-        return this;
-    }
-
-    public MqttServerOptionsBuilder WithEncryptionCertificate(ICertificateProvider certificateProvider)
-    {
-        ArgumentNullException.ThrowIfNull(certificateProvider);
-
-        _options.TlsEndpointOptions.CertificateProvider = certificateProvider;
-
-        return this;
-    }
 }

Source/MQTTnet.Server/Options/MqttServerTcpEndpointBaseOptions.cs

@@ -9,13 +9,30 @@ namespace MQTTnet.Server;
 
 public abstract class MqttServerTcpEndpointBaseOptions
 {
-    public bool IsEnabled { get; set; }
+    /// <summary>
+    ///     Usually the MQTT packets can be sent partially. This is done by using multiple TCP packets
+    ///     or WebSocket frames etc. Unfortunately not all clients do support this feature and
+    ///     will close the connection when receiving such packets. If such clients are connecting to this
+    ///     server the flag must be set to _false_.
+    /// </summary>
+    public bool AllowPacketFragmentation { get; set; } = true;
 
-    public int Port { get; set; }
+    /// <summary>
+    ///     Gets or sets the IPv4 network address to bind.
+    ///     Defaults to any available IPv4 address of the machine.
+    ///     Set this to `IPAddress.None` to disable the IPv4 support entirely.
+    /// </summary>
+    public IPAddress BoundInterNetworkAddress { get; set; } = IPAddress.Any;
+
+    /// <summary>
+    ///     Gets or sets the IPv6 network address to bind.
+    ///     Defaults to any available IPv6 address of the machine.
+    ///     Set this to `IPAddress.None` to disable the IPv6 support entirely.
+    /// </summary>
+    public IPAddress BoundInterNetworkV6Address { get; set; } = IPAddress.IPv6Any;
 
     public int ConnectionBacklog { get; set; } = 100;
-
-    public bool NoDelay { get; set; } = true;
+    public bool IsEnabled { get; set; }
 
     /// <summary>
     ///     Gets or sets whether the sockets keep alive feature should be used.
@@ -23,13 +40,16 @@ public abstract class MqttServerTcpEndpointBaseOptions
     /// </summary>
     public bool? KeepAlive { get; set; }
 
+    public LingerOption LingerState { get; set; } = new(true, 0);
+
+    public bool NoDelay { get; set; } = true;
+
+    public int Port { get; set; }
+
     /// <summary>
-    ///     Usually the MQTT packets can be send partially. This is done by using multiple TCP packets
-    ///     or WebSocket frames etc. Unfortunately not all clients do support this feature and
-    ///     will close the connection when receiving such packets. If such clients are connecting to this
-    ///     server the flag must be set to _false_.
+    ///     This requires admin permissions on Linux.
     /// </summary>
-    public bool AllowPacketFragmentation { get; set; } = true;
+    public bool ReuseAddress { get; set; }
 
     /// <summary>
     ///     Gets or sets the TCP keep alive interval.
@@ -48,15 +68,4 @@ public abstract class MqttServerTcpEndpointBaseOptions
     ///     The value _null_ indicates that the OS and framework defaults should be used.
     /// </summary>
     public int? TcpKeepAliveTime { get; set; }
-
-    public LingerOption LingerState { get; set; } = new LingerOption(true, 0);
-
-    public IPAddress BoundInterNetworkAddress { get; set; } = IPAddress.Any;
-
-    public IPAddress BoundInterNetworkV6Address { get; set; } = IPAddress.IPv6Any;
-
-    /// <summary>
-    ///     This requires admin permissions on Linux.
-    /// </summary>
-    public bool ReuseAddress { get; set; }
 }

Source/MQTTnet.Tests/Exceptions/MqttProtocolViolationException_Tests.cs

@@ -0,0 +1,24 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using MQTTnet.Exceptions;
+
+namespace MQTTnet.Tests.Exceptions;
+
+[TestClass]
+public class MqttProtocolViolationException_Tests
+{
+    [TestMethod]
+    public void No_For_Max_Large_Variable_Byte_Integer()
+    {
+        MqttProtocolViolationException.ThrowIfVariableByteIntegerExceedsLimit(MqttProtocolViolationException.VariableByteIntegerMaxValue);
+    }
+
+    [TestMethod]
+    public void Throw_Exception_For_Too_Large_Variable_Byte_Integer()
+    {
+        Assert.ThrowsExactly<MqttProtocolViolationException>(() => MqttProtocolViolationException.ThrowIfVariableByteIntegerExceedsLimit(1726790899));
+    }
+}

Source/MQTTnet/Exceptions/MqttProtocolViolationException.cs

@@ -6,4 +6,15 @@ using System;
 
 namespace MQTTnet.Exceptions;
 
-public class MqttProtocolViolationException(string message) : Exception(message);
+public class MqttProtocolViolationException(string message) : Exception(message)
+{
+    public const uint VariableByteIntegerMaxValue = 268435455;
+
+    public static void ThrowIfVariableByteIntegerExceedsLimit(uint value)
+    {
+        if (value > VariableByteIntegerMaxValue)
+        {
+            throw new MqttProtocolViolationException($"The value {value} is too large for a variable byte integer ({VariableByteIntegerMaxValue}).");
+        }
+    }
+}

Source/MQTTnet/Formatter/MqttBufferWriter.cs

@@ -19,7 +19,6 @@ namespace MQTTnet.Formatter;
 /// </summary>
 public sealed class MqttBufferWriter
 {
-    const uint VariableByteIntegerMaxValue = 268435455;
     const int EncodedStringMaxLength = 65535;
 
     readonly int _maxBufferSize;
@@ -223,10 +222,7 @@ public sealed class MqttBufferWriter
             return;
         }
 
-        if (value > VariableByteIntegerMaxValue)
-        {
-            throw new MqttProtocolViolationException($"The specified value ({value}) is too large for a variable byte integer.");
-        }
+        MqttProtocolViolationException.ThrowIfVariableByteIntegerExceedsLimit(value);
 
         var size = 0;
         var x = value;

Source/MQTTnet/MqttApplicationMessageBuilder.cs

@@ -21,7 +21,6 @@ public sealed class MqttApplicationMessageBuilder
     string _contentType;
     byte[] _correlationData;
     uint _messageExpiryInterval;
-
     MqttPayloadFormatIndicator _payloadFormatIndicator;
     ReadOnlySequence<byte> _payload;
     MqttQualityOfServiceLevel _qualityOfServiceLevel = MqttQualityOfServiceLevel.AtMostOnce;
@@ -84,6 +83,8 @@ public sealed class MqttApplicationMessageBuilder
     /// </summary>
     public MqttApplicationMessageBuilder WithMessageExpiryInterval(uint messageExpiryInterval)
     {
+        // No validation required because this is a 4 byte integer!
+
         _messageExpiryInterval = messageExpiryInterval;
         return this;
     }
@@ -228,9 +229,11 @@ public sealed class MqttApplicationMessageBuilder
     /// </summary>
     public MqttApplicationMessageBuilder WithSubscriptionIdentifier(uint subscriptionIdentifier)
     {
+        MqttProtocolViolationException.ThrowIfVariableByteIntegerExceedsLimit(subscriptionIdentifier);
+
         if (_subscriptionIdentifiers == null)
         {
-            _subscriptionIdentifiers = new List<uint>();
+            _subscriptionIdentifiers = [];
         }
 
         _subscriptionIdentifiers.Add(subscriptionIdentifier);

Source/ReleaseNotes.md

@@ -1,5 +1,6 @@
 * Core: Used new language features across the entire library
 * Core: Performance improvements
+* Core: Added validations for variable byte integers which do not match the .NET uint perfectly
 * Server: Improved performance of retained messages when no event handler is attached (#2093, thanks to @zhaowgit)
 * Server: The event `InterceptingClientEnqueue` is now also called for retained messages (BREAKING CHANGE!)
 * Server: The local end point is now also exposed in the channel adapter (#2179)