From 676f2f84d74e5b6d3d55345ff880cea8734b492c Mon Sep 17 00:00:00 2001
From: Peter Johnson <johnson.peter@gmail.com>
Date: Wed, 18 Jun 2025 22:03:24 -0600
Subject: [PATCH] [ntcore] Check id ranges in control messages (#7726)

---
 ntcore/doc/networktables4.adoc                |  5 +++
 .../src/main/native/cpp/net/WireDecoder.cpp   | 43 +++++++++++++++++++
 .../test/native/cpp/server/ServerImplTest.cpp | 16 +++++++
 3 files changed, 64 insertions(+)

diff --git a/ntcore/doc/networktables4.adoc b/ntcore/doc/networktables4.adoc
index 318afac7f8..e41bbdc3e7 100644
--- a/ntcore/doc/networktables4.adoc
+++ b/ntcore/doc/networktables4.adoc
@@ -163,6 +163,11 @@ Clients may publish a value at any time following clock synchronization.  Client
 
 Clients should subscribe to the `$sub$<topic>` meta topic for each topic published and use this metadata to determine how frequently to send updates to the network. However, this is not required--clients may choose to ignore this and send updates at any time.
 
+[[ids]]
+=== IDs and UIDs
+
+IDs and UIDs should be selected by clients and servers to be as small as possible for transmission efficiency.  Clients and servers should support a signed 32-bit range for IDs and UIDs, but exceptions can be made for implementation reasons.  Clients and servers must ignore messages with unsupported IDs and UIDs, and should report a diagnostic if this occurs.
+
 [[meta-topics]]
 === Server-Published Meta Topics
 
diff --git a/ntcore/src/main/native/cpp/net/WireDecoder.cpp b/ntcore/src/main/native/cpp/net/WireDecoder.cpp
index 14a051d479..e588229aa6 100644
--- a/ntcore/src/main/native/cpp/net/WireDecoder.cpp
+++ b/ntcore/src/main/native/cpp/net/WireDecoder.cpp
@@ -174,6 +174,12 @@ static bool WireDecodeTextImpl(std::string_view in, T& out,
             goto err;
           }
 
+          // limit to 32-bit range and exclude endpoints used by DenseMap
+          if (pubuid >= 0x7fffffffLL || pubuid <= (-0x7fffffffLL - 1)) {
+            error = "pubuid out of range";
+            goto err;
+          }
+
           // properties; allow missing (treated as empty)
           wpi::json* properties = nullptr;
           auto propertiesIt = params->find("properties");
@@ -200,6 +206,12 @@ static bool WireDecodeTextImpl(std::string_view in, T& out,
             goto err;
           }
 
+          // limit to 32-bit range and exclude endpoints used by DenseMap
+          if (pubuid >= 0x7fffffffLL || pubuid <= (-0x7fffffffLL - 1)) {
+            error = "pubuid out of range";
+            goto err;
+          }
+
           // complete
           out.ClientUnpublish(pubuid);
           rv = true;
@@ -231,6 +243,12 @@ static bool WireDecodeTextImpl(std::string_view in, T& out,
             goto err;
           }
 
+          // limit to 32-bit range and exclude endpoints used by DenseMap
+          if (subuid >= 0x7fffffffLL || subuid <= (-0x7fffffffLL - 1)) {
+            error = "subuid out of range";
+            goto err;
+          }
+
           // options
           PubSubOptionsImpl options;
           auto optionsIt = params->find("options");
@@ -303,6 +321,12 @@ static bool WireDecodeTextImpl(std::string_view in, T& out,
             goto err;
           }
 
+          // limit to 32-bit range and exclude endpoints used by DenseMap
+          if (subuid >= 0x7fffffffLL || subuid <= (-0x7fffffffLL - 1)) {
+            error = "pubuid out of range";
+            goto err;
+          }
+
           // complete
           out.ClientUnsubscribe(subuid);
           rv = true;
@@ -324,6 +348,12 @@ static bool WireDecodeTextImpl(std::string_view in, T& out,
             goto err;
           }
 
+          // limit to 32-bit range and exclude endpoints used by DenseMap
+          if (id >= 0x7fffffffLL || id <= (-0x7fffffffLL - 1)) {
+            error = "id out of range";
+            goto err;
+          }
+
           // type
           auto typeStr = ObjGetString(*params, "type", &error);
           if (!typeStr) {
@@ -339,6 +369,13 @@ static bool WireDecodeTextImpl(std::string_view in, T& out,
               error = "pubuid value must be a number";
               goto err;
             }
+
+            // limit to 32-bit range and exclude endpoints used by DenseMap
+            if (val >= 0x7fffffffLL || val <= (-0x7fffffffLL - 1)) {
+              error = "pubuid out of range";
+              goto err;
+            }
+
             pubuid = val;
           }
 
@@ -369,6 +406,12 @@ static bool WireDecodeTextImpl(std::string_view in, T& out,
             goto err;
           }
 
+          // limit to 32-bit range and exclude endpoints used by DenseMap
+          if (id >= 0x7fffffffLL || id <= (-0x7fffffffLL - 1)) {
+            error = "id out of range";
+            goto err;
+          }
+
           // complete
           out.ServerUnannounce(*name, id);
         } else if (*method == PropertiesUpdateMsg::kMethodStr) {
diff --git a/ntcore/src/test/native/cpp/server/ServerImplTest.cpp b/ntcore/src/test/native/cpp/server/ServerImplTest.cpp
index 0db7f15d5b..2821e7c8f5 100644
--- a/ntcore/src/test/native/cpp/server/ServerImplTest.cpp
+++ b/ntcore/src/test/native/cpp/server/ServerImplTest.cpp
@@ -423,4 +423,20 @@ TEST_F(ServerImplTest, ZeroTimestampNegativeTime) {
   }
 }
 
+TEST_F(ServerImplTest, InvalidPubUid) {
+  EXPECT_CALL(logger, Call(_, _, _, "0: pubuid out of range"));
+  server.SetLocal(&local, &queue);
+
+  // connect client
+  ::testing::StrictMock<net::MockWireConnection> wire;
+  MockSetPeriodicFunc setPeriodic;
+  auto [name, id] = server.AddClient("test", "connInfo", false, wire,
+                                     setPeriodic.AsStdFunction());
+
+  server.ProcessIncomingText(
+      id,
+      "[{\"method\":\"publish\",\"params\":{\"type\":\"string\",\"name\":"
+      "\"myvalue\",\"pubuid\":2147483647,\"properties\":{}}}]");
+}
+
 }  // namespace nt