Validate FTPS certificates, stop losing data on failure, plug leaks - #8
Draft
onslaughtq wants to merge 1 commit into
Draft
Validate FTPS certificates, stop losing data on failure, plug leaks#8onslaughtq wants to merge 1 commit into
onslaughtq wants to merge 1 commit into
Conversation
Fixes the security, silent-corruption and resource findings from the audit. Breaking, so both packages go to 0.6.0.0. withFTPS did not verify the server's certificate. Validation was disabled in connectTLS with no way for a caller to enable it, so the connection was encrypted but not authenticated. It now validates by default, with withFTPSSettings for callers who need to supply their own TLSSettings. Reported by @ysangkok in #1, which proposed the same fix and was approved but closed unmerged. That PR changed only the control connection. On its own it would have left every FTPS data transfer unable to validate, because the data channel took its TLS host name from getSocketName -- the local end of the data socket, which no server certificate can match. Security now carries a TLSContext so a data connection can reproduce the control connection's protection against the host it was actually opened to. This completes that PR, and adds the caller-supplied settings variant @jbrechtel asked for on it. Reply lines are bounded. connectionGetLine was called with maxBound, so a server that never sent a newline could exhaust memory before authentication. IO failures are no longer reported as successful short reads. recvAll, getAllLineResp, getMlsxResponse and the conduit sources mapped any IOError to a clean end of data, so a reset or timed-out connection produced a truncated result indistinguishable from a whole one. End of input and failure are now distinct, and only end of input terminates a read. The same conflation made a blank line truncate a conduit listing while the caller still saw 226. A transfer that does not complete normally now still consumes the server's completion reply. Left unread it became the answer to the next command, and every reply after that belonged to the previous command for the rest of the session -- reachable from ordinary conduit code such as retr .| takeC 1. TYPE A transfers send CRLF as RFC 959 requires. sendType TA doubled a CR that was already present and appended a record the input did not have, and the conduit sink terminated per awaited chunk rather than per line, so uploading from sourceFile injected a terminator at every chunk boundary. Three descriptor leaks: createTLSConnection had no bracketOnError, so a refused greeting or rejected AUTH TLS leaked the socket; the data handshake ran after socketToHandle had invalidated the socket its release closed; and the active-mode listening socket was never closed on success, since bracketOnError is only right for the passive case where socketToHandle takes ownership. Both socket helpers now say which they are and why. ccc and auth are gone. CCC cannot work -- there is no way to downgrade our side, so the control connection would desynchronise and security could not be corrected -- and auth alone tells the server to expect a handshake that never comes. Both remain reachable as FTPCommand constructors.
There was a problem hiding this comment.
Pull request overview
Hardens FTP/FTPS security, transfer integrity, protocol handling, and resource cleanup, with corresponding breaking version bumps.
Changes:
- Enables certificate validation and propagates TLS context to data connections.
- Preserves transfer errors, drains completion replies, and corrects ASCII framing.
- Closes descriptor leaks and updates documentation/package versions.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
ftp-client/test/test.hs |
Tests transfer errors and ASCII conversion. |
ftp-client/src/Network/FTP/Client.hs |
Implements TLS and transfer hardening. |
ftp-client/README.md |
Documents certificate validation settings. |
ftp-client/package.yaml |
Bumps package version. |
ftp-client/ftp-client.cabal |
Updates generated package version. |
ftp-client/CHANGELOG.md |
Documents breaking and security changes. |
ftp-client-conduit/src/Network/FTP/Client/Conduit.hs |
Updates streaming transfer behavior. |
ftp-client-conduit/package.yaml |
Bumps version and dependencies. |
ftp-client-conduit/ftp-client-conduit.cabal |
Updates generated package metadata. |
ftp-client-conduit/CHANGELOG.md |
Documents conduit behavior changes. |
Suppressed comments (1)
ftp-client/src/Network/FTP/Client.hs:855
- This handler runs for acquisition failures as well as failures during the transfer. A rejected
PBSZ,PROT,PASV, or transfer command has no pending completion reply, so the drain can block indefinitely after the actual error response was already consumed. Restrict draining to failures that occur after a preliminary transfer response has been accepted.
`M.onException` drainDataResponse ch
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+283
to
+286
| {- | Cap on one reply line. RFC 959 replies are short; this exists so a server | ||
| that never sends a newline cannot make us buffer without limit. Exceeding it | ||
| raises 'Connection.LineTooLong', which is not an 'IOError' and so is not | ||
| swallowed by the end-of-input handling elsewhere in this module. |
| (createSendDataCommand ch pa cmd) | ||
| (MIO.liftIO . SIO.hClose) | ||
| (f . sIOHandleImpl) | ||
| `M.onException` drainDataResponse ch |
Comment on lines
+160
to
+164
| Conduit.bracketP | ||
| (createTLSSendDataCommand ch pa cmd) | ||
| ( \conn -> do | ||
| Connection.connectionClose conn | ||
| getResponse ch >>= debugResponse |
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.
Fixes what an audit of the library turned up. Breaking, so both packages go to 0.6.0.0 — the only major bump in the stack.
withFTPSdidn't verify the server's certificate. Validation was disabled inconnectTLSand a caller had no way to enable it, so the connection was encrypted but not authenticated. It validates by default now, andwithFTPSSettingstakes your ownTLSSettingsif you need something else.@ysangkok reported this in #1 and proposed the same fix, which was approved but closed unmerged. That PR changed only the control connection, and on its own it would have left every FTPS data transfer unable to validate: the data channel took its TLS host name from
getSocketName, the local end of its own socket, which no server certificate can match.Securitynow carries the settings and the host, so a data connection can reproduce the control connection's protection against the host it was actually opened to. This finishes that PR, and adds the settings variant @jbrechtel asked for on it.The rest, briefly. Reply lines are bounded, so a server that never sends a newline can't exhaust memory before authentication.
IOerrors during a transfer no longer come back as a clean short read, which previously made a truncated download indistinguishable from a whole one. A transfer that doesn't finish normally still consumes the server's completion reply, rather than leaving it to become the answer to the next command.TYPE Atransfers send CRLF as RFC 959 requires. Three descriptor leaks are closed.cccandauthare removed, since neither can work outside the sequencecreateTLSConnectionuses.The fuller description of the certificate change is deliberately held back until 0.6.0.0 is on Hackage and there's something to upgrade to.
Stacked:
add-ci-multi-ghc→fix-response-line-partiality→pr-2-multiline-and-handle-exports→add-ci-flag→add-henforcer-fourmolu→fix-audit-findings. Merge bottom-up.