Enable SO_REUSEADDR by default in IP_SOCKET_OPTIONS - #169
Enable SO_REUSEADDR by default in IP_SOCKET_OPTIONS#169devin-ai-integration[bot] wants to merge 2 commits into
Conversation
| // Projects should evaluate their threat model and choose options accordingly. | ||
| static const IpSocketOptions IP_SOCKET_OPTIONS[] = { | ||
| makeIntOption(SO_REUSEADDR, SOL_SOCKET, 0), // Example | ||
| makeIntOption(SO_REUSEADDR, SOL_SOCKET, SOCKET_REUSE_ADDRESS), |
There was a problem hiding this comment.
🚩 SO_REUSEADDR enabled by default despite security warning in adjacent comments
The comment block at lines 84-88 explicitly warns: "if enabling SO_REUSEADDR and there is a hostile actor on the same machine, they could potentially bind to the same port and intercept messages. Projects should evaluate their threat model and choose options accordingly." The PR now enables this option by default for all socket types (TCP client at Drv/Ip/TcpClientSocket.cpp:72, TCP server at Drv/Ip/TcpServerSocket.cpp:69, and UDP at Drv/Ip/UdpSocket.cpp:159). While SO_REUSEADDR is very common for server sockets (to avoid bind failures after process restart), applying it uniformly to client sockets and UDP sockets may be unnecessary and slightly expands the attack surface. This is a design choice rather than a bug, but the contradiction between the warning comment and the new default value is worth acknowledging.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional — the previous 0 value was marked as an example placeholder. Enabling by default avoids EADDRINUSE on server restart, which is the primary use case. The existing security warning at lines 86-89 already documents the tradeoff for projects with stricter threat models.
| static const IpSocketOptions IP_SOCKET_OPTIONS[] = { | ||
| makeIntOption(SO_REUSEADDR, SOL_SOCKET, 0), // Example | ||
| makeIntOption(SO_REUSEADDR, SOL_SOCKET, SOCKET_REUSE_ADDRESS), | ||
| // Add other socket options as needed, and expand above helper functions | ||
| // if other types are needed | ||
| }; |
There was a problem hiding this comment.
🚩 No mechanism to apply SO_REUSEADDR selectively per socket type
The IP_SOCKET_OPTIONS[] array at line 90-94 is a single global list applied uniformly to all socket types via IpSocket::setupSocketOptions() (Drv/Ip/IpSocket.cpp:229-245). There is no way to enable SO_REUSEADDR on TCP server sockets only (where it's most commonly needed to avoid TIME_WAIT bind failures) while leaving it disabled on TCP client and UDP sockets. If selective application is desired, the architecture would need per-socket-type option arrays or a filtering mechanism. This is a pre-existing design limitation but becomes more relevant now that the option is enabled by default.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — the existing IP_SOCKET_OPTIONS[] architecture is a single global list with no per-socket-type filtering. Adding per-type option arrays would be a larger refactor. For now this uses the existing mechanism; projects needing selective behavior can override at the config level.
Set the SO_REUSEADDR socket option default from 0 (disabled) to 1 (enabled) in IpCfg.hpp. This prevents TIME_WAIT delays when rerunning flight software that uses TcpServer. Projects can set the value back to 0 to disable. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
| // Default the TcpServer to reuse the same port, which prevents TIME_WAIT delays when rerunning | ||
| // flight software. Set to zero to turn off. | ||
| makeIntOption(SO_REUSEADDR, SOL_SOCKET, 1), |
There was a problem hiding this comment.
🔴 SO_REUSEADDR=1 applied to TcpClient and UDP sockets, not just TcpServer as intended
The comment says "Default the TcpServer to reuse the same port", but IP_SOCKET_OPTIONS is a global array applied by IpSocket::setupSocketOptions() to all socket types. It is called from TcpServerSocket::startup() (Drv/Ip/TcpServerSocket.cpp:69), TcpClientSocket::openProtocol() (Drv/Ip/TcpClientSocket.cpp:72), and UdpSocket::openProtocol() (Drv/Ip/UdpSocket.cpp:159). Enabling SO_REUSEADDR on TCP clients and UDP sockets is unintended per the comment, and the security warning in the same file (lines 84–88) explicitly warns that SO_REUSEADDR allows a hostile actor on the same machine to bind to the same port and intercept messages. For UDP on Linux in particular, SO_REUSEADDR allows multiple sockets to bind to the same address:port, directly enabling this attack vector.
Prompt for agents
The IP_SOCKET_OPTIONS array in default/config/IpCfg.hpp is applied globally to all socket types (TcpServer, TcpClient, UDP) via IpSocket::setupSocketOptions() in Drv/Ip/IpSocket.cpp:229-244. The comment states the intent is only for TcpServer, but the mechanism does not distinguish socket types.
To fix this properly, either:
1. Move the SO_REUSEADDR option out of the global IP_SOCKET_OPTIONS array and apply it only in TcpServerSocket::startup() (Drv/Ip/TcpServerSocket.cpp around line 69), or
2. Add a socket-type parameter to setupSocketOptions so options can be filtered by socket type, or
3. If SO_REUSEADDR is truly intended for all socket types, update the comment to accurately reflect this and document the security implications for UDP and TcpClient.
The call sites are: TcpServerSocket::startup() at Drv/Ip/TcpServerSocket.cpp:69, TcpClientSocket::openProtocol() at Drv/Ip/TcpClientSocket.cpp:72, and UdpSocket::openProtocol() at Drv/Ip/UdpSocket.cpp:159.
Was this helpful? React with 👍 or 👎 to provide feedback.
b60266b to
f6462b5
Compare
Add a 'Configuration: Socket Options' section explaining the SO_REUSEADDR default, how to disable it, and the security note. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
Coverage report — base
|
| Module | Line | Function | Branch |
|---|---|---|---|
CFDP/Checksum |
71.15 | 57.14 | 53.85 |
Drv/AsyncByteStreamBufferAdapter |
100.00 | 100.00 | 100.00 |
Drv/ByteStreamBufferAdapter |
100.00 | 100.00 | 100.00 |
Drv/Ip |
44.32 | 57.38 | 25.36 |
Drv/TcpClient |
75.00 | 100.00 | 47.37 |
Drv/TcpServer |
84.72 | 100.00 | 60.00 |
Drv/Udp |
68.09 | 90.91 | 42.86 |
Fw/Buffer |
81.25 | 89.47 | 58.62 |
Fw/DataStructures |
98.12 | 97.14 | 82.66 |
Fw/Dp |
94.83 | 96.67 | 95.95 |
Fw/FilePacket |
75.24 | 89.06 | 54.30 |
Fw/Log |
71.43 | 72.41 | 60.00 |
Fw/Logger |
100.00 | 100.00 | 100.00 |
Fw/SerializableFile |
90.00 | 100.00 | 79.41 |
Fw/Time |
88.62 | 85.48 | 86.84 |
Fw/Tlm |
53.76 | 60.00 | 37.50 |
Fw/Types |
54.75 | 57.62 | 34.96 |
Os |
18.07 | 19.52 | 14.52 |
Os/Generic |
89.10 | 96.30 | 67.18 |
Os/Generic/Types |
92.45 | 91.67 | 82.14 |
Os/Posix |
62.27 | 84.21 | 44.10 |
Svc/ActiveRateGroup |
100.00 | 100.00 | 92.31 |
Svc/ActiveTextLogger |
79.05 | 90.00 | 71.23 |
Svc/AssertFatalAdapter |
94.74 | 100.00 | 86.67 |
Svc/BufferAccumulator |
88.00 | 94.12 | 74.49 |
Svc/BufferLogger |
92.62 | 86.96 | 80.46 |
Svc/BufferManager |
99.05 | 100.00 | 83.64 |
Svc/BufferRepeater |
91.67 | 100.00 | 75.00 |
Svc/ChronoTime |
100.00 | 100.00 | 100.00 |
Svc/CmdDispatcher |
96.97 | 91.67 | 91.75 |
Svc/CmdSequencer |
93.89 | 97.12 | 84.63 |
Svc/CmdSplitter |
100.00 | 100.00 | 100.00 |
Svc/ComLogger |
97.37 | 91.67 | 83.91 |
Svc/ComSplitter |
100.00 | 100.00 | 100.00 |
Svc/ComStub |
98.51 | 100.00 | 85.19 |
Svc/DpCatalog |
78.00 | 100.00 | 66.27 |
Svc/DpManager |
97.44 | 100.00 | 100.00 |
Svc/DpWriter |
97.58 | 90.00 | 97.22 |
Svc/FileDownlink |
84.15 | 90.91 | 71.90 |
Svc/FileManager |
88.41 | 93.33 | 82.44 |
Svc/FileUplink |
92.35 | 96.55 | 80.95 |
Svc/FileWorker |
90.29 | 100.00 | 84.87 |
Svc/FprimeDeframer |
100.00 | 100.00 | 97.92 |
Svc/FprimeFramer |
100.00 | 100.00 | 95.45 |
Svc/FprimeRouter |
88.89 | 100.00 | 78.26 |
Svc/FpySequencer |
86.76 | 99.04 | 76.89 |
Svc/GenericHub |
100.00 | 100.00 | 85.64 |
Svc/Health |
100.00 | 100.00 | 88.66 |
Svc/LinuxTimer |
97.06 | 100.00 | 82.35 |
Svc/OsTime |
70.00 | 83.33 | 60.98 |
Svc/PassiveRateGroup |
100.00 | 100.00 | 89.47 |
Svc/PolyDb |
100.00 | 100.00 | 53.57 |
Svc/PosixTime |
100.00 | 100.00 | 75.00 |
Svc/PrmDb |
92.75 | 89.47 | 88.24 |
Svc/RateGroupDriver |
100.00 | 100.00 | 68.75 |
Svc/SeqDispatcher |
73.08 | 80.00 | 71.05 |
Svc/StaticMemory |
100.00 | 100.00 | 100.00 |
Svc/SystemResources |
98.63 | 100.00 | 76.19 |
Svc/TlmChan |
77.59 | 85.71 | 63.03 |
Svc/TlmPacketizer |
92.49 | 100.00 | 77.45 |
Svc/Version |
96.10 | 100.00 | 86.96 |
Utils |
20.04 | 26.44 | 24.07 |
Utils/Types |
92.11 | 95.83 | 77.08 |
Modules without UTs
CFDP/Checksum/GTest, Drv/ByteStreamDriverModel, Drv/Interfaces, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports, Drv/Ports/DataTypes, FppTestProject/FppTest/interfaces, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/ports, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Cmd, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Interfaces, Fw/Obj, Fw/Port, Fw/Ports/CompletionStatus, Fw/Ports/Ready, Fw/Ports/Signal, Fw/Ports/SuccessCondition, Fw/Prm, Fw/SerializableFile/test/TestSerializable, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Cycle, Svc/DpPorts, Svc/Fatal, Svc/FatalHandler, Svc/FileDownlinkPorts, Svc/FprimeProtocol, Svc/Interfaces, Svc/PassiveConsoleTextLogger, Svc/Ping, Svc/PolyIf, Svc/Ports/CommsPorts, Svc/Ports/FilePorts, Svc/Ports/OsTimeEpoch, Svc/Ports/TlmPacketizerPorts, Svc/Ports/VersionPorts, Svc/Sched, Svc/Seq, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestChainedAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestTargetAutocoder, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
Change Description
Flip the
SO_REUSEADDRdefault from0(disabled) to1(enabled) in theIP_SOCKET_OPTIONSarray:static const IpSocketOptions IP_SOCKET_OPTIONS[] = { - makeIntOption(SO_REUSEADDR, SOL_SOCKET, 0), // Example + makeIntOption(SO_REUSEADDR, SOL_SOCKET, 1), };Rationale
TCP servers that restart quickly hit
EADDRINUSEbecause the kernel holds the socket inTIME_WAIT. EnablingSO_REUSEADDReliminates this. Projects with stricter threat models can set the value back to0in their ownIpCfg.hpp.Testing/Review Recommendations
setupSocketOptions(); only the value changed).TcpServerSocket::startup()→setupSocketOptions(serverFd)→ iteratesIP_SOCKET_OPTIONS→setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &1, ...).Future Work
None.
AI Usage (see policy)
AI was used for code generation and PR authoring.
IAMAI
Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/91e0e7ed4e114228bd0c1f9335f80023