Skip to content

[dhcp4relay] Reject chained requests with a local giaddr - #126

Draft
Xichen96 wants to merge 1 commit into
sonic-net:masterfrom
Xichen96:dev/xichenlin/item10-local-giaddr-spoof
Draft

[dhcp4relay] Reject chained requests with a local giaddr#126
Xichen96 wants to merge 1 commit into
sonic-net:masterfrom
Xichen96:dev/xichenlin/item10-local-giaddr-spoof

Conversation

@Xichen96

@Xichen96 Xichen96 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Description of PR

Summary:

Reject nonzero-giaddr chained requests when giaddr matches any IPv4 address
owned by the local relay, preventing relay loops and local-address spoofing.

This is a low-priority placeholder for a non-current deployment scenario. This
draft must not merge yet.

Type of change

  • Bug fix

Approach

What is the motivation for this PR?

RFC 3046 requires rejecting a reforwarded request whose giaddr spoofs an
address implemented by the local relay. Native relay currently performs no
local-address check.

How did you do it?

  • Enumerate all local IPv4 interface addresses for each chained request rather
    than comparing only one VLAN/source field.
  • Drop and count packets whose giaddr matches any local address.
  • Fail closed with an explicit error when local-address enumeration fails.
  • Add focused native coverage for a local match on a secondary enumerated
    address and a nonlocal forwarding case.

How did you verify/test it?

Focused native unit code is included but was NOT RUN, per explicit test
deferral. Test execution is deferred until the higher-priority DHCP relay PRs
listed below merge.

Any platform specific information?

None. This covers a lower-priority, non-current deployment scenario.

Dependencies and future validation TODOs

Validation sequencing depends on these higher-priority relay PRs merging first
(no code dependency; this branch is independently based on fresh origin/master):

Before marking ready or merging:

  • Execute the focused native unit coverage included in this PR.
  • Open the dependent sonic-mgmt PR proving local-giaddr drop across more
    than one relay-owned address and nonlocal-giaddr forwarding:
    [dhcp_relay] Cover local giaddr loop rejection sonic-mgmt#26325
  • Verify the drop counter and diagnostic, including the fail-closed lookup
    error path.
  • Run applicable PR CI/hardware validation after the dependency PRs merge.
  • Keep this PR in draft; do not merge before these TODOs are complete.

Documentation

Behavior follows RFC 3046 local-giaddr loop/spoof protection.

Check nonzero giaddr values against all local IPv4 interface addresses and drop local matches or lookup failures before forwarding.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 2b881aa9-3a3a-4aaf-b2ca-b941705b2438
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants