smbclient: resolve server-provided file names in recursive get

[musthaf-bughunter]

Description

Path traversal in smbclient.get -r. In _getr() the file names returned by the server are joined to the local destination without being resolved, so the root not in local.parents check runs on the lexical path and a directory entry named ../evil passes it (PurePath.parents does not collapse ..). A hostile server can make the client write files, or create directories, outside of the directory it downloads into.

Repro: a server whose listing contains ../evil, see

scapy/scapy/layers/smbclient.py

Lines 1745 to 1750 in aa87edd

	# Discard . and ..
	continue
	remote = directory / x[0]
	local = dest / x[0]
	# Word of caution: this check ONLY works because root and local have been
	# resolve(). Be careful

. The test added in smbclientserver.uts is the minimal version of it and does not need samba.

Fix: resolve the local path before the containment check, and the root that get() hands to _getr(), as the comment next to the check already requires.

[gpotter2]

I don't think that this work considering parsepath strips any potential /. Do you have a reproducible against a real "fake" SMB server?

[musthaf-bughunter]

I think parsepath only sees the argument the user gives to get. The names that _getr joins to the local destination come straight out of query_directory (the FileName of each FILE_ID_BOTH_DIR_INFORMATION entry), nothing strips them, and the .. filter only drops the literal "." and ".." entries.

I've replaced the mocked test with one against a real server: a SMB_Server subclass whose listing contains a file named ../evil, with the stock smbclient doing get -r . over the loopback (https://github.com/musthaf-bughunter/scapy/blob/6a1a8ba535ae45a2cddf9cc4e3f34e2798c87d29/test/scapy/layers/smbclientserver.uts#L240-L297). Without the fix, evil is written one level above the download directory with the content the server serves for it; with the fix, get raises FileNotFoundError and nothing is written. It is the same resolve-then-parents check that lookup_file already does on the server side.