Security: fix authenticated path traversal (arbitrary file write) in Share and Notification plugins#2163
Open
jessedegans wants to merge 2 commits into
Open
Security: fix authenticated path traversal (arbitrary file write) in Share and Notification plugins#2163jessedegans wants to merge 2 commits into
jessedegans wants to merge 2 commits into
Conversation
The Share plugin built the save path directly from packet.body.filename without sanitization, so a paired device could send '../' or an absolute path and write outside the download directory (made worse by the 'open' flag, which auto-launches the received file). Reduce the filename to a single basename before building the destination path.
_downloadIcon used packet.body.payloadHash directly as the cache filename, the same untrusted-filename-as-path issue as the Share plugin. Collapse it to a basename so a crafted payloadHash can't traverse out of the cache dir.
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.
I think there's a path traversal in the Share plugin's file receive, current as of v72, and the same pattern in the Notification plugin's icon cache.
When a file comes in,
_getFile()insrc/service/plugins/share.jsbuilds the save path directly from the remote-supplied filename:filenameispacket.body.filenamefrom the incomingkdeconnect.share.request, andbuild_filenamevdoesn't normalize.., so a paired device can send something likefilename: "../../.config/autostart/x.desktop"and the file gets written outside the download directory. It's made worse bypacket.body.open(around line 220), which auto-launches the received file, so a malicious or compromised paired device can drop a file at an arbitrary path and have it opened automatically.The same issue occurs in
notification.js_downloadIcon, which usespacket.body.payloadHashdirectly as the cache filename (around line 471), so a craftedpayloadHashcan traverse out of the cache dir.It does require a paired device, so it isn't unauthenticated, but pairing is meant to gate "can send me files", not "can write anywhere in my home directory and add autostart entries".
Repro:
filenameset to../evil(or an autostart.desktoppath) plus a payload.~/Downloads.Affected: v72, and as far as I can tell every prior version, since the logic has been the same.
This PR reduces both names to their basename before joining, which blocks
..and absolute paths.