Hi BusyBox team,
I recently sent you an email detailing the HTTP header-injection
vulnerability with PoCs;here are the minimal patches. Additionally, this
submission fixes an unrelated inline TODO in the FTP code (not included in
my previous email): it implements proper handling on the FTP control path.
Summary
wget: HTTP header injection via request-target (CR/LF in path)
Before: If the URL path contains actual CR/LF, the request line is split
and the remainder is interpreted as forged headers.
After: Such URLs are rejected up front with bad URL; no forged headers are
sent.
wget: FTP control-path hardening for SIZE / RETR
Before: Path bytes were sent raw on the control channel; 0xff (Telnet IAC)
could be misinterpreted, and the source contained a TODO about escaping.
After (this patch): Implement the TODO by duplicating 0xff (IAC) for
SIZE/RETR so IAC is never emitted as data.
vi: sanitize names/patterns on the status line and in error messages
Before: File names and user-supplied strings could carry terminal control
sequences (e.g., ANSI escape codes) or other non-printable bytes that would
be written to the status line verbatim.
After: Use the existing print_literal() consistently before printing those
strings (no functional change otherwise).
What the patches do, concretely
wget (shared outbound validation + FTP IAC rule)
Add die_on_bad_http_bytes(const char *s, int reject_space, const char
*what) and call it before protocol branching:
Rejects CR (0x0D), LF (0x0A), all C0 controls (<0x20), and DEL (0x7F).
For the request-target (path/query) only, also rejects space (0x20) to
preserve the HTTP/1.1 request-line shape (METHOD SP request-target SP
HTTP/1.1).
Add ftp_sanitize_path(const char *s) and apply it to SIZE and RETR:
Duplicate 0xff (Telnet IAC) on the FTP control connection (implements the
in-code TODO).
vi
Route file names/patterns through print_literal() in status/errors
(format_edit_status(), status_line_bold_errno(), file_insert() diagnostics,
regex error in char_search(), and the :write/:edit/:read/:file outputs).
Signed-off-by: Takeuchi Yuma
---
diff --git a/networking/wget.c b/networking/wget.c
index ec3767793..0c3d72a66 100644
--- a/networking/wget.c
+++ b/networking/wget.c
@@ -474,6 +474,40 @@ static char* sanitize_string(char *s)
return s;
}
+/* Unified validator:
+ * Reject CR/LF and other control characters. Optionally reject SP as well.
+ * Use reject_space=1 for the HTTP request-target (path/query), because a
space
+ * would split the request line and enable header injection.
+ * Keep this minimal to avoid code size growth (no %-encoding here).
+ */
+static void die_on_bad_http_bytes(const char *s, int reject_space, const
char *what)
+{
+ const unsigned char *p = (const unsigned char*)s;
+ while (*p) {
+ unsigned char c = *p++;
+ if (c == '\r' || c == '\n' || c < 0x20 || c == 0x7f ||
(reject_space && c == ' '))
+ bb_error_msg_and_die("bad %s", what);
+ }
+}
+
+#if ENABLE_FEATURE_WGET_FTP
+/* FTP path sanitizer:
+ * - Disallow CR/LF (would terminate the control command)
+ * - Double 0xff (Telnet IAC) per RFC/telnet rules
+ * Minimal approach: reject CR/LF, duplicate IAC, no other transformations.
+ */
+static char *ftp_sanitize_path(const char *s)
+{
+ const unsigned char *p = (const unsigned char*)s;
+ size_t len = 0, iac = 0;
+ for (; *p; p++) { if (*p == 0xff) iac++; len++; }
+ if (!iac) return xstrdup(s);
+ char *out = xmalloc(len + iac + 1), *q = out;
+ for (p = (const unsigned char*)s; *p; p++) { *q++ = *p; if (*p ==
0xff) *q++ = 0xff; }
+ *q = '\0'; return out;
+}
+#endif
+
/* Returns '\n' if it was seen, else '\0'. Trims at first '\r' or '\n' */
static char fgets_trim_sanitize(FILE *fp, const char *fmt)
{
@@ -862,12 +896,16 @@ static FILE* prepare_ftp_session(FILE **dfpp, struct
host_info *target, len_and_
ftpcmd("TYPE I", NULL, sfp);
/* Query file size */
- if (ftpcmd("SIZE ", target->path, sfp) == 213) {
- G.content_len = BB_STRTOOFF(G.wget_buf + 4, NULL, 10);
- if (G.content_len < 0 || errno) {
- bb_error_msg_and_die("bad SIZE value '%s'",
G.wget_buf + 4);
+ {
+ char *ftp_path = ftp_sanitize_path(target->path);
+ int code = ftpcmd("SIZE ", ftp_path, sfp);
+ free(ftp_path);
+ if (code == 213) {
+ G.content_len = BB_STRTOOFF(G.wget_buf + 4, NULL,
10);
+ if (G.content_len < 0 || errno)
+ bb_error_msg_and_die("bad SIZE value '%s'",
G.wget_buf + 4);
+ G.got_clen = 1;
}
- G.got_clen = 1;
}
/* Enter passive mode */
@@ -904,11 +942,14 @@ static FILE* prepare_ftp_session(FILE **dfpp, struct
host_info *target, len_and_
reset_beg_range_to_zero();
}
-//TODO: needs ftp-escaping 0xff and '\n' bytes here.
-//Or disallow '\n' altogether via sanitize_string() in parse_url().
-//But 0xff's are possible in valid utf8 filenames.
- if (ftpcmd("RETR ", target->path, sfp) > 150)
- bb_error_msg_and_die("bad response to %s: %s", "RETR",
G.wget_buf);
+ /* Apply FTP sanitization when issuing RETR to prevent command
injection */
+ {
+ char *ftp_path = ftp_sanitize_path(target->path);
+ int code = ftpcmd("RETR ", ftp_path, sfp);
+ free(ftp_path);
+ if (code > 150)
+ bb_error_msg_and_die("bad response to %s: %s",
"RETR", G.wget_buf);
+ }
return sfp;
}
@@ -1177,6 +1218,12 @@ static void download_one_url(const char *url)
establish_session:
/*G.content_len = 0; - redundant, got_clen = 0 is enough */
G.got_clen = 0;
+ /* Prevent request-line/header injection.
+ * For request-target, also reject space characters.
+ */
+ die_on_bad_http_bytes(target.path, 1, "URL");
+ die_on_bad_http_bytes(target.host, 0, "Host");
+
G.chunked = 0;
if (!ENABLE_FEATURE_WGET_FTP
|| use_proxy || target.protocol[0] != 'f' /*not ftp[s]*/
Signed-off-by: Takeuchi Yuma
---
diff --git a/editors/vi.c b/editors/vi.c
index 34932f60c..0e941d924 100644
--- a/editors/vi.c
+++ b/editors/vi.c
@@ -556,6 +556,7 @@ static int crashme = 0;
static void show_status_line(void); // put a message on the bottom line
static void status_line_bold(const char *, ...);
+static void print_literal(char *buf, const char *s);
static void show_help(void)
{
@@ -1291,19 +1292,29 @@ static int format_edit_status(void)
trunc_at = columns < STATUS_BUFFER_LEN-1 ?
columns : STATUS_BUFFER_LEN-1;
- ret = snprintf(status_buffer, trunc_at+1,
+ {
+ const char *fname_disp;
+ char fn_buf[MAX_INPUT_LEN];
+ if (current_filename) {
+ print_literal(fn_buf, current_filename);
+ fname_disp = fn_buf;
+ } else {
+ fname_disp = "No file";
+ }
+ ret = snprintf(status_buffer, trunc_at+1,
#if ENABLE_FEATURE_VI_READONLY
- "%c %s%s%s %d/%d %d%%",
+ "%c %s%s%s %d/%d %d%%",
#else
- "%c %s%s %d/%d %d%%",
+ "%c %s%s %d/%d %d%%",
#endif
- cmd_mode_indicator[cmd_mode & 3],
- (current_filename != NULL ? current_filename : "No file"),
+ cmd_mode_indicator[cmd_mode & 3],
+ fname_disp,
#if ENABLE_FEATURE_VI_READONLY
- (readonly_mode ? " [Readonly]" : ""),
+ (readonly_mode ? " [Readonly]" : ""),
#endif
- (modified_count ? " [Modified]" : ""),
- cur, tot, percent);
+ (modified_count ? " [Modified]" : ""),
+ cur, tot, percent);
+ }
if (ret >= 0 && ret < trunc_at)
return ret; // it all fit
@@ -1376,7 +1387,13 @@ static void status_line_bold(const char *format, ...)
}
static void status_line_bold_errno(const char *fn)
{
- status_line_bold("'%s' "STRERROR_FMT, fn STRERROR_ERRNO);
+ char fnbuf[MAX_INPUT_LEN];
+ if (!fn) {
+ fnbuf[0] = '\0';
+ } else {
+ print_literal(fnbuf, fn);
+ }
+ status_line_bold("'%s' "STRERROR_FMT, fnbuf STRERROR_ERRNO);
}
// copy s to buf, convert unprintable
@@ -2003,7 +2020,9 @@ static int file_insert(const char *fn, char *p, int
initial)
goto fi;
}
if (!S_ISREG(statbuf.st_mode)) {
- status_line_bold("'%s' is not a regular file", fn);
+ char fnbuf[MAX_INPUT_LEN];
+ print_literal(fnbuf, fn);
+ status_line_bold("'%s' is not a regular file", fnbuf);
goto fi;
}
size = (statbuf.st_size < INT_MAX ? (int)statbuf.st_size : INT_MAX);
@@ -2015,7 +2034,12 @@ static int file_insert(const char *fn, char *p, int
initial)
} else if (cnt < size) {
// There was a partial read, shrink unused space
p = text_hole_delete(p + cnt, p + size - 1, NO_UNDO);
- status_line_bold("can't read '%s'", fn);
+ {
+ char fnbuf[MAX_INPUT_LEN];
+ print_literal(fnbuf, fn);
+ status_line_bold("can't read '%s'",
+ fnbuf);
+ }
}
# if ENABLE_FEATURE_VI_UNDO
else {
@@ -2401,7 +2425,9 @@ static char *char_search(char *p, const char *pat,
int dir_and_range)
preg.not_bol = p != text;
preg.not_eol = p != end - 1;
if (err != NULL) {
- status_line_bold("bad search pattern '%s': %s", pat, err);
+ char pbuf[MAX_INPUT_LEN];
+ print_literal(pbuf, pat);
+ status_line_bold("bad search pattern '%s': %s", pbuf, err);
return p;
}
@@ -2824,10 +2850,16 @@ static void colon(char *buf)
} else {
modified_count = 0;
last_modified_count = -1;
- status_line("'%s' %uL, %uC",
- current_filename,
+ {
+ char fnbuf[MAX_INPUT_LEN];
+ const char *disp = current_filename
+ ? (print_literal(fnbuf,
current_filename), fnbuf)
+ : "No file";
+ status_line("'%s' %uL, %uC",
+ disp,
count_lines(text, end - 1), cnt
- );
+ );
+ }
if (p[0] == 'x'
|| p[1] == 'q' || p[1] == 'n'
) {
@@ -2977,6 +3009,7 @@ static void colon(char *buf)
dot_skip_over_ws();
} else if (strncmp(cmd, "edit", i) == 0) { // Edit a file
int size;
+ char fnbuf[MAX_INPUT_LEN];
// don't edit, if the current file has been modified
if (modified_count && !useforce) {
@@ -3008,16 +3041,21 @@ static void colon(char *buf)
# endif
// how many lines in text[]?
li = count_lines(text, end - 1);
- status_line("'%s'%s"
+ {
+ const char *disp = args[0]
+ ? (print_literal(fnbuf, fn), fnbuf)
+ : (current_filename ? (print_literal(fnbuf,
current_filename), fnbuf) : "No file");
+ status_line("'%s'%s"
IF_FEATURE_VI_READONLY("%s")
" %uL, %uC",
- fn,
+ disp,
(size < 0 ? " [New file]" : ""),
IF_FEATURE_VI_READONLY(
((readonly_mode) ? " [Readonly]" : ""),
)
li, (int)(end - text)
- );
+ );
+ }
} else if (strncmp(cmd, "file", i) == 0) { // what File is this
if (e >= 0) {
status_line_bold("No address allowed on this
command");
@@ -3109,6 +3147,7 @@ static void colon(char *buf)
editing = 0;
} else if (strncmp(cmd, "read", i) == 0) { // read file into
text[]
int size, num;
+ char fnbuf[MAX_INPUT_LEN];
if (args[0]) {
// the user supplied a file name
@@ -3141,13 +3180,18 @@ static void colon(char *buf)
goto ret; // nothing was inserted
// how many lines in text[]?
li = count_lines(q, q + size - 1);
- status_line("'%s'"
+ {
+ const char *disp = args[0]
+ ? (print_literal(fnbuf, fn), fnbuf)
+ : (current_filename ? (print_literal(fnbuf,
current_filename), fnbuf) : "No file");
+ status_line("'%s'"
IF_FEATURE_VI_READONLY("%s")
" %uL, %uC",
- fn,
+ disp,
IF_FEATURE_VI_READONLY((readonly_mode ? "
[Readonly]" : ""),)
li, size
- );
+ );
+ }
dot = find_line(num);
} else if (strncmp(cmd, "rewind", i) == 0) { // rewind cmd line
args
if (modified_count && !useforce) {
@@ -3365,7 +3409,10 @@ static void colon(char *buf)
}
# if ENABLE_FEATURE_VI_READONLY
else if (readonly_mode && !useforce && fn) {
- status_line_bold("'%s' is read only", fn);
+ char rb[MAX_INPUT_LEN];
+ print_literal(rb, fn);
+ status_line_bold("'%s' is read only",
+ rb);
goto ret;
}
# endif
@@ -3394,7 +3441,13 @@ static void colon(char *buf)
} else {
// how many lines written
li = count_lines(q, q + l - 1);
- status_line("'%s' %uL, %uC", fn, li, l);
+ {
+ char fnbuf[MAX_INPUT_LEN];
+ const char *disp = fn
+ ? (print_literal(fnbuf, fn), fnbuf)
+ : (current_filename ?
(print_literal(fnbuf, current_filename), fnbuf) : "No file");
+ status_line("'%s' %uL, %uC", disp, li, l);
+ }
if (l == size) {
if (q == text && q + l == end) {
modified_count = 0;
_______________________________________________
busybox mailing list
[email protected]
https://lists.busybox.net/mailman/listinfo/busybox
