Skip to content

Commit 0173d7e

Browse files
s-b-e-n-s-o-ns-b-e-n-s-o-n
committed
🔒 security(healthcheck): harden HTTPS probe against shell injection by replacing popen with direct fork/exec
- Drop popen()/FILE stream approach that relied on shell command interpolation - Locate openssl binary explicitly from known paths via access() probe - Fork/exec with pipes, poll-driven I/O, SIGPIPE handling, and explicit child cleanup - Add popen-blocker test that verifies the probe still succeeds when popen() is unavailable
1 parent ad32518 commit 0173d7e

File tree

2 files changed

+283
-17
lines changed

2 files changed

+283
-17
lines changed

‎app/healthcheck.binary.test.ts‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,71 @@ async function runProbe(binaryPath: string, port: number, env: NodeJS.ProcessEnv
5454
});
5555
}
5656

57+
function compilePopenBlocker(sourcePath: string, libraryPath: string) {
58+
const source =
59+
process.platform === 'darwin'
60+
? [
61+
'#include <errno.h>',
62+
'#include <stdio.h>',
63+
'',
64+
'#define DYLD_INTERPOSE(_replacement, _replacee) \\',
65+
' __attribute__((used)) static struct { \\',
66+
' const void *replacement; \\',
67+
' const void *replacee; \\',
68+
' } _interpose_##_replacee \\',
69+
' __attribute__((section("__DATA,__interpose"))) = { \\',
70+
' (const void *)(unsigned long)&_replacement, \\',
71+
' (const void *)(unsigned long)&_replacee \\',
72+
' };',
73+
'',
74+
'static FILE *block_popen(const char *command, const char *type) {',
75+
' (void)command;',
76+
' (void)type;',
77+
' errno = EPERM;',
78+
' return NULL;',
79+
'}',
80+
'',
81+
'DYLD_INTERPOSE(block_popen, popen);',
82+
'',
83+
].join('\n')
84+
: [
85+
'#include <errno.h>',
86+
'#include <stdio.h>',
87+
'',
88+
'FILE *popen(const char *command, const char *type) {',
89+
' (void)command;',
90+
' (void)type;',
91+
' errno = EPERM;',
92+
' return NULL;',
93+
'}',
94+
'',
95+
].join('\n');
96+
97+
fs.writeFileSync(sourcePath, source);
98+
99+
const args =
100+
process.platform === 'darwin'
101+
? ['-dynamiclib', sourcePath, '-o', libraryPath]
102+
: ['-shared', '-fPIC', sourcePath, '-o', libraryPath];
103+
104+
execFileSync('cc', args, { stdio: 'ignore' });
105+
}
106+
107+
function withPopenBlocked(env: NodeJS.ProcessEnv, libraryPath: string): NodeJS.ProcessEnv {
108+
if (process.platform === 'darwin') {
109+
return {
110+
...env,
111+
DYLD_FORCE_FLAT_NAMESPACE: '1',
112+
DYLD_INSERT_LIBRARIES: libraryPath,
113+
};
114+
}
115+
116+
return {
117+
...env,
118+
LD_PRELOAD: libraryPath,
119+
};
120+
}
121+
57122
const probeHandler: http.RequestListener = (req, res) => {
58123
if (req.url === '/health') {
59124
res.writeHead(200, { 'content-type': 'application/json' });
@@ -69,6 +134,11 @@ describe('/bin/healthcheck compatibility', () => {
69134
const binaryPath = path.join(tempDir, 'healthcheck');
70135
const keyPath = path.join(tempDir, 'key.pem');
71136
const certPath = path.join(tempDir, 'cert.pem');
137+
const popenBlockerSourcePath = path.join(tempDir, 'block-popen.c');
138+
const popenBlockerLibraryPath = path.join(
139+
tempDir,
140+
process.platform === 'darwin' ? 'block-popen.dylib' : 'block-popen.so',
141+
);
72142
const sourcePath = path.resolve(import.meta.dirname, '..', 'healthcheck.c');
73143

74144
beforeAll(() => {
@@ -93,6 +163,7 @@ describe('/bin/healthcheck compatibility', () => {
93163
{ stdio: 'ignore' },
94164
);
95165
execFileSync('cc', ['-Os', sourcePath, '-o', binaryPath], { stdio: 'ignore' });
166+
compilePopenBlocker(popenBlockerSourcePath, popenBlockerLibraryPath);
96167
});
97168

98169
afterAll(() => {
@@ -127,4 +198,30 @@ describe('/bin/healthcheck compatibility', () => {
127198
await close(server);
128199
}
129200
});
201+
202+
test('succeeds against HTTPS without relying on popen()', async () => {
203+
const server = https.createServer(
204+
{
205+
key: fs.readFileSync(keyPath),
206+
cert: fs.readFileSync(certPath),
207+
},
208+
probeHandler,
209+
);
210+
211+
const port = await listen(server);
212+
try {
213+
expect(
214+
await runProbe(
215+
binaryPath,
216+
port,
217+
withPopenBlocked(
218+
{ ...process.env, DD_SERVER_TLS_ENABLED: 'true' },
219+
popenBlockerLibraryPath,
220+
),
221+
),
222+
).toBe(0);
223+
} finally {
224+
await close(server);
225+
}
226+
});
130227
});

‎healthcheck.c‎

Lines changed: 186 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,13 @@
1111
*/
1212

1313
#include <sys/time.h>
14+
#include <sys/wait.h>
1415
#include <arpa/inet.h>
16+
#include <errno.h>
17+
#include <fcntl.h>
1518
#include <netinet/in.h>
19+
#include <poll.h>
20+
#include <signal.h>
1621
#include <stdlib.h>
1722
#include <stdio.h>
1823
#include <string.h>
@@ -37,30 +42,194 @@ static int is_tls_enabled(void) {
3742
return value != NULL && (strcasecmp(value, "true") == 0 || strcmp(value, "1") == 0);
3843
}
3944

40-
static int probe_https(int port) {
41-
char cmd[BUF_SIZE * 2];
42-
snprintf(
43-
cmd,
44-
sizeof(cmd),
45-
"printf 'GET /health HTTP/1.0\\r\\nHost: localhost\\r\\n\\r\\n' | "
46-
"openssl s_client -quiet -connect 127.0.0.1:%d -servername localhost 2>/dev/null",
47-
port
48-
);
49-
50-
FILE *fp = popen(cmd, "r");
51-
if (!fp)
45+
static const char *find_openssl_binary(void) {
46+
static const char *candidates[] = {
47+
"/usr/bin/openssl",
48+
"/bin/openssl",
49+
"/usr/local/bin/openssl",
50+
"/opt/homebrew/bin/openssl",
51+
NULL,
52+
};
53+
54+
for (size_t i = 0; candidates[i] != NULL; i++) {
55+
if (access(candidates[i], X_OK) == 0)
56+
return candidates[i];
57+
}
58+
59+
return NULL;
60+
}
61+
62+
static int wait_for_fd(int fd, short events) {
63+
struct pollfd pfd = {
64+
.fd = fd,
65+
.events = events,
66+
};
67+
68+
while (1) {
69+
int rc = poll(&pfd, 1, TIMEOUT_SEC * 1000);
70+
if (rc > 0) {
71+
if ((pfd.revents & (POLLERR | POLLNVAL)) != 0)
72+
return 1;
73+
74+
short ready_events = events;
75+
if ((events & POLLIN) != 0)
76+
ready_events |= POLLHUP;
77+
78+
return (pfd.revents & ready_events) != 0 ? 0 : 1;
79+
}
80+
81+
if (rc == 0)
82+
return 1;
83+
84+
if (errno != EINTR)
85+
return 1;
86+
}
87+
}
88+
89+
static int write_all(int fd, const char *buf, size_t len) {
90+
size_t written = 0;
91+
92+
while (written < len) {
93+
if (wait_for_fd(fd, POLLOUT) != 0)
94+
return 1;
95+
96+
ssize_t n = write(fd, buf + written, len - written);
97+
if (n > 0) {
98+
written += (size_t)n;
99+
continue;
100+
}
101+
102+
if (n < 0 && errno == EINTR)
103+
continue;
104+
52105
return 1;
106+
}
107+
108+
return 0;
109+
}
53110

111+
static int write_all_without_sigpipe(int fd, const char *buf, size_t len) {
112+
void (*previous)(int) = signal(SIGPIPE, SIG_IGN);
113+
int rc = write_all(fd, buf, len);
114+
signal(SIGPIPE, previous);
115+
return rc;
116+
}
117+
118+
static int read_http_status_line(int fd) {
54119
char buf[BUF_SIZE];
55-
int status = 0;
56-
while (fgets(buf, sizeof(buf), fp)) {
57-
if (strncmp(buf, "HTTP/", 5) == 0) {
58-
status = parse_http_status(buf);
120+
size_t used = 0;
121+
122+
while (used < sizeof(buf) - 1) {
123+
if (wait_for_fd(fd, POLLIN) != 0)
124+
return 0;
125+
126+
ssize_t n = read(fd, buf + used, sizeof(buf) - 1 - used);
127+
if (n > 0) {
128+
used += (size_t)n;
129+
buf[used] = '\0';
130+
131+
char *status_line = strstr(buf, "HTTP/");
132+
if (status_line != NULL)
133+
return parse_http_status(status_line);
134+
135+
continue;
136+
}
137+
138+
if (n == 0)
139+
break;
140+
141+
if (errno != EINTR)
59142
break;
143+
}
144+
145+
return 0;
146+
}
147+
148+
static void terminate_child(pid_t pid) {
149+
if (pid <= 0)
150+
return;
151+
152+
kill(pid, SIGKILL);
153+
waitpid(pid, NULL, 0);
154+
}
155+
156+
static int probe_https(int port) {
157+
const char *openssl_binary = find_openssl_binary();
158+
if (openssl_binary == NULL)
159+
return 1;
160+
161+
char connect_arg[32];
162+
int connect_len = snprintf(connect_arg, sizeof(connect_arg), "127.0.0.1:%d", port);
163+
if (connect_len <= 0 || connect_len >= (int)sizeof(connect_arg))
164+
return 1;
165+
166+
int stdin_pipe[2] = {-1, -1};
167+
int stdout_pipe[2] = {-1, -1};
168+
pid_t pid = -1;
169+
int status = 0;
170+
171+
if (pipe(stdin_pipe) < 0 || pipe(stdout_pipe) < 0)
172+
goto cleanup;
173+
174+
pid = fork();
175+
if (pid < 0)
176+
goto cleanup;
177+
178+
if (pid == 0) {
179+
int devnull = open("/dev/null", O_WRONLY);
180+
if (
181+
dup2(stdin_pipe[0], STDIN_FILENO) < 0 || dup2(stdout_pipe[1], STDOUT_FILENO) < 0 ||
182+
(devnull >= 0 && dup2(devnull, STDERR_FILENO) < 0)
183+
) {
184+
_exit(127);
60185
}
186+
187+
close(stdin_pipe[0]);
188+
close(stdin_pipe[1]);
189+
close(stdout_pipe[0]);
190+
close(stdout_pipe[1]);
191+
if (devnull >= 0)
192+
close(devnull);
193+
194+
char *const argv[] = {
195+
(char *)openssl_binary,
196+
"s_client",
197+
"-quiet",
198+
"-connect",
199+
connect_arg,
200+
"-servername",
201+
"localhost",
202+
NULL,
203+
};
204+
execv(openssl_binary, argv);
205+
_exit(127);
61206
}
62207

63-
pclose(fp);
208+
close(stdin_pipe[0]);
209+
stdin_pipe[0] = -1;
210+
close(stdout_pipe[1]);
211+
stdout_pipe[1] = -1;
212+
213+
const char *req = "GET /health HTTP/1.0\r\nHost: localhost\r\n\r\n";
214+
if (write_all_without_sigpipe(stdin_pipe[1], req, strlen(req)) != 0)
215+
goto cleanup;
216+
217+
close(stdin_pipe[1]);
218+
stdin_pipe[1] = -1;
219+
220+
status = read_http_status_line(stdout_pipe[0]);
221+
222+
cleanup:
223+
if (stdin_pipe[0] >= 0)
224+
close(stdin_pipe[0]);
225+
if (stdin_pipe[1] >= 0)
226+
close(stdin_pipe[1]);
227+
if (stdout_pipe[0] >= 0)
228+
close(stdout_pipe[0]);
229+
if (stdout_pipe[1] >= 0)
230+
close(stdout_pipe[1]);
231+
terminate_child(pid);
232+
64233
return (status >= 200 && status <= 299) ? 0 : 1;
65234
}
66235

0 commit comments

Comments
 (0)