Repository navigation
Refuse '/' in Call-ID and bound recording paths in rtpp_record_ctor - #179
Conversation
|
@arfa-21 thanks, that looks ok. In general we discourage recording without specifying the file name, that API is considered legacy for obvious security reasons. It is therefore recommended to approach whoever the maintainer of your client is and ask them to use version that takes recording name explicitly, which could easily be provided by passing call-id through md5() or something. |
|
Makes sense. C with a name the client derives itself, md5 of the Call-ID or similar, avoids all of this since nothing from SIP ends up in the path, and that's what clients should be using. R and -a have no way to pass a name, so they still land on the Call-ID path in rtpp_record_ctor(), and this keeps them inside -r until clients move over. |
f592f83 to
04c22a9
Compare
rtpp_record_ctor() names local recordings <rdir>/<call-id>=<from-tag> 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.
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.
04c22a9 to
3a06fce
Compare
|
@arfa-21 changes are merged, thanks for contribution. I've added targeted test to excercise new '/' and PATH_MAX failure modes specifically. |
rtpp_record_ctor() names local recordings
<rdir>/<call-id>=<from-tag><suffix>and builds them with sprintf() into the PATH_MAX+1spath/rpathbuffers. The Call-ID and from-tag arrive verbatim from SIP through the controlling proxy, and/is a legal Call-ID character, so this is a plain path traversal: with-r /tmp/rec -a, the commandU ../outside/x 127.0.0.1 10000 ftagcreates/tmp/outside/x=ftag.{o,a}.{rtp,rtcp}, opened with O_TRUNC and filled with whatever RTP the peer sends (R and Rs on an existing session do the same). The same lines also overrun on a long Call-ID: a 6000-byte one aborts a_FORTIFY_SOURCE=2glibc build with "buffer overflow detected", and with a PATH_MAX of 1024 (FreeBSD, macOS) an ~8 KB one writes past the end of the allocation, over the refcount and destructor pointers that rtpp_rzmalloc() places right after the struct (reproduced under ASan by building rtpp_record.c with PATH_MAX=1024 on Linux: "WRITE of size 8133" into a 6848-byte region). Such Call-IDs could never be recorded correctly anyway, since the/turns into a directory separator, so the fix refuses to record when the Call-ID or from-tag used in the name contains/, and switches the four sprintf() calls to snprintf() with a truncation check, the same way rtpp_server_ctor() already builds prompt paths. Names given explicitly with the C command are left alone, since the proxy chooses those. I added abadpathsub-test to recording/basic that fails on master (R returns 0 and the files land outside-r) and passes with the fix; recording/basic, recording/tos and command_parser pass locally.