diff options
| -rw-r--r-- | docs/ng40-safeguards-review.md | 135 | ||||
| -rw-r--r-- | tests/unit/dhcpv6_safeguards_test.c | 154 | ||||
| -rw-r--r-- | tests/unit/lcp_safeguards_test.c | 40 | ||||
| -rw-r--r-- | tests/unit/mempool_safeguards_test.c | 38 | ||||
| -rw-r--r-- | tests/unit/radius_safeguards_test.c | 119 | ||||
| -rw-r--r-- | tests/unit/run_safeguards.sh | 21 | ||||
| -rw-r--r-- | tests/unit/vrf_safeguards_test.c | 72 |
7 files changed, 579 insertions, 0 deletions
diff --git a/docs/ng40-safeguards-review.md b/docs/ng40-safeguards-review.md new file mode 100644 index 00000000..592869c6 --- /dev/null +++ b/docs/ng40-safeguards-review.md @@ -0,0 +1,135 @@ +# Additional safeguards from accel-ppp-ng PR #40 + +Compared against freshly fetched accel-ppp `upstream/master` at +`c414f0a7` (2026-09-07). Reference: +[accel-ppp-ng PR #40](https://github.com/accel-ppp/accel-ppp-ng/pull/40/commits), +base `c7b67eb010a99ceefe13d2f74fc672cb91ce0af9`, head +`3843773f15af477c39a326720ac5f3e713b373fb`. + +The reference contains 20 commits. Several reported vulnerabilities already +have independent upstream fixes by Denys Fedoryshchenko. This branch adds the +remaining safeguards while retaining upstream's existing parsing and policy. + +## What was missing + +- PPPoE rejected missing Service-Name tags but accepted duplicates. A matching + earlier tag could validate a different final selection. Require exactly one + tag. Preserve silent rejection before cookie validation; no error response + is sent for an unauthenticated request. +- DHCPv6 accepted duplicate/nonempty Rapid-Commit options and emitted one + response option per occurrence. Reject malformed requests and emit at most + one Rapid-Commit. AFTR printing was already bounded, but parsing did not + validate the uncompressed, terminated DNS name or reject duplicate AFTR + options. Add those checks and an eight-layer relay nesting limit. +- DHCPv6 reply bounds were measured from a moving inner-message pointer. + Bound relay headers and both option allocators by the actual 4096-byte + allocation, including the inner header. Propagate allocation failures + through both reply builders, status insertion, and ORO expansion; discard + an incomplete reply. Encode Relay-Message length in network byte order, + excluding its option header, so valid relayed replies remain usable. +- RADIUS accepted replies based only on the identifier. Verify the Response + Authenticator using the request's wire authenticator and secret snapshot + before updating server health, releasing the request slot, or invoking its + callback. Use constant-time comparison. This is independent of the existing + outbound Message-Authenticator/`blast-protection` setting. +- Accounting signing did not preserve its computed Request Authenticator. + Centralize signing after server selection and delay updates, zero the + authenticator before each hash, and retain the result and signing secret. + This covers Start/Interim/Stop and Accounting-On/Off, including retransmits + with `acct-delay-time` disabled. Snapshot Access-Request secrets too; use + that snapshot for PAP encryption and MPPE decryption, refresh it on server + changes, and zero Message-Authenticator before each retransmit calculation. +- Accel-VRF-Name could reach an unbounded interface-name copy. Reject oversized + and embedded-NUL Access-Accept values and handle allocation failure. Enforce + the same boundary at interface lookup and the length-delimited session API. + Preserve the CoA attribute length instead of silently accepting a prefix + before NUL. Only the literal CoA value `0` removes the VRF; names such as + `0blue` are now treated as interface names. Empty names and `(NULL, 0)` still + select the default VRF. +- LCP already bounded received packets and mandatory fields, but an Echo-Request + larger than the peer's MTU produced a truncated response with the old length + field. Discard it before modifying the packet or echo state. +- Normal pooled allocations retained their former contents after free. Clear + the payload before returning it to the pool as defense in depth. This adds a + write proportional to object size; it does not replace protocol bounds checks. + The separate MEMDEBUG allocator is unchanged. + +## Reference commit coverage + +Reference hashes link to the original authors' commits. “Present” identifies +upstream code already providing the relevant safeguard, not necessarily an +identical patch. + +| Reference commit | Topic | Upstream baseline and this branch | +| --- | --- | --- | +| [ef7323fd](https://github.com/accel-ppp/accel-ppp-ng/commit/ef7323fd6dfe5655a46581733c86f4abcbfee548) | T8464 Service-Name | Missing-name rejection present (`f8dfbeca`, `4deb615d`); add duplicate rejection. | +| [371e7f0a](https://github.com/accel-ppp/accel-ppp-ng/commit/371e7f0a3e21d3040941330858274e104f1dfdcd) | T8473 Rapid-Commit | Add parser checks and single response option. | +| [10a3e72d](https://github.com/accel-ppp/accel-ppp-ng/commit/10a3e72d523c7fe910a23febe979a3de9cd55d8f) | T8474 AFTR | Printer bounds present (`3004db8b`); add parser semantics and duplicate checks. | +| [bd1806e7](https://github.com/accel-ppp/accel-ppp-ng/commit/bd1806e74baff0ef9b6a24a1a73741f8b7ca5125) | T8475 relay layers | Inner bounds/progress present (`b0e7444c`, `a9bfcdb8`, `94993558`); add depth and physical reply bounds, including caller failure handling. | +| [248e7caa](https://github.com/accel-ppp/accel-ppp-ng/commit/248e7caa2cf5c2e1fcaad60abedd7ccfc6365d3b) | T8526 response authentication | Add verification with upstream's OpenSSL API and reload-safe secret ownership. | +| [7ce6244f](https://github.com/accel-ppp/accel-ppp-ng/commit/7ce6244f044f10650adadfcb34e8b6f095a8967b) | T8543 Framed-Route | Present (`a4c79494`, `027bff94`); retain configurable strictness. | +| [be2f54db](https://github.com/accel-ppp/accel-ppp-ng/commit/be2f54dbf8272d6bdd3da1c98c7d9838b95dc27b) | T8545 accounting RA | Add shared zero/hash/save signing. | +| [24afb859](https://github.com/accel-ppp/accel-ppp-ng/commit/24afb859e0a28a509d930a6cf9bea1d8f5baf732) | T8549 status printing | Present (`6019e1d1`, `52a01730`). | +| [2fa64759](https://github.com/accel-ppp/accel-ppp-ng/commit/2fa647594e9d2c64e2f75e3e1537a748e5568d73) | T8550 SSTP lengths | Present (`1719b4ab`). | +| [55a804af](https://github.com/accel-ppp/accel-ppp-ng/commit/55a804afaaddd671fbb58290977a59c03369ab5e) | T8611 VRF | Add Access-Accept, CoA, session API, and shared lookup safeguards. | +| [c93b3675](https://github.com/accel-ppp/accel-ppp-ng/commit/c93b36755c285ef05400fa7be34f7560f02fe2e3) | T8825 tag copy | Present (`c32518d7`); no duplicate port. | +| [1a42ab40](https://github.com/accel-ppp/accel-ppp-ng/commit/1a42ab408da86acde9292120835bd76eb82e8809) | T8830 PPP lengths/pool clear | Packet bounds present (`028b942d`, `d22666a8`); add pool payload clearing. | +| [1957e7d1](https://github.com/accel-ppp/accel-ppp-ng/commit/1957e7d1c131b65768f9fc4b3881e5fdb544f159) | T8830 LCP fields/Echo | Mandatory-field access checks present (`028b942d`); add Echo MTU rejection. | +| [64311652](https://github.com/accel-ppp/accel-ppp-ng/commit/64311652003f719a16cecbb1d5be71af02f57370) | T8545 Accounting-On/Off | Use shared signing; callback UAF already fixed by `7bd6843c`. | +| [01baa010](https://github.com/accel-ppp/accel-ppp-ng/commit/01baa010a45b01ff067405646da89411fd2cc02a) | T8464 cookie ordering | Upstream silently rejects malformed Service-Name; preserve that safe behavior. | +| [8305387d](https://github.com/accel-ppp/accel-ppp-ng/commit/8305387d80c7a5009988106702b23c6eeca8f57a) | T8825 tag fixup | Existing capacity check protects the write; no duplicate port. | +| [f52607d4](https://github.com/accel-ppp/accel-ppp-ng/commit/f52607d4ae216e7d84d7bdacd8581a136248b3e2) | T8545 EVP cleanup | Upstream uses stack MD5 contexts; no EVP allocation to free here. | +| [177238b1](https://github.com/accel-ppp/accel-ppp-ng/commit/177238b1799456510d56637eaa210d62eedef353) | T8611 allocation failure | Include VRF allocation failure handling. | +| [2f18c76d](https://github.com/accel-ppp/accel-ppp-ng/commit/2f18c76d92eef9d09805146847ee690234371963) | T8550 cast fixup | SSTP safeguard already present. | +| [3843773f](https://github.com/accel-ppp/accel-ppp-ng/commit/3843773f15af477c39a326720ac5f3e713b373fb) | T8474 log indentation | New parser log follows upstream style; no standalone cosmetic port. | + +## Credits + +Original safeguard patches: **Ritika Chopra** `<r.chopra@vyos.io>`. +The tag fixup `8305387d` is authored as **RC** `<ritika0313@gmail.com>` and +carries the original trailer +`Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>`. +That fixup is already covered upstream and was not copied into this branch. +Adaptations retain Ritika's credit in their commit messages. Existing upstream +fixes and this integration are by **Denys Fedoryshchenko**. + +## Validation + +Run from the repository root: + +```sh +cmake -S . -B /tmp/accel-ppp-ng40-build -DCMAKE_BUILD_TYPE=Debug \ + -DBUILD_IPOE_DRIVER=FALSE -DBUILD_VLAN_MON_DRIVER=FALSE +cmake --build /tmp/accel-ppp-ng40-build -j 8 +sh tests/unit/run_safeguards.sh /tmp/accel-ppp-ng40-build +git diff --check +``` + +Build, the existing IPv6 DNS selection test, and five standalone suites passed. Tests compile production code with +small external-service stubs. DHCPv6, RADIUS, VRF, and pool tests use ASan, +UBSan, and LeakSanitizer; the LCP test uses UBSan (ASan's global registration +otherwise retains the entire daemon's layer table). No sanitizer failures. +The VRF harness emits an unused-path compiler warning about the uninitialized +namespace configuration in `alloc_net`; that path is not executed. + +Covered: duplicate/nonempty Rapid-Commit, valid single Rapid-Commit replies, +malformed/duplicate AFTR, direct and eight-layer replies, excessive nesting, +exact buffer fill and overflow rejection, negative allocation lengths, ORO +expansion exhaustion in both reply builders, response digests for Accept/ +Reject/Challenge/Accounting, forged reply rejection before callbacks/health, +valid reply delivery, Message-Authenticator retransmission, accounting digest +recomputation and secret changes, +VRF boundaries/embedded NUL/CoA/default removal, Echo MTU boundaries, and pool +payload clearing on reuse. + +Baseline reproductions against `c414f0a7` failed as expected: the duplicate +Rapid-Commit rejection assertion and the forged Access-Accept rejection +assertion. The corresponding patched checks pass. + +Limits: these are focused in-process tests, not a complete deployed NAS or +network-namespace session suite. PPPoE duplicate rejection was reviewed against +its real parser and callers but was not exercised over an Ethernet interface. +Live RADIUS server failover, full PAP/MPPE interoperability, and pool-clearing +throughput were not measured. In particular, upstream already sends PAP's +original encrypted password when changing to a server with a different secret; +this branch does not redesign that pre-existing failover behavior. diff --git a/tests/unit/dhcpv6_safeguards_test.c b/tests/unit/dhcpv6_safeguards_test.c new file mode 100644 index 00000000..63692ee3 --- /dev/null +++ b/tests/unit/dhcpv6_safeguards_test.c @@ -0,0 +1,154 @@ +/* Standalone tests of the production parser and reply builders. See run_safeguards.sh. */ +#include <assert.h> +#include "triton.h" +#undef DEFINE_INIT +#define DEFINE_INIT(o, f) +#include "../../accel-pppd/ipv6/dhcpv6_packet.c" +#undef BUF_SIZE +#include "../../accel-pppd/ipv6/dhcpv6.c" + +__thread struct ap_net *net; +static unsigned int sent; +static unsigned char response[4096]; +static size_t response_len; +void log_warn(const char *fmt, ...) {} +void log_emerg(const char *fmt, ...) {} +void log_ppp_error(const char *fmt, ...) {} +void log_ppp_info2(const char *fmt, ...) {} +int ip6addr_add_peer(int i, struct in6_addr *a, struct in6_addr *p) { return 0; } +int ip6route_add(int i, const struct in6_addr *a, int p, const struct in6_addr *g, int m, uint32_t prio, const char *v) { return 0; } +struct ipv6db_prefix_t *ipdb_get_ipv6_prefix(struct ap_session *s) { return NULL; } + +static ssize_t capture_send(int fd, const void *buf, size_t len, int flags, + const struct sockaddr *addr, socklen_t addrlen) +{ + assert(len <= sizeof(response)); + memcpy(response, buf, len); + response_len = len; + sent++; + return len; +} + +static size_t option(unsigned char *p, unsigned int code, const void *data, size_t len) +{ + u_write_be16(p, code); + u_write_be16(p + 2, len); + if (len) + memcpy(p + 4, data, len); + return len + 4; +} + +static size_t request(unsigned char *buf) +{ + static const unsigned char duid[] = {0, 3, 0, 1, 1, 2, 3, 4, 5, 6}; + size_t len = 4; + memset(buf, 0, 4); + buf[0] = D6_SOLICIT; + len += option(buf + len, D6_OPTION_CLIENTID, duid, sizeof(duid)); + len += option(buf + len, D6_OPTION_SERVERID, duid, sizeof(duid)); + return len; +} + +static size_t relay(unsigned char *buf, size_t len) +{ + size_t hdrlen = sizeof(struct dhcpv6_relay_hdr); + memmove(buf + hdrlen + 4, buf, len); + memset(buf, 0, hdrlen); + buf[0] = D6_RELAY_FORW; + u_write_be16(buf + hdrlen, D6_OPTION_RELAY_MSG); + u_write_be16(buf + hdrlen + 2, len); + return len + hdrlen + 4; +} + +int main(void) +{ + unsigned char buf[16000], oro[4000], name[256]; + const unsigned char valid_name[] = {2, 'g', 'w', 0}; + struct dhcpv6_packet *pkt, *reply; + struct dhcpv6_option *opt; + struct ap_session ses = {0}; + struct dhcpv6_pd pd = {0}; + struct ap_net test_net = {.sendto = capture_send}; + size_t len, base, avail; + int i; + + net = &test_net; + base = request(buf); + len = base + option(buf + base, D6_OPTION_RAPID_COMMIT, NULL, 0); + pkt = dhcpv6_packet_parse(buf, len); + assert(pkt && pkt->rapid_commit); + pkt->ses = &ses; + dhcpv6_send_reply(pkt, &pd, D6_REPLY); + assert(sent == 1); + reply = dhcpv6_packet_parse(response, response_len); + assert(reply && reply->rapid_commit); + dhcpv6_packet_free(reply); + dhcpv6_packet_free(pkt); + len += option(buf + len, D6_OPTION_RAPID_COMMIT, NULL, 0); + assert(!dhcpv6_packet_parse(buf, len)); + len = base + option(buf + base, D6_OPTION_RAPID_COMMIT, "x", 1); + assert(!dhcpv6_packet_parse(buf, len)); + + len = base + option(buf + base, D6_OPTION_AFTR_NAME, valid_name, sizeof(valid_name)); + pkt = dhcpv6_packet_parse(buf, len); + assert(pkt); + dhcpv6_packet_free(pkt); + len += option(buf + len, D6_OPTION_AFTR_NAME, valid_name, sizeof(valid_name)); + assert(!dhcpv6_packet_parse(buf, len)); + memset(name, 'x', sizeof(name)); + for (i = 0; i < 5; i++) { + const unsigned char invalid[][4] = {{0,0,0,0}, {0xc0,1,1,0}, {4,'a','b',0}, {2,'a','b',1}, {1,'a',0,1}}; + len = base + option(buf + base, D6_OPTION_AFTR_NAME, invalid[i], 4); + assert(!dhcpv6_packet_parse(buf, len)); + } + len = base + option(buf + base, D6_OPTION_AFTR_NAME, name, sizeof(name)); + assert(!dhcpv6_packet_parse(buf, len)); + + len = request(buf); + for (i = 0; i <= DHCPV6_HOP_COUNT_LIMIT; i++) { + pkt = dhcpv6_packet_parse(buf, len); + assert(pkt); + reply = dhcpv6_packet_alloc_reply(pkt, D6_REPLY); + assert(reply); + /* The last legal byte is writable; both allocators reject overflow. */ + avail = (char *)(reply + 1) + 4096 - (char *)reply->endptr; + opt = dhcpv6_option_alloc(reply, 65000, avail - 4); + assert(opt); + memset(opt->hdr->data, 1, avail - 4); + assert(!dhcpv6_option_alloc(reply, 65000, 0)); + assert(!dhcpv6_nested_option_alloc(reply, opt, 65000, 0)); + assert(!dhcpv6_option_alloc(reply, 65000, -1)); + dhcpv6_fill_relay_info(reply); + assert((char *)reply->endptr == (char *)(reply + 1) + 4096); + /* Verify Relay-Message wire length in every enclosing layer. */ + if (i) { + struct dhcpv6_relay_hdr *h = (void *)reply->hdr; + struct dhcpv6_opt_hdr *o = (void *)h->data; + assert(ntohs(o->len) == (char *)reply->endptr - (char *)o->data); + } + dhcpv6_packet_free(reply); + dhcpv6_packet_free(pkt); + len = relay(buf, len); + } + assert(!dhcpv6_packet_parse(buf, len)); + + /* An ORO can amplify a small request into more than 4096 response bytes. */ + conf_dns_count = 1; + for (i = 0; i < sizeof(oro); i += 2) + u_write_be16(oro + i, D6_OPTION_DNS_SERVERS); + len = request(buf); + len += option(buf + len, D6_OPTION_ORO, oro, sizeof(oro)); + pkt = dhcpv6_packet_parse(buf, len); + assert(pkt); + pkt->ses = &ses; + sent = 0; + dhcpv6_send_reply(pkt, &pd, D6_REPLY); + assert(!sent); + dhcpv6_send_reply2(pkt, &pd, D6_REPLY); + assert(!sent); + dhcpv6_packet_free(pkt); + puts("DHCPv6 safeguards: PASS"); + return 0; +} +void build_ip6_addr(struct ipv6db_addr_t *a, uint64_t id, struct in6_addr *addr) { *addr = a->addr; } +int ip6addr_add(int i, struct in6_addr *a, int p) { return 0; } diff --git a/tests/unit/lcp_safeguards_test.c b/tests/unit/lcp_safeguards_test.c new file mode 100644 index 00000000..32a14dfb --- /dev/null +++ b/tests/unit/lcp_safeguards_test.c @@ -0,0 +1,40 @@ +/* Exercise production Echo-Reply handling at the negotiated MTU. */ +#include <assert.h> +#include "triton.h" +#undef DEFINE_INIT +#define DEFINE_INIT(o, f) +#include "../../accel-pppd/ppp/ppp_lcp.c" + +int conf_ppp_verbose; +static int sent, sent_len; +void log_ppp_debug(const char *fmt, ...) {} +int ppp_chan_send(struct ppp_t *ppp, void *buf, int size) +{ + const struct lcp_hdr_t *hdr = buf; + assert(size == ntohs(hdr->len) + 2); + assert(hdr->code == ECHOREP); + sent++; + sent_len = size; + return 0; +} +int main(void) +{ + unsigned char buf[1502] = {0}; + struct ppp_t ppp = {.buf = buf, .buf_size = sizeof(buf), .mtu = 1492}; + struct ppp_lcp_t lcp = {.ppp = &ppp, .magic = 0x12345678}; + struct lcp_hdr_t *hdr = (void *)buf; + + hdr->len = htons(1493); + hdr->code = ECHOREQ; + send_echo_reply(&lcp); + assert(!sent && hdr->code == ECHOREQ); + hdr->len = htons(1492); + send_echo_reply(&lcp); + assert(sent == 1 && sent_len == 1494); + assert(u_read_be32(hdr + 1) == lcp.magic); + hdr->len = htons(8); + send_echo_reply(&lcp); + assert(sent == 2 && sent_len == 10); + puts("LCP safeguards: PASS"); + return 0; +} diff --git a/tests/unit/mempool_safeguards_test.c b/tests/unit/mempool_safeguards_test.c new file mode 100644 index 00000000..b0c6a3dc --- /dev/null +++ b/tests/unit/mempool_safeguards_test.c @@ -0,0 +1,38 @@ +/* Pool reuse must not expose the previous object's payload. */ +#include <assert.h> +#include <pthread.h> +#include "triton.h" +#undef __init +#define __init +#include "../../accel-pppd/triton/mempool.c" + +void triton_log_error(const char *fmt, ...) {} +void triton_stat_mempool_allocated_add(uint64_t v) {} +void triton_stat_mempool_allocated_sub(uint64_t v) {} +void triton_stat_mempool_available_add(uint64_t v) {} +void triton_stat_mempool_available_sub(uint64_t v) {} + +int main(void) +{ + mempool_t *pool; + unsigned char *p, *q; + int i; + + spinlock_init(&pools_lock); + pool = mempool_create(128); + p = mempool_alloc(pool); + assert(p); + memset(p, 0xa5, 128); + mempool_free(p); + q = mempool_alloc(pool); + assert(q == p); + for (i = 0; i < 128; i++) + assert(!q[i]); + memset(q, 0x5a, 128); + mempool_free(q); + mempool_clean(); + list_del(&((struct _mempool_t *)pool)->entry); + free(pool); + puts("Mempool safeguards: PASS"); + return 0; +} diff --git a/tests/unit/radius_safeguards_test.c b/tests/unit/radius_safeguards_test.c new file mode 100644 index 00000000..6b925bf4 --- /dev/null +++ b/tests/unit/radius_safeguards_test.c @@ -0,0 +1,119 @@ +/* Exercise production response verification and accounting signing with real packets. */ +#include <assert.h> +#include "triton.h" +#undef DEFINE_INIT +#define DEFINE_INIT(o, f) +#include "../../accel-pppd/radius/req.c" +#include "../../accel-pppd/radius/packet.c" + +static const char *server_secret = "first-secret"; +static int healthy, delivered; +int conf_verbose; +char *rad_server_secret_dup(struct rad_server_t *s) { return strdup(server_secret); } +void log_emerg(const char *fmt, ...) {} +void log_ppp_error(const char *fmt, ...) {} +void log_ppp_warn(const char *fmt, ...) {} +void log_switch(struct triton_context_t *ctx, void *arg) {} +struct triton_context_t *triton_context_self(void) { return NULL; } +void rad_server_reply(struct rad_server_t *s) { healthy++; } +void rad_server_req_exit(struct rad_req_t *r) { r->active = 0; } +int rad_server_req_cancel(struct rad_req_t *r, int full) { return 0; } +void *mempool_alloc(mempool_t *p) { return calloc(1, (size_t)p); } +void mempool_free(void *p) { free(p); } +struct rad_dict_attr_t *rad_dict_find_attr_id(struct rad_dict_vendor_t *v, int id) { return NULL; } +struct rad_dict_vendor_t *rad_dict_find_vendor_id(int id) { return NULL; } +struct rad_dict_value_t *rad_dict_find_val(struct rad_dict_attr_t *a, rad_value_t v) { return NULL; } + +static void receive_reply(struct rad_req_t *req) { delivered++; } +static void sign_response(unsigned char *buf, int code, const unsigned char *ra, const char *secret) +{ + unsigned char input[256]; + size_t n = strlen(secret); + memset(buf, 0, 20); + buf[0] = code; + buf[1] = 1; + buf[3] = 20; + memcpy(input, buf, 4); + memcpy(input + 4, ra, 16); + memcpy(input + 20, secret, n); + MD5(input, 20 + n, buf + 4); +} + +int main(void) +{ + struct rad_packet_t request = {.code = CODE_ACCOUNTING_REQUEST, .id = 1, .len = 20}; + struct rad_packet_t response_packet = {.len = 20}; + struct rad_server_t server = {0}; + struct rad_req_t req = {.pack = &request, .serv = &server, .recv = receive_reply}; + unsigned char response[20], expected[16], input[256], first_ra[16]; + int pair[2], code, i; + + packet_pool = (void *)sizeof(struct rad_packet_t); + buf_pool = (void *)REQ_LENGTH_MAX; + INIT_LIST_HEAD(&request.attrs); + assert(!rad_req_set_RA(&req)); + memcpy(first_ra, req.RA, 16); + memcpy(input, request.buf, 20); + memset(input + 4, 0, 16); + memcpy(input + 20, server_secret, strlen(server_secret)); + MD5(input, 20 + strlen(server_secret), expected); + assert(!memcmp(expected, req.RA, 16)); + /* Rebuilding must zero the previous digest before hashing. */ + assert(!rad_req_set_RA(&req)); + assert(!memcmp(first_ra, req.RA, 16)); + response_packet.buf = response; + for (i = 0; i < 4; i++) { + const int codes[] = {CODE_ACCESS_ACCEPT, CODE_ACCESS_REJECT, CODE_ACCESS_CHALLENGE, CODE_ACCOUNTING_RESPONSE}; + code = codes[i]; + sign_response(response, code, request.buf + 4, server_secret); + assert(!verify_response_authenticator(&req, &response_packet)); + response[4] ^= 1; + assert(verify_response_authenticator(&req, &response_packet)); + sign_response(response, code, first_ra, "wrong-secret"); + assert(verify_response_authenticator(&req, &response_packet)); + } + /* An in-flight response still verifies after the configuration changes. */ + sign_response(response, CODE_ACCOUNTING_RESPONSE, first_ra, server_secret); + server_secret = "second-secret"; + assert(!verify_response_authenticator(&req, &response_packet)); + /* A new accounting send after failover/reload signs with the new secret. */ + assert(!rad_req_set_RA(&req)); + assert(memcmp(first_ra, req.RA, 16)); + assert(verify_response_authenticator(&req, &response_packet)); + sign_response(response, CODE_ACCOUNTING_RESPONSE, req.RA, server_secret); + assert(!verify_response_authenticator(&req, &response_packet)); + + assert(!socketpair(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0, pair)); + req.hnd.fd = pair[1]; + response[4] ^= 1; + assert(write(pair[0], response, 20) == 20); + assert(!rad_req_read(&req.hnd)); + assert(!healthy && !delivered && !req.reply); + response[4] ^= 1; + assert(write(pair[0], response, 20) == 20); + assert(rad_req_read(&req.hnd) == 1); + assert(healthy == 1 && delivered == 1 && req.reply); + rad_packet_free(req.reply); + /* Outbound Message-Authenticator retransmits must hash a zeroed field. */ + { + unsigned char wire[38] = {CODE_ACCESS_REQUEST, 1, 0, 38}; + unsigned char first[38], second[38]; + struct rad_packet_t access = { + .buf = wire, .len = sizeof(wire), .message_authenticator = 1, + .secret = (uint8_t *)"blast-secret" + }; + wire[20] = 80; + wire[21] = 18; + assert(!rad_packet_send(&access, pair[0], NULL)); + assert(read(pair[1], first, sizeof(first)) == sizeof(first)); + assert(!rad_packet_send(&access, pair[0], NULL)); + assert(read(pair[1], second, sizeof(second)) == sizeof(second)); + assert(!memcmp(first, second, sizeof(first))); + } + close(pair[0]); + close(pair[1]); + free(request.secret); + free(request.buf); + puts("RADIUS safeguards: PASS"); + return 0; +} diff --git a/tests/unit/run_safeguards.sh b/tests/unit/run_safeguards.sh new file mode 100644 index 00000000..1c69efd7 --- /dev/null +++ b/tests/unit/run_safeguards.sh @@ -0,0 +1,21 @@ +#!/bin/sh +# Run from any directory, passing a configured CMake build directory. +set -eu +root=$(CDPATH= cd -- "$(dirname -- "$0")/../.." && pwd) +build=${1:?usage: run_safeguards.sh /path/to/cmake-build} +out=$(mktemp -d) +trap 'rm -rf "$out"' EXIT HUP INT TERM +cd "$root" +for name in dhcpv6 radius vrf lcp mempool; do + extra= + # LCP's global layer table retains the whole daemon under ASan; its test + # checks wire lengths with UBSan. The packet/VRF tests also use ASan. + if [ "$name" = lcp ]; then extra=-fno-sanitize=address; fi + ${CC:-cc} -O1 -g -Wall -Wno-unused-function -Wno-unused-result -D_GNU_SOURCE -DAP_SESSIONID_LEN=16 \ + -DOPENSSL_API_COMPAT=0x10100000L -fno-strict-aliasing \ + -ffunction-sections -fdata-sections -Wl,--gc-sections \ + -fsanitize=address,undefined -fno-sanitize-recover=all $extra \ + -I "$build" -I accel-pppd -I accel-pppd/include -I accel-pppd/triton \ + -o "$out/$name" "tests/unit/${name}_safeguards_test.c" -lcrypto -lpthread + "$out/$name" +done diff --git a/tests/unit/vrf_safeguards_test.c b/tests/unit/vrf_safeguards_test.c new file mode 100644 index 00000000..1fa5a414 --- /dev/null +++ b/tests/unit/vrf_safeguards_test.c @@ -0,0 +1,72 @@ +/* Exercise the shared interface lookup and length-delimited VRF API. */ +#include <assert.h> +#include <pthread.h> +#include "triton.h" +#undef DEFINE_INIT +#undef __init +#define __init +#define DEFINE_INIT(o, f) +#include "../../accel-pppd/net.c" +#include "../../accel-pppd/ifcfg.c" +#define init coa_init +#include "../../accel-pppd/radius/dm_coa.c" +#undef init + +static int lookups, changes, last_master; +static char last_name[IFNAMSIZ]; +void log_ppp_error(const char *fmt, ...) {} +void log_ppp_info2(const char *fmt, ...) {} + +static int lookup(const char *name) +{ + lookups++; + assert(strlen(name) < IFNAMSIZ); + strcpy(last_name, name); + return 7; +} +static int set_vrf(int index, int master) +{ + changes++; + last_master = master; + return 0; +} + +int main(void) +{ + struct kern_net kn = {0}; + struct ap_net backend = {.get_ifindex = lookup, .set_vrf = set_vrf}; + struct ap_session ses = {.net = &backend}; + struct radius_pd_t rpd = {.ses = &ses}; + char name[IFNAMSIZ + 1]; + + net = &kn.net; + kn.sock = socket(AF_INET, SOCK_DGRAM, 0); + assert(kn.sock >= 0); + assert(def_get_ifindex("lo") > 0); + assert(def_get_ifindex(NULL) == -1); + memset(name, 'x', sizeof(name)); + name[IFNAMSIZ] = 0; + assert(def_get_ifindex(name) == -1); + assert(ap_session_vrf(&ses, name, -1) == -1); + assert(ap_session_vrf(&ses, name, IFNAMSIZ) == -1); + assert(ap_session_vrf(&ses, NULL, 1) == -1); + assert(ap_session_vrf(&ses, name, -2) == -1); + assert(ap_session_vrf(&ses, "a\0b", 3) == -1); + assert(!lookups && !changes); + /* Explicit length need not be followed by a NUL in caller memory. */ + assert(!ap_session_vrf(&ses, name, IFNAMSIZ - 1)); + assert(strlen(last_name) == IFNAMSIZ - 1 && last_master == 7); + assert(!ap_session_vrf(&ses, NULL, 0)); + assert(last_master == 0 && !ses.vrf_name); + assert(!ap_session_vrf(&ses, NULL, -1)); + assert(lookups == 1 && changes == 3); + assert(!rad_update_vrf(&rpd, "blue\0junk", 9)); + assert(!rad_update_vrf(&rpd, name, IFNAMSIZ)); + assert(rad_update_vrf(&rpd, "0", 1)); + assert(last_master == 0); + assert(rad_update_vrf(&rpd, "0blue", 5)); + assert(last_master == 7 && !strcmp(last_name, "0blue")); + close(kn.sock); + puts("VRF safeguards: PASS"); + return 0; +} |