Fix path traversal in tag writer and read sinks via MPD song URI by acts-1631 · Pull Request #660 · ncmpcpp/ncmpcpp · GitHub
Skip to content

Fix path traversal in tag writer and read sinks via MPD song URI - #660

Open
acts-1631 wants to merge 1 commit into
ncmpcpp:masterfrom
acts-1631:fix/tag-writer-traversal
Open

Fix path traversal in tag writer and read sinks via MPD song URI#660
acts-1631 wants to merge 1 commit into
ncmpcpp:masterfrom
acts-1631:fix/tag-writer-traversal

Conversation

@acts-1631

Copy link
Copy Markdown

Tags::write built the on-disk path by concatenating mpd_music_dir with
the raw MPD song URI without filtering '..' segments, and
EditLibraryAlbum did the same with a direct TagLib::FileRef.
EditDirectoryName joined MPD-supplied directory paths with mpd_music_dir
the same way. A non-stream song whose URI contains '..' (served by a
remote or compromised MPD server) therefore resolved outside
mpd_music_dir: f.save() rewrote tags on an existing audio file at an
attacker-chosen path and, when the song was renamed,
boost::filesystem::rename moved the file.

The same mpd_music_dir + getURI pattern also appeared in read sinks:
TagsLyricsFetcher, the song info screen, and the tiny tag editor all
opened an arbitrary existing audio file at a traversed path with
TagLib::FileRef.

Add a shared normalizePath helper that collapses '.' and '..' segments
(relative paths cannot escape above their root, absolute paths keep
their leading slash) and apply it to every MPD-supplied URI/directory
before joining with mpd_music_dir. Legitimate relative URIs and
absolute local paths are unchanged; only traversal segments are removed.

Tags::write built the on-disk path by concatenating mpd_music_dir with
the raw MPD song URI without filtering '..' segments, and
EditLibraryAlbum did the same with a direct TagLib::FileRef.
EditDirectoryName joined MPD-supplied directory paths with mpd_music_dir
the same way. A non-stream song whose URI contains '..' (served by a
remote or compromised MPD server) therefore resolved outside
mpd_music_dir: f.save() rewrote tags on an existing audio file at an
attacker-chosen path and, when the song was renamed,
boost::filesystem::rename moved the file.

The same mpd_music_dir + getURI pattern also appeared in read sinks:
TagsLyricsFetcher, the song info screen, and the tiny tag editor all
opened an arbitrary existing audio file at a traversed path with
TagLib::FileRef.

Add a shared normalizePath helper that collapses '.' and '..' segments
(relative paths cannot escape above their root, absolute paths keep
their leading slash) and apply it to every MPD-supplied URI/directory
before joining with mpd_music_dir. Legitimate relative URIs and
absolute local paths are unchanged; only traversal segments are removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant