Skip to content

ftp: require TLS on the data connection for implicit FTPS - #22995

Closed
MegaManSec wants to merge 1 commit into
curl:masterfrom
MegaManSec:ftps-implicit-data-tls
Closed

MegaManSec wants to merge 1 commit into
curl:masterfrom
MegaManSec:ftps-implicit-data-tls

Conversation

@MegaManSec

@MegaManSec MegaManSec commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

curl ftps://host/file leaves CURLOPT_USE_SSL at CURLUSESSL_NONE, so a server rejecting PROT P got a cleartext data connection and a successful transfer. Treat an implicit TLS scheme as CURLUSESSL_ALL when the application set no level; it now fails with CURLE_USE_SSL_FAILED. --ftp-ssl-control is unaffected and restores the old behavior.

Introduced in 4b62cd3 (curl 7.19.3).

LLM-archeology:


On whether the old behavior was intentional: before 7.19.3, ftps_setup_connection() set conn->ssl[SECONDARYSOCKET].use and FTP_PBSZ skipped PROT entirely, so a bare ftps:// URL always encrypted the data connection. 4b62cd3 replaced that assumption with real PROT negotiation because FileZilla served clear-mode implicit control connections — a control-connection fix. But the refusal gate is keyed on the application-set level, added in c78ee11 as "CURLFTPSSL_ALL should make sure that the transfer fails if the data connection isn't set to encrypted properly", so once the scheme stopped implying a level nothing carried the requirement and a refused PROT P became silently acceptable.

docs/cmdline-opts/ssl-reqd.md still promises the enforced behavior: "This option is unnecessary if you use a URL scheme that in itself implies immediate and implicit use of TLS, like for FTPS ... Such a transfer always fails if the TLS handshake does not work." That was written in 50750d0 (2022), long after the change.

Caveat: a server offering implicit TLS but refusing PROT P used to transfer in cleartext and now fails with error 64. --ftp-ssl-control still selects that.

@github-actions github-actions Bot added the tests label Sep 18, 2026
A bare ftps:// URL leaves CURLOPT_USE_SSL at CURLUSESSL_NONE, so a
server rejecting PROT P got a cleartext data connection and CURLE_OK.
Treat an implicit TLS scheme as CURLUSESSL_ALL when deciding the data
connection protection level, without changing ftp_conn.use_ssl, which
takes part in connection reuse matching.

Test 1790.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The TLS enforcement is correct and covered by a focused regression test; only a minor connection-reuse inefficiency remains.

Pull request overview

Enforces encrypted data connections for implicit FTPS unless control-only TLS is explicitly selected.

Changes:

  • Applies CURLUSESSL_ALL semantics to implicit FTPS.
  • Adds regression coverage for rejected PROT P.
  • Registers the new test.
File summaries
File Description
tests/data/test1790 Tests rejected FTPS data encryption.
tests/data/Makefile.am Registers test 1790.
lib/ftp.c Enforces implicit FTPS data TLS.
Review details

Suppressed comments (1)

lib/ftp.c:3176

  • The implicit CURLUSESSL_ALL value is applied only while processing PROT. Curl_ftp_conns_match() still compares the raw ftpc->use_ssl values, so two requests to the same FTPS endpoint—one using the implicit default and one explicitly setting CURLUSESSL_ALL—cannot reuse each other's control connection even though their TLS behavior is now identical. Normalize the implicit FTPS default during setup, or compare effective values in the FTP connection matcher.
  if(!ftpc->use_ssl && (conn->scheme->flags & PROTOPT_SSL))
    return CURLUSESSL_ALL;
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@bagder bagder closed this in 7f36402 Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants