fix(security): validate UPSTREAM_DNS before shell interpolation#1141
fix(security): validate UPSTREAM_DNS before shell interpolation#1141fdzdev wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughValidate Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as "fix-coredns.sh"
participant jq
participant kubectl
participant K8sAPI as "Kubernetes API"
User->>Script: invoke with UPSTREAM_DNS
Script->>Script: validate UPSTREAM_DNS (charset)
alt invalid
Script-->>User: exit 1 (invalid input)
else valid
Script->>jq: escape Corefile heredoc -> PATCH_JSON
jq-->>Script: PATCH_JSON
Script->>kubectl: kubectl patch --patch PATCH_JSON
kubectl->>K8sAPI: PATCH request
K8sAPI-->>kubectl: 200 OK
kubectl-->>Script: patch applied
Script->>kubectl: rollout restart deployment/coredns
kubectl->>K8sAPI: restart request
K8sAPI-->>kubectl: restart accepted
kubectl-->>Script: restart triggered
Script-->>User: exit 0
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/fix-coredns.sh`:
- Line 70: The current if condition that tests UPSTREAM_DNS uses a permissive
regex and accepts invalid IPv4 addresses; change the validation in the if block
that references UPSTREAM_DNS so it enforces each octet is 0–255 (i.e., replace
the simple dot-number regex with a strict per-octet check that only matches
0–255 for all four octets) or alternatively call a reliable validator (e.g.,
POSIX utility or a short python/ipaddress check) to ensure UPSTREAM_DNS is a
valid IPv4 before proceeding to patch CoreDNS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a616607c-2d46-4ca2-8036-8e356e17d6b8
📒 Files selected for processing (1)
scripts/fix-coredns.sh
scripts/fix-coredns.sh
Outdated
|
|
||
| # Validate UPSTREAM_DNS is a valid IPv4 address before shell-interpolating | ||
| # into the docker exec command (CWE-78, NVBUG 6009988). | ||
| if ! echo "$UPSTREAM_DNS" | grep -qE '^([0-9]{1,3}\.){3}[0-9]{1,3}$'; then |
There was a problem hiding this comment.
IPv4 validation on Line 70 is too permissive and accepts invalid addresses.
The current regex allows octets outside 0-255 (e.g., 999.1.1.1), so invalid DNS values can still be patched into CoreDNS and break resolution.
Proposed fix
-if ! echo "$UPSTREAM_DNS" | grep -qE '^([0-9]{1,3}\.){3}[0-9]{1,3}$'; then
+if ! printf '%s\n' "$UPSTREAM_DNS" | awk -F. '
+ NF != 4 { exit 1 }
+ {
+ for (i = 1; i <= 4; i++) {
+ if ($i !~ /^[0-9]+$/ || $i < 0 || $i > 255) exit 1
+ }
+ }
+ { exit 0 }
+'; then
echo "ERROR: UPSTREAM_DNS='$UPSTREAM_DNS' is not a valid IPv4 address. Aborting."
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! echo "$UPSTREAM_DNS" | grep -qE '^([0-9]{1,3}\.){3}[0-9]{1,3}$'; then | |
| if ! printf '%s\n' "$UPSTREAM_DNS" | awk -F. ' | |
| NF != 4 { exit 1 } | |
| { | |
| for (i = 1; i <= 4; i++) { | |
| if ($i !~ /^[0-9]+$/ || $i < 0 || $i > 255) exit 1 | |
| } | |
| } | |
| { exit 0 } | |
| '; then | |
| echo "ERROR: UPSTREAM_DNS='$UPSTREAM_DNS' is not a valid IPv4 address. Aborting." | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/fix-coredns.sh` at line 70, The current if condition that tests
UPSTREAM_DNS uses a permissive regex and accepts invalid IPv4 addresses; change
the validation in the if block that references UPSTREAM_DNS so it enforces each
octet is 0–255 (i.e., replace the simple dot-number regex with a strict
per-octet check that only matches 0–255 for all four octets) or alternatively
call a reliable validator (e.g., POSIX utility or a short python/ipaddress
check) to ensure UPSTREAM_DNS is a valid IPv4 before proceeding to patch
CoreDNS.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Thanks for tackling this. I don't think we should merge the current approach as-is.
My main concern is that the fix constrains UPSTREAM_DNS to an IPv4 dotted-quad, which can reject valid upstreams unnecessarily (for example IPv6 nameservers). That makes the script less robust without really addressing the root issue.
I also think the security claim here is a bit overstated: the current code is definitely doing unsafe string construction, but the right fix is to stop interpolating the value into a shell/JSON blob directly, not to ban everything except IPv4.
I'd prefer a rework that constructs the kubectl patch payload safely (for example via a temp file / heredoc / proper escaping / jq-style JSON generation) and then validates the resolved upstream in a way that still permits legitimate values.
wscurran
added
security
Replace unsafe inline JSON string construction with jq-based payload generation so UPSTREAM_DNS is never interpolated into a shell-constructed JSON/string literal (CWE-78, NVBUG 6009988). - Build Corefile as a heredoc, let jq handle all JSON escaping - Defense-in-depth character-class check accepts IPv4, IPv6, and hostnames - Require jq (standard on Ubuntu/Brev VMs) with explicit check Made-with: Cursor
fdzdev
force-pushed
the
fix/coredns-dns-injection
branch
from
352ce80 to
d681791
Compare
|
Reworked in d681791 — thanks for the feedback.
Ready for re-review. |
3 participants
Summary
UPSTREAM_DNSbefore it is interpolated into thedocker exec kubectl patchcommand (CWE-78, NVBUG 6009988)resolv.confentry could inject shell metacharacters into the kubectl patch JSON payloadTest plan
fix-coredns.shwith valid DNS (e.g.8.8.8.8) — patches CoreDNS normallyfix-coredns.shwith no resolvable DNS — falls back to8.8.8.8, passes validationUPSTREAM_DNS='"; rm -rf /'— rejected with error, no executionSummary by CodeRabbit