Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a shell command injection risk in the client hostname resolution path (when using popen("host ...")) and expands regression coverage to detect attempted command execution via crafted -connect hostnames.
Changes:
- Add hostname character validation before constructing a
popen()command in the client. - Add client regression tests that ensure injected shell metacharacters do not result in file creation.
- Loosen OCSP interop test error-message matching and adjust hash-type switch guards for newer wolfSSL versions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/client/client.c |
Adds hostname validation to prevent shell metacharacters from reaching the popen() command. |
tests/client/client-test.sh |
Adds regression tests intended to detect hostname-based shell injection. |
tests/ocsp/ocsp-interop-test.sh |
Expands grep patterns for expected failure messages in interop testing. |
src/x509/clu_x509_sign.c |
Updates preprocessor version gating for additional WC_HASH_TYPE_* enum cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
miyazakh
force-pushed
the
f-739_shellcommand
branch
from
566a39f to
b55a628
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const char* cp; | ||
|
|
||
| /* Validate hostname: only allow characters valid in DNS names | ||
| * (RFC 1123) to prevent shell injection via popen(). */ | ||
| for (cp = peer; *cp != '\0'; cp++) { | ||
| if (!isalnum((unsigned char)*cp) && | ||
| *cp != '.' && *cp != '-') { | ||
| err_sys("invalid character in hostname"); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
isalnum() is locale-dependent, which can allow non-ASCII bytes to be treated as “alnum” under some locales. Since this validation is part of a shell-injection mitigation, it’s safer to enforce an explicit ASCII allowlist (e.g., [A-Za-z0-9.-]) rather than isalnum(), so behavior is deterministic across environments/locales.
| /* Validate hostname: only allow characters valid in DNS names | ||
| * (RFC 1123) to prevent shell injection via popen(). */ | ||
| for (cp = peer; *cp != '\0'; cp++) { |
There was a problem hiding this comment.
This change enforces “DNS hostname only” semantics on peer for the WOLFSSL_USE_POPEN_HOST path; if peer previously supported IP literals (notably IPv6 forms) in this path, this will now reject them due to : / []. If IP literals are expected to work, consider detecting numeric addresses (or bracketed IPv6) and bypassing the popen("host ...") code path entirely (use getaddrinfo), while keeping the strict hostname allowlist for actual DNS names.
| ./wolfssl s_client -connect 'evil.com;touch clu_injection_probe.txt:443' \ | ||
| 2>/dev/null |
There was a problem hiding this comment.
These regression tests can become flaky/hang in CI because they rely on external DNS/network behavior (e.g., evil.com) and may block longer than desired depending on resolver settings. To make the tests deterministic, wrap each invocation with a short timeout (and consider redirecting stdout as well) so the test suite can’t stall on slow resolution/connection attempts while still validating that no injected touch executes.
| ./wolfssl s_client -connect 'evil$(touch clu_injection_probe.txt).com:443' \ | ||
| 2>/dev/null |
There was a problem hiding this comment.
These regression tests can become flaky/hang in CI because they rely on external DNS/network behavior (e.g., evil.com) and may block longer than desired depending on resolver settings. To make the tests deterministic, wrap each invocation with a short timeout (and consider redirecting stdout as well) so the test suite can’t stall on slow resolution/connection attempts while still validating that no injected touch executes.
| ./wolfssl s_client -connect 'evil.com|touch clu_injection_probe.txt:443' \ | ||
| 2>/dev/null |
There was a problem hiding this comment.
These regression tests can become flaky/hang in CI because they rely on external DNS/network behavior (e.g., evil.com) and may block longer than desired depending on resolver settings. To make the tests deterministic, wrap each invocation with a short timeout (and consider redirecting stdout as well) so the test suite can’t stall on slow resolution/connection attempts while still validating that no injected touch executes.
F-739 : Shell command injection via popen with unsensitized hostname
Add test coverage
Depend on : #211 (Fixed)
Depend on : #219