lib, zebra: bound SRv6 locator name length in ZAPI

[jamestiotio]

zread_srv6_manager_get_srv6_sid() and zread_srv6_manager_get_locator() read a uint16_t length from the ZAPI stream and pass it directly to STREAM_GET() to copy into a 256-byte stack buffer (SRV6_LOCNAME_SIZE), without bounding the length first. STREAM_GET() only validates the source side of the read; the destination is a raw memcpy. A malformed ZAPI message with len >= SRV6_LOCNAME_SIZE writes past the buffer on Zebra's stack, up to the ZAPI packet cap of around 16 KiB. Normally, stack canaries would catch this and cause Zebra to abort (which could lead to a DoS). Let's fix the root cause of the issue instead of relying on stack canaries by adding a guard in both handlers.

For the other similar guards, change the check to len >= SRV6_LOCNAME_SIZE to accommodate the trailing null terminator. Additionally, print SRV6_LOCNAME_SIZE - 1 as the maximum in the warnings so that the message stays accurate, and make the warning format across the Zebra handlers consistent.

These SRv6 locator name length checks are also refactored into a helper macro so that they do not have to be done manually everywhere.

Also add a helper for the ZAPI send paths that emit an SRv6 locator name. The helper bounds the read with strnlen(), treats NULL as an empty name, and warns then truncates if the input lacks a null terminator within SRV6_LOCNAME_SIZE bytes, so that the byte stream on the wire stays well-formed.

[greptile-apps]

Greptile Summary

This PR hardens SRv6 locator name handling on the ZAPI receive path by bounding the user-supplied length before the stack memcpy, and centralises the encode/decode logic into two helpers (zapi_srv6_locname_encode / zapi_srv6_locname_decode) and a macro (ZAPI_GET_SRV6_LOCNAME).

• Receive (decode): The new zapi_srv6_locname_decode uses len >= bufsize (correct >=, fixing the pre-existing off-by-one) and falls back to stream_failure on any malformed input, closing the stack-overwrite path described in the PR.
• Send (encode): zapi_srv6_locname_encode handles NULL names as empty and truncates with a warning when the name lacks a null terminator within SRV6_LOCNAME_SIZE, keeping the wire format well-formed.
• Existing call sites: All six Zebra ZAPI handlers and the encode functions in zclient.c are updated to use the new helpers, eliminating all ad-hoc length checks.

Confidence Score: 5/5

The change tightens bounds checking on ZAPI input and consolidates previously scattered guards; all call sites in zebra/zapi_msg.c use zero-initialised buffers, so the new helpers are correct as written.

The decode path now correctly rejects names where len >= bufsize, closing the stack-overwrite window. The encode path uses strnlen to guard against unterminated inputs. The only open question is whether zapi_srv6_locname_decode should own the null terminator itself, but this is a pre-existing design characteristic and does not introduce any regression.

lib/zclient.c — specifically zapi_srv6_locname_decode, which writes len bytes but does not write a null terminator; safe for current callers but could silently misbehave if future callers pass non-zeroed struct fields.

Important Files Changed

Filename	Overview
lib/zclient.c	Introduces zapi_srv6_locname_encode/decode helpers and refactors all SRv6 locator name encode/decode paths to use them; decode uses >= for bound check (fixing the off-by-one vs old > check) but does not explicitly null-terminate the output buffer
lib/zclient.h	Declares the two new helpers and the ZAPI_GET_SRV6_LOCNAME macro; sizeof(BUF) is safe because all macro call sites use stack-allocated array names (not pointers)
zebra/zapi_msg.c	All six SRv6 locator-name read paths replaced with ZAPI_GET_SRV6_LOCNAME; every buffer is stack-allocated with = {0}, guaranteeing null-termination

Sequence Diagram

sequenceDiagram
    participant C as ZAPI Client
    participant ZC as zclient.c (encode)
    participant NET as ZAPI Stream
    participant ZM as zapi_msg.c (decode)
    participant ZK as Zebra kernel

    C->>ZC: srv6_manager_get_sid(locator_name)
    ZC->>ZC: zapi_srv6_locname_encode() strnlen bound + truncate warn
    ZC->>NET: putw(len) + put(bytes)

    NET->>ZM: zread_srv6_manager_get_srv6_sid()
    ZM->>ZM: "ZAPI_GET_SRV6_LOCNAME() -> zapi_srv6_locname_decode() len >= bufsize? -> stream_failure"
    ZM->>ZK: srv6_manager_get_sid_call()

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
lib/zclient.c:1274-1280
The function reads exactly `len` bytes into `buf` but does not write a null terminator at `buf[len]`. For callers in `zapi_srv6_locator_chunk_decode` and `zapi_srv6_locator_decode`, the destination is a struct field whose prior content is caller-controlled. Adding an explicit null byte here makes null-termination a property of the function rather than an implicit requirement on the caller.

```suggestion
	if (len >= bufsize) {
		zlog_warn("%s: SRv6 locator name length %u exceeds maximum %zu", caller, len,
			  bufsize - 1);
		return false;
	}
	if (!stream_get2(buf, s, len))
		return false;
	buf[len] = '\0';
	return true;
}
```

_{Reviews (3): Last reviewed commit: "lib, zebra: bound SRv6 locator name leng..." | Re-trigger Greptile}

[ton31337]

@Mergifyio backport stable/10.6 stable/10.5 stable/10.4

[mergify]

backport stable/10.6 stable/10.5 stable/10.4

🟠 Waiting for conditions to match

Details

• merged [📌 backport requirement]

[ton31337]

Also, could you apply frrbot (styling) patch?

[jamestiotio]

Will do. I will also check the off-by-one issue that Greptile pointed out.

[jamestiotio]

@ton31337 I have updated the PR. I changed a few more checks from > to >=. That should address the off-by-one issue.

[cscarpitta]

@greptile review

[cscarpitta]

LGTM

[mjstapp]

gosh - doesn't it seem like a problem that there are so very many places that have to make this decision - and make it correctly? can't we put an api in place so that the rules for handling this attribute are done in one place?

[jamestiotio]

@mjstapp I agree that repeating the length checks everywhere is prone to errors and can easily become very messy. I can add a macro and refactor the code to use that macro instead so that we do not have to keep manually checking the length.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[mjstapp]

so there look to be two pieces of mess in this srv6 code:

• a "locator name" is a cstring, but it is usually (always?) encoded unterminated
• the decoders for the objects that use that string don't consistently deal with that missing NULL

changing the > to a >= just moves the problem around, assuming (hoping) that the callers will manage the NULL properly in ... several/many places.

I think it would be safer to have "locator_name_encode" and "_decode" functions that get used instead of the copy-pasted approach we have now. the string should be sent terminated, and the decoder should ensure that whatever is returned to its caller is NULL-terminated.

[jamestiotio]

@greptile review