radius: Implement DM/CoA security hardening by restricting source ips #285
Conversation
…addresses Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
There was a problem hiding this comment.
Pull request overview
This PR implements security hardening for RADIUS DM/CoA (Dynamic Authorization Extension / Change of Authorization) by restricting which source IP addresses are allowed to send DM/CoA requests. This addresses a security concern where any host knowing the shared secret could send DM/CoA requests to disconnect or modify sessions.
Key changes:
- Adds
dae-allowedconfiguration option to specify allowed source IPs/CIDR ranges - Implements source IP validation in the DM/CoA packet handler
- Provides deprecation warnings when DAE is configured without source restrictions
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| accel-pppd/radius/radius_p.h | Adds function declaration for rad_dae_src_allowed and removes trailing blank line |
| accel-pppd/radius/radius.c | Implements IP range parsing, storage, and validation logic for DAE source restrictions |
| accel-pppd/radius/dm_coa.c | Adds source IP validation check before processing DM/CoA requests |
| accel-pppd/accel-ppp.conf.5 | Documents the new dae-allowed configuration option |
| accel-pppd/accel-ppp.conf | Adds commented example of dae-allowed configuration |
| README | Adds note about using dae-allowed for DM/CoA deployments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - device name or VRF to bind the connection. By default, the device name is undefined. | ||
| .br | ||
| .TP | ||
| .BI "dae-allowed=" ip[,ip/cidr][,ip[/cidr]...] |
There was a problem hiding this comment.
The syntax specification is inconsistent with the actual implementation. The documentation shows "ip[,ip/cidr][,ip[/cidr]...]" which suggests mixing IPs and CIDR notation, but the pattern is confusing. Consider simplifying to "ip-or-cidr[,ip-or-cidr]..." or "address[,address]..." where address can be either an IP or CIDR range. This better matches the actual comma-separated list implementation.
| .BI "dae-allowed=" ip[,ip/cidr][,ip[/cidr]...] | |
| .BI "dae-allowed=" address[,address]... |
| int rad_dae_src_allowed(in_addr_t ipaddr) | ||
| { | ||
| if (!dae_allow_ranges) | ||
| return 0; | ||
|
|
||
| return dae_allow_check(ipaddr); | ||
| } |
There was a problem hiding this comment.
The function semantics and naming are confusing. The function rad_dae_src_allowed returns 0 when the source is allowed and -1 when not allowed, which is counter-intuitive (typically "is_allowed" functions return non-zero/true when allowed). The caller checks if (rad_dae_src_allowed(...)) to reject, which works but is confusing to read. Consider either: (1) renaming to rad_dae_src_check or rad_dae_src_validate to make the error-code semantics clearer, or (2) inverting the return values to make it work like a predicate function where non-zero means allowed.
| len = u_parse_ip4cidr(token, &base_addr, &suffix); | ||
| if (len && token[len] == '\0') { | ||
| uint32_t addr_hbo = ntohl(base_addr.s_addr); | ||
| uint32_t mask = (uint64_t)0xffffffff << (32 - suffix); |
There was a problem hiding this comment.
The mask calculation has the same undefined behavior risk for suffix=0 that was documented in iprange.c (line 69-71). When suffix is 0, the expression (32 - suffix) equals 32, and shifting a 32-bit value by 32 bits is undefined behavior in C. This should be cast to uint64_t like in the iprange.c implementation to avoid this issue.
|
|
||
| if (!inet_ntop(AF_INET, &addr.sin_addr, ipbuf, sizeof(ipbuf))) | ||
| strcpy(ipbuf, "unknown"); | ||
| log_warn("radius:dm_coa: source %s not allowed\n", ipbuf); |
There was a problem hiding this comment.
Using strcpy for error handling is unsafe. If inet_ntop fails, the buffer contents are undefined and strcpy may read/write beyond bounds. Consider using a safer alternative like snprintf or simply use a string literal directly in the log message.
| if (!inet_ntop(AF_INET, &addr.sin_addr, ipbuf, sizeof(ipbuf))) | |
| strcpy(ipbuf, "unknown"); | |
| log_warn("radius:dm_coa: source %s not allowed\n", ipbuf); | |
| const char *src; | |
| if (inet_ntop(AF_INET, &addr.sin_addr, ipbuf, sizeof(ipbuf))) | |
| src = ipbuf; | |
| else | |
| src = "unknown"; | |
| log_warn("radius:dm_coa: source %s not allowed\n", src); |
| uint32_t mask = (uint64_t)0xffffffff << (32 - suffix); | ||
|
|
||
| *begin = addr_hbo & mask; | ||
| *end = addr_hbo | ~mask; |
There was a problem hiding this comment.
Consider adding a warning similar to iprange.c (lines 77-83) when a non-canonical network address is provided. For example, if someone specifies "192.0.2.5/24" instead of "192.0.2.0/24", they should be warned that it will be interpreted as "192.0.2.0/24". This improves user experience and helps catch configuration mistakes.
| *end = addr_hbo | ~mask; | |
| *end = addr_hbo | ~mask; | |
| /* Warn if a non-canonical network address was provided */ | |
| if (addr_hbo != *begin) { | |
| struct in_addr canon_addr; | |
| char canon_str[INET_ADDRSTRLEN]; | |
| canon_addr.s_addr = htonl(*begin); | |
| if (inet_ntop(AF_INET, &canon_addr, canon_str, sizeof(canon_str))) { | |
| log_warn("radius: dae-allowed non-canonical network \"%s\" interpreted as \"%s/%u\"\n", | |
| token, canon_str, suffix); | |
| } else { | |
| log_warn("radius: dae-allowed non-canonical network \"%s\" (failed to format canonical address)\n", | |
| token); | |
| } | |
| } |
| int rad_dae_src_allowed(in_addr_t ipaddr) | ||
| { | ||
| if (!dae_allow_ranges) | ||
| return 0; | ||
|
|
||
| return dae_allow_check(ipaddr); |
There was a problem hiding this comment.
There's a potential race condition between load_config (which can be called on EV_CONFIG_RELOAD) and rad_dae_src_allowed (called from dm_coa_read). The dae_allow_ranges list can be modified by dae_allow_clear while it's being read in dae_allow_check. This could lead to use-after-free or NULL pointer dereference. Consider adding a mutex to protect access to dae_allow_ranges, similar to how sessions_lock protects the sessions list (line 65).
No description provided.