FTP: refuse to delete, rename or overwrite the mounted image - #236
Closed
iTechMedic wants to merge 1 commit into
Closed
FTP: refuse to delete, rename or overwrite the mounted image#236iTechMedic wants to merge 1 commit into
iTechMedic wants to merge 1 commit into
Conversation
The web UI has always refused to delete the image the host has mounted
(deleteapi.cpp:43, "cannot delete the mounted image - mount another one
first"). FTP had no such guard, so the same card could be broken over FTP in a
way the browser would not allow - and a user reported the device locking up
after the image file stopped existing.
Three FTP paths could do it:
DELE f_unlink on the mounted image, leaving the gadget reading storage with
no directory entry while the host keeps issuing READ(10).
RNTO f_rename moves it out from under the mount just as effectively, and
renaming something else ONTO it destroys it.
STOR opens FA_CREATE_ALWAYS, which truncates. Uploading over the mounted
image is the worst of the three: the directory entry survives, so
nothing looks wrong until a read comes back short.
The comparison is the whole difficulty here, and it is not obvious. The same
file reaches the guard spelled three different ways:
"1:/x.cue" what SCSITBService::GetCurrentCDPath() always reports.
"1://x.cue" RealPath() formats "%s/%s" onto m_CurrentPath, and m_CurrentPath
is "1:/" for the whole session whenever the worker auto-enters
the images partition on connect - which it does for every client
that does not CWD somewhere first.
"1:x.cue" FTPPathToFatFsPath() consumes the separator after the volume
when it converts an absolute FTP path, and never restores it.
FatFs resolves all three to the same file - a drive-relative path is taken
against that volume's current directory, which is its root - so every spelling
works for f_open/f_unlink/f_rename and only a string comparison can tell them
apart. A guard that compares them naively is silently inert for the most
ordinary case there is, and looks like it works the moment the client happens
to CWD first.
So paths are normalized before comparing: separators collapsed, trailing one
dropped, and one guaranteed after the volume colon. Case-insensitively, because
FAT is.
That comparison lives in fatfspath.h rather than in the worker because
ftpworker.cpp cannot be compiled into the host suite - it needs the socket
stack - and a guard nothing can test is a guard that quietly stops working.
test_ftppaths.cpp asserts every spelling the worker actually produces, and the
negative cases too: a prefix, a deeper path, or a same-named file on 0: must
NOT match, because a guard that over-refuses is its own bug.
Two smaller things in the same code:
Log the refusal before sending it. SendStatus() formats the reply into
m_CommandBuffer (ftpworker.cpp:349) and pArgs points into that same buffer, so
replying first overwrites the argument being logged - the line prints the reply
back to itself, offset by the five bytes of "553 T".
Delete and RenameTo looked the service up by task name and dereferenced the
result without checking it, on a path that already tolerates its absence
elsewhere. Both now go through RefreshImageCache().
This does not address a mounted image that goes missing by other means - a card
pulled, or an image deleted before this build. That is the separate
missing-image-at-boot behaviour.
Collaborator
Author
|
Superseded by #238, which combines this with the rest of the batch into a single branch. Same commits, no changes dropped. Closing this one. |
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.
One commit. The web UI has always refused to delete the image the host has mounted. FTP had no such guard, so the same card could be broken over FTP in a way the browser would not allow, and a user reported the device locking up after the image file stopped existing.
Three FTP paths could do it:
DELEunlinks the mounted image, leaving the gadget reading storage with no directory entry while the host keeps issuing READ(10).RNTOmoves it out from under the mount just as effectively, and renaming something else onto it destroys it.STORopensFA_CREATE_ALWAYS, which truncates. Uploading over the mounted image is the worst of the three, because the directory entry survives and nothing looks wrong until a read comes back short.The comparison is the whole difficulty here, and it is not obvious. The same file reaches the guard spelled three different ways:
1:/x.cueis whatSCSITBService::GetCurrentCDPath()always reports.1://x.cueis whatRealPath()produces, because it formats"%s/%s"ontom_CurrentPath, and that is"1:/"for the whole session whenever the worker auto enters the images partition on connect, which it does for every client that does not CWD somewhere first.1:x.cueis whatFTPPathToFatFsPath()produces, because it consumes the separator after the volume when converting an absolute FTP path and never restores it.FatFs resolves all three to the same file, since a drive relative path is taken against that volume's current directory, which is its root. So every spelling works for
f_open,f_unlinkandf_rename, and only a string comparison can tell them apart. A guard that compares them naively is silently inert for the most ordinary case there is, and appears to work the moment the client happens to CWD first. I know because mine was, twice.Paths are now normalised before comparing, case insensitively because FAT is.
That comparison lives in its own header rather than in the worker because ftpworker.cpp cannot be compiled into the host suite, it needs the socket stack, and a guard nothing can test is a guard that quietly stops working.
test_ftppaths.cppasserts every spelling the worker actually produces, and the negative cases too: a prefix, a deeper path, or a same named file on0:must not match, because a guard that over refuses is its own bug.Hardware tested: delete, rename and upload over the mounted image are all refused over FTP, and the same operations on any other image still work.
Host suite 151/151.