Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ntcore] Clarification and Safeguards for pub/subuid Constraints in NetworkTables #7679

Open
cjlawson02 opened this issue Jan 14, 2025 · 4 comments · May be fixed by #7726
Open

[ntcore] Clarification and Safeguards for pub/subuid Constraints in NetworkTables #7679

cjlawson02 opened this issue Jan 14, 2025 · 4 comments · May be fixed by #7726
Assignees
Labels
type: bug Something isn't working.

Comments

@cjlawson02
Copy link

cjlawson02 commented Jan 14, 2025

Describe the bug
Hi team, the pubuid and subuid for NetworkTables publishers are constrained to fit within a signed 32-bit integer space (source). However, this constraint is not explicitly documented in the relevant sections of the networktables4.adoc. Exceeding this range can cause unexpected behavior, including crashes.

To Reproduce
Steps to reproduce the behavior:

  1. Produce a pubuid exceeding the signed 32-bit positive integer range.
  2. Send a publish request to the server, such as:
    [{"method":"publish","params":{"type":"string","name":"/MyTable/myvalue","pubuid":2147483647,"properties":{}}}]

Expected behavior
The pubuid should be validated and rejected if it exceeds the supported range.

Desktop (please complete the following information):

  • OS: macOS
  • Project Information: Getting Started example simulator on Java 17, WPILib 2025.2.1

Additional context
This issue became evident when testing with values near and beyond the signed 32-bit limit:

  • A pubuid of 2147483646 worked as expected.

  • A pubuid of 2147483647 caused a crash, however not all integers above the max cause the code to crash.

  • the UID's are defined as int here: Message.h

Potential Security Concern

A malicious actor with websocket access could exploit this limitation to trigger denial-of-service scenarios by sending invalid publish requests, such as:

[{"method":"publish","params":{"type":"string","name":"/MyTable/myvalue","pubuid":2147483647,"properties":{}}}]

Suggested Solutions

  1. Change pubuid and subuid to unsigned integers with server-side validation.
  2. Increase their size to unsigned 64-bit integers.
  3. Allow pubuid and subuid to support strings for greater flexibility.

Additionally, the documentation should be updated to clearly specify the current constraint, preventing similar issues for developers and mitigating security risks.

@cjlawson02 cjlawson02 added the type: bug Something isn't working. label Jan 14, 2025
@cjlawson02 cjlawson02 changed the title [NTCore] Clarification and Safeguards for pub/subuid Constraints in NetworkTables [NTCore] Clarification and Safeguards for pub/subuid Constraints in NetworkTables Jan 14, 2025
@cjlawson02 cjlawson02 changed the title [NTCore] Clarification and Safeguards for pub/subuid Constraints in NetworkTables [ntcore] Clarification and Safeguards for pub/subuid Constraints in NetworkTables Jan 14, 2025
@PeterJohnson
Copy link
Member

PeterJohnson commented Jan 14, 2025

I will document and add implementation validation of the current signed 32-bit values (at a minimum, fix the crash). At some point (e.g. for 2027, when the platform goes to 64-bit), I may add support for signed 64-bit values. I do not plan on changing the value type to strings or unsigned integers. Security is not a concern in our environment, but we do like to avoid crashes.

@PeterJohnson PeterJohnson self-assigned this Jan 14, 2025
@PeterJohnson
Copy link
Member

Investigating further, the 32-bit range won't support the full 32-bit range, because (as you discovered) some of these values are used for sentinel values in the internal implementation. So the spec will likely say something like the server "should" support signed 32-bit values, but may be constrained to smaller ranges based on implementation. It will also recommend that clients use as small of values as possible for pubuid's, because those IDs are used in the MessagePack messages from the client to server (so larger values mean they take more space and time to encode/decode for every message).

@cjlawson02
Copy link
Author

Does the client need to worry about a collision with another pubuid, or could it just increment from 0?

@PeterJohnson
Copy link
Member

pubuid's are client-unique; there is no possibility of colliding with another client's pubuids. So it's perfectly fine to just increment from 0.

@PeterJohnson PeterJohnson linked a pull request Jan 24, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants