From 82d8bd24528921a447e47390b3f01ad3e23d70ac Mon Sep 17 00:00:00 2001 From: arfa k Date: Wed, 28 Oct 2026 20:57:11 +0530 Subject: [PATCH 1/2] Refuse '/' in Call-ID and bound recording paths in rtpp_record_ctor rtpp_record_ctor() names local recordings /= with sprintf() into the PATH_MAX+1 spath/rpath buffers. Both values come verbatim from SIP, where '/' is a legal Call-ID character, so a Call-ID like "../../tmp/x" places the recording, filled with RTP sent by the peer, outside of the recording directory. A Call-ID longer than PATH_MAX overruns spath/rpath; with PATH_MAX of 1024 (FreeBSD, macOS) the write goes past the allocation over the trailing refcount and its destructors. Refuse to record when the Call-ID or from-tag used in the file name contains '/', and build both paths with snprintf() checking for truncation, as rtpp_server_ctor() already does for prompts. Add a recording/basic sub-test for the traversal case. Validated with recording/basic (all sub-tests), recording/tos and command_parser; an 8 KB Call-ID no longer trips ASan with PATH_MAX=1024 or _FORTIFY_SOURCE=2 on glibc. --- src/rtpp_record.c | 34 +++++++++++++++++++++++------- tests/recording/basic | 2 +- tests/recording/basic.badpath.rptl | 23 ++++++++++++++++++++ 3 files changed, 50 insertions(+), 9 deletions(-) create mode 100644 tests/recording/basic.badpath.rptl diff --git a/src/rtpp_record.c b/src/rtpp_record.c index ca035e3e5..30c216e7b 100644 --- a/src/rtpp_record.c +++ b/src/rtpp_record.c @@ -171,6 +171,7 @@ rtpp_record_ctor(const struct rtpp_cfg *cfsp, const struct remote_copy_args *rap struct rtpp_record_channel *rrc; const char *sdir, *suffix1, *suffix2; int rval, remote; + size_t len; pcap_hdr_t pcap_hdr; remote = (rname != NULL && strncmp("udp:", rname, 4) == 0) ? 1 : 0; @@ -210,6 +211,13 @@ rtpp_record_ctor(const struct rtpp_cfg *cfsp, const struct remote_copy_args *rap goto e2; } + if (rname == NULL && (memchr(sp->call_id->s, '/', sp->call_id->len) != NULL || + memchr(sp->from_tag_nmn->s, '/', sp->from_tag_nmn->len) != NULL)) { + RTPP_LOG(sp->log, RTPP_LOG_ERR, "call-id or from-tag contains '/', " + "refusing to use it in the recording file name"); + goto e2; + } + if (cfsp->record_pcap != 0) { rrc->mode = MODE_LOCAL_PCAP; } else { @@ -232,19 +240,29 @@ rtpp_record_ctor(const struct rtpp_cfg *cfsp, const struct remote_copy_args *rap sdir = cfsp->sdir; rrc->needspool = 1; if (rname == NULL) { - sprintf(rrc->rpath, "%s/%.*s=%.*s%s%s", cfsp->rdir, (int)sp->call_id->len, - sp->call_id->s, (int)sp->from_tag_nmn->len, sp->from_tag_nmn->s, - suffix1, suffix2); + len = snprintf(rrc->rpath, sizeof(rrc->rpath), "%s/%.*s=%.*s%s%s", + cfsp->rdir, (int)sp->call_id->len, sp->call_id->s, + (int)sp->from_tag_nmn->len, sp->from_tag_nmn->s, suffix1, suffix2); } else { - sprintf(rrc->rpath, "%s/%s%s", cfsp->rdir, rname, suffix2); + len = snprintf(rrc->rpath, sizeof(rrc->rpath), "%s/%s%s", + cfsp->rdir, rname, suffix2); + } + if (len >= sizeof(rrc->rpath)) { + RTPP_LOG(sp->log, RTPP_LOG_ERR, "recording file name is too long"); + goto e2; } } if (rname == NULL) { - sprintf(rrc->spath, "%s/%.*s=%.*s%s%s", sdir, (int)sp->call_id->len, - sp->call_id->s, (int)sp->from_tag_nmn->len, sp->from_tag_nmn->s, - suffix1, suffix2); + len = snprintf(rrc->spath, sizeof(rrc->spath), "%s/%.*s=%.*s%s%s", + sdir, (int)sp->call_id->len, sp->call_id->s, + (int)sp->from_tag_nmn->len, sp->from_tag_nmn->s, suffix1, suffix2); } else { - sprintf(rrc->spath, "%s/%s%s", sdir, rname, suffix2); + len = snprintf(rrc->spath, sizeof(rrc->spath), "%s/%s%s", sdir, rname, + suffix2); + } + if (len >= sizeof(rrc->spath)) { + RTPP_LOG(sp->log, RTPP_LOG_ERR, "recording file name is too long"); + goto e2; } rrc->fd = open(rrc->spath, O_WRONLY | O_CREAT | O_TRUNC, DEFFILEMODE); if (rrc->fd == -1) { diff --git a/tests/recording/basic b/tests/recording/basic index bbaf50fe5..2426e7763 100755 --- a/tests/recording/basic +++ b/tests/recording/basic @@ -37,7 +37,7 @@ run_recording() { report_rc ${RPTL_RC} "running basic.${stest}.rptl RPTL script" } -SUB_TESTS="${SUB_TESTS:-"files singlefile singlefile_c stopall files_stophalf singlefile_stophalf remote remote_p"}" +SUB_TESTS="${SUB_TESTS:-"files singlefile singlefile_c stopall files_stophalf singlefile_stophalf remote remote_p badpath"}" for stest in ${SUB_TESTS} do diff --git a/tests/recording/basic.badpath.rptl b/tests/recording/basic.badpath.rptl new file mode 100644 index 000000000..8bfb3f4c6 --- /dev/null +++ b/tests/recording/basic.badpath.rptl @@ -0,0 +1,23 @@ +# Call-ID is copied from SIP, where '/' is a legal character. Recording +# (R, Rs) must be refused rather than create files outside of the +# recording directory. + +.eval: E50 -> RES_E50 +.eval: recording.%%SUBTEST%% -> WORK_BASE +.eval: ../%%WORK_BASE%%.rec. -> CALLID +.eval: %%RECORD_SPL_DIR%%/%%CALLID%%=from_tag_1 -> REC_BASE + +.exec: mkdir -p %%RECORD_SPL_DIR%% +.exec: rm -f %%REC_BASE%%.* + +.eval: -d dbug -T60 -W60 -P -r %%RECORD_SPL_DIR%% -> RTPPROXY_ARGS +socket rtpp: stdio: %%RTPPROXY_ARGS%% -> %%WORK_BASE%%.rout + +rtpp: U %%CALLID%% 127.0.0.1 12345 from_tag_1 -> validate_port(PORT_A) +rtpp: L %%CALLID%% 127.0.0.1 54321 from_tag_1 to_tag_1 -> validate_port(PORT_O) + +rtpp: R %%CALLID%% to_tag_1 from_tag_1 -> str_compare(RES_E50) +rtpp: Rs %%CALLID%% to_tag_1 from_tag_1 -> str_compare(RES_E50) + +.close: rtpp +.exec: for f in %%REC_BASE%%.*; do test ! -e "$f" || exit 1; done From 3a06fce89afb5bc3b2cedcaf8efb0cdcbe1b7f37 Mon Sep 17 00:00:00 2001 From: Maksym Sobolyev Date: Fri, 30 Oct 2026 11:04:35 -0700 Subject: [PATCH 2/2] recording/basic.badpath: exercise PATH_MAX limit on recording file names Use a Call-ID (R, Rs) and an explicit file name (C, Cs) longer than PATH_MAX, both without -S (spool path check) and with -S (final path check), and make sure a sane name still records on the same session. --- tests/recording/basic.badpath.rptl | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/recording/basic.badpath.rptl b/tests/recording/basic.badpath.rptl index 8bfb3f4c6..c7d694bc5 100644 --- a/tests/recording/basic.badpath.rptl +++ b/tests/recording/basic.badpath.rptl @@ -1,8 +1,14 @@ # Call-ID is copied from SIP, where '/' is a legal character. Recording # (R, Rs) must be refused rather than create files outside of the # recording directory. +# +# Recording file name built from a Call-ID (R, Rs) or given explicitly +# (C, Cs) that does not fit into PATH_MAX must be refused as well, both +# for the spool (no -S) and for the final (-S) path. +.eval: 0 -> RES_OK .eval: E50 -> RES_E50 +.eval: E65 -> RES_E65 .eval: recording.%%SUBTEST%% -> WORK_BASE .eval: ../%%WORK_BASE%%.rec. -> CALLID .eval: %%RECORD_SPL_DIR%%/%%CALLID%%=from_tag_1 -> REC_BASE @@ -10,6 +16,12 @@ .exec: mkdir -p %%RECORD_SPL_DIR%% .exec: rm -f %%REC_BASE%%.* +# Longer than PATH_MAX on any supported platform, yet short enough for +# the commands carrying it to fit into the RTPP_CMD_BUFLEN. +.exec: printf '%4200s' '' | tr ' ' x -> LONG_NAME +.eval: %%WORK_BASE%%.long.%%LONG_NAME%% -> CALLID_LONG +.eval: %%WORK_BASE%%.short -> CALLID_SHORT + .eval: -d dbug -T60 -W60 -P -r %%RECORD_SPL_DIR%% -> RTPPROXY_ARGS socket rtpp: stdio: %%RTPPROXY_ARGS%% -> %%WORK_BASE%%.rout @@ -19,5 +31,35 @@ rtpp: L %%CALLID%% 127.0.0.1 54321 from_tag_1 to_tag_1 -> validate_port(PORT_O) rtpp: R %%CALLID%% to_tag_1 from_tag_1 -> str_compare(RES_E50) rtpp: Rs %%CALLID%% to_tag_1 from_tag_1 -> str_compare(RES_E50) +rtpp: U %%CALLID_LONG%% 127.0.0.1 12345 from_tag_1 -> validate_port(_) +rtpp: L %%CALLID_LONG%% 127.0.0.1 54321 from_tag_1 to_tag_1 -> validate_port(_) +rtpp: R %%CALLID_LONG%% to_tag_1 from_tag_1 -> str_compare(RES_E50) +rtpp: Rs %%CALLID_LONG%% to_tag_1 from_tag_1 -> str_compare(RES_E50) + +rtpp: U %%CALLID_SHORT%% 127.0.0.1 12345 from_tag_1 -> validate_port(_) +rtpp: L %%CALLID_SHORT%% 127.0.0.1 54321 from_tag_1 to_tag_1 -> validate_port(_) +rtpp: C %%CALLID_SHORT%% %%LONG_NAME%% to_tag_1 from_tag_1 -> str_compare(RES_E65) +rtpp: Cs %%CALLID_SHORT%% %%LONG_NAME%% to_tag_1 from_tag_1 -> str_compare(RES_E65) + .close: rtpp .exec: for f in %%REC_BASE%%.*; do test ! -e "$f" || exit 1; done + +.eval: -d dbug -T60 -W60 -P -r %%RECORD_DIR%% -S %%RECORD_SPL_DIR%% -> RTPPROXY_ARGS +socket rtpp: stdio: %%RTPPROXY_ARGS%% -> %%WORK_BASE%%.S.rout + +rtpp: U %%CALLID_LONG%% 127.0.0.1 12345 from_tag_1 -> validate_port(_) +rtpp: L %%CALLID_LONG%% 127.0.0.1 54321 from_tag_1 to_tag_1 -> validate_port(_) +rtpp: R %%CALLID_LONG%% to_tag_1 from_tag_1 -> str_compare(RES_E50) +rtpp: Rs %%CALLID_LONG%% to_tag_1 from_tag_1 -> str_compare(RES_E50) + +rtpp: U %%CALLID_SHORT%% 127.0.0.1 12345 from_tag_1 -> validate_port(_) +rtpp: L %%CALLID_SHORT%% 127.0.0.1 54321 from_tag_1 to_tag_1 -> validate_port(_) +rtpp: C %%CALLID_SHORT%% %%LONG_NAME%% to_tag_1 from_tag_1 -> str_compare(RES_E65) +rtpp: Cs %%CALLID_SHORT%% %%LONG_NAME%% to_tag_1 from_tag_1 -> str_compare(RES_E65) + +# Sanity check: the same session still records fine with a sane name. +rtpp: Cs %%CALLID_SHORT%% %%WORK_BASE%%.ok.pcap to_tag_1 from_tag_1 -> str_compare(RES_OK) + +.close: rtpp +.exec: test -e %%RECORD_DIR%%/%%WORK_BASE%%.ok.pcap +.exec: rm -f %%RECORD_DIR%%/%%WORK_BASE%%.ok.pcap