jcs
/subtext
/amendments
/630
binkp: Validate filename, more safety checks
jcs made amendment 630 4 days ago
--- binkp.c Sat Dec 6 15:10:18 2025
+++ binkp.c Fri Sep 18 15:13:24 2026
@@ -56,6 +56,7 @@ bool binkp_zip_decider(char *filename, size_t size);
void binkp_fidopkt_processor(char *filename, unsigned char *data,
size_t size);
ssize_t binkp_buffer_file(Str255 path, char **data);
+bool binkp_valid_filename(char *filename);
void binkp_toss_inbox(void);
void binkp_deliver_outbox(void);
@@ -157,11 +158,12 @@ binkp_poll(void)
elapsed == 1 ? "" : "s");
done:
- if (!binkpc || binkpc->error)
- binkp_last_poll_error = true;
+ if (binkpc != NULL) {
+ if (binkpc->error)
+ binkp_last_poll_error = true;
- if (binkpc != NULL)
binkp_free();
+ }
binkp_next_poll = Time + db->config.binkp_interval_seconds;
@@ -359,7 +361,7 @@ binkp_read_frame(void)
{
char tmp[128];
size_t len, off, frame_data_read;
- unsigned short rlen;
+ unsigned short rlen, slen;
short error;
Ptr read_dest;
@@ -469,8 +471,21 @@ binkp_read_frame(void)
rlen, frame_data_read, binkpc->cur_frame.data_size);
#endif
- if (binkpc->cur_incoming_file.frefnum == 0)
- panic("binkpc: no frefnum for data, bogus state");
+ if (binkpc->cur_incoming_file.frefnum == 0) {
+ logger_printf("[binkp] Received data frame before "
+ "M_FILE");
+ goto failed_read;
+ }
+
+ if (binkpc->cur_incoming_file.data_read >
+ binkpc->cur_incoming_file.size) {
+ logger_printf("[binkp] Received %ld bytes for %s but "
+ "size is %ld", binkpc->cur_incoming_file.data_read,
+ binkpc->cur_incoming_file.filename,
+ binkpc->cur_incoming_file.size);
+ goto failed_read;
+ }
+
len = rlen;
error = FSWrite(binkpc->cur_incoming_file.frefnum, &len,
binkpc->buf);
@@ -525,10 +540,22 @@ binkp_read_frame(void)
}
}
- if (sscanf(binkpc->buf + 1, "%128s %lu %lu %lu",
- &binkpc->cur_incoming_file.filename,
+ if (sscanf(binkpc->buf + 1, "%64s %lu %lu %lu%n",
+ binkpc->cur_incoming_file.filename,
&binkpc->cur_incoming_file.size,
- &binkpc->cur_incoming_file.mtime, &off) == 4) {
+ &binkpc->cur_incoming_file.mtime, &off, &slen) == 4) {
+ if (!binkp_valid_filename(
+ binkpc->cur_incoming_file.filename)) {
+ logger_printf("[binkp] Refusing M_FILE with bad "
+ "filename \"%s\"",
+ binkpc->cur_incoming_file.filename);
+ binkp_send_frame(BINKP_COMMAND_M_SKIP,
+ binkpc->buf + 1, slen);
+ binkpc->cur_incoming_file.filename[0] = '\0';
+ /* not an error */
+ return true;
+ }
+
logger_printf("[binkp] Receiving file \"%s\" size %ld",
binkpc->cur_incoming_file.filename,
binkpc->cur_incoming_file.size);
@@ -615,6 +642,10 @@ binkp_read_frame(void)
binkpc->cur_incoming_file.filename,
binkpc->cur_incoming_file.size,
binkpc->cur_incoming_file.mtime);
+ if (len >= sizeof(tmp)) {
+ logger_printf("[binkp] M_GOT snprintf overflow");
+ goto failed_read;
+ }
if (!binkp_send_frame(BINKP_COMMAND_M_GOT, tmp, len))
logger_printf("[binkp] Failed sending M_GOT %s",
binkpc->cur_incoming_file.filename);
@@ -649,19 +680,35 @@ binkp_login(void)
return false;
len = snprintf(command, sizeof(command), "SYS %s", db->config.name);
+ if (len >= sizeof(command)) {
+ logger_printf("[binkp] SYS snprintf overflow");
+ return false;
+ }
if (!binkp_send_frame(BINKP_COMMAND_M_NUL, command, len))
return false;
len = snprintf(command, sizeof(command), "LOC %s", db->config.location);
+ if (len >= sizeof(command)) {
+ logger_printf("[binkp] LOC snprintf overflow");
+ return false;
+ }
if (!binkp_send_frame(BINKP_COMMAND_M_NUL, command, len))
return false;
len = snprintf(command, sizeof(command), "NDL 14400,TCP,BINKP");
+ if (len >= sizeof(command)) {
+ logger_printf("[binkp] NDL snprintf overflow");
+ return false;
+ }
if (!binkp_send_frame(BINKP_COMMAND_M_NUL, command, len))
return false;
len = snprintf(command, sizeof(command), "VER Subtext/%s binkp/1.0",
get_version(false));
+ if (len >= sizeof(command)) {
+ logger_printf("[binkp] VER snprintf overflow");
+ return false;
+ }
if (!binkp_send_frame(BINKP_COMMAND_M_NUL, command, len))
return false;
@@ -866,7 +913,7 @@ binkp_zip_decider(char *filename, size_t size)
{
size_t flen = strlen(filename);
- if (strcmp(filename + flen - 4, ".pkt") == 0)
+ if (flen >= 4 && strcmp(filename + flen - 4, ".pkt") == 0)
return true;
return false;
@@ -976,6 +1023,31 @@ binkp_buffer_file(Str255 path, char **data)
return count;
}
+bool
+binkp_valid_filename(char *filename)
+{
+ size_t len, n;
+ unsigned char c;
+
+ len = strlen(filename);
+ if (!len)
+ return false;
+
+ for (n = 0; n < len; n++) {
+ c = filename[n];
+
+ if (n == 0 && c == '.')
+ return false;
+
+ if (!((c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') ||
+ (c >= 'a' && c <= 'z') || c == '_' || c == '-' || c == '+' ||
+ (c == '.')))
+ return false;
+ }
+
+ return true;
+}
+
void
binkp_deliver_outbox(void)
{
@@ -1038,6 +1110,10 @@ binkp_deliver_outbox(void)
len = snprintf(command, sizeof(command), "%s %lu %lu 0",
file_name_c, sb.st_size, MAC_TO_UNIX_TIME(sb.st_ctime));
+ if (len >= sizeof(command)) {
+ logger_printf("[binkp] M_FILE snprintf overflow");
+ goto done;
+ }
if (!binkp_send_frame(BINKP_COMMAND_M_FILE, command, len))
goto done;