{{ message }}
Fix path traversal in tag writer and read sinks via MPD song URI - #660
Open
acts-1631 wants to merge 1 commit into
Open
Fix path traversal in tag writer and read sinks via MPD song URI#660acts-1631 wants to merge 1 commit into
acts-1631 wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

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.