security(Postfix): Protect against "SMTP Smuggling" attack

[georglauterbach]

Description

Prevent (currently known) attack vectors of SMTP smuggling, see #3719.

Fixes #3719

Type of change

• Improvement (non-breaking change that does improve existing functionality)

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (README.md or the documentation under docs/)
• If necessary I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have added information about changes made in this PR to CHANGELOG.md

[georglauterbach]

Oh great, this messes with our test suite commands for sending and receiving e-mails, I think... I do not have time now to fix this though...

[polarathene]

Change overview (see review feedback for insights):

• Add smtpd_data_restrictions to main.cf, remove the client and recipient restriction for reject_unauth_pipelining.
  • This defers rejection to a later stage (assuming a permit rule didn't skip restrictions prior), where any detected pipelining in the exchange would trigger the restriction, not just within DATA command.
• Keep smtpd_data_restrictions in Amavis config.
• For a v14 target, smtpd_data_restrictions is not necessary to workaround SMTP smuggling vulnerability.
  • We would have Postfix 3.7.6, this makes smtpd_forbid_unauth_pipelining = yes available. It should be adopted instead (but the smtpd_data_restrictions checklist above is valid (perhaps as a separate PR).
  • Both short-term workarounds also reference enabling smtpd_discard_ehlo_keywords = chunking. This conflicts with the same existing parameter already configured, prepending/appending to that should be fine, but it should be assessed that the feature doesn't have any logic that would accidentally remove chunking, the feature tests may not be sufficient for that, but should be obvious looking over the PR diff (EDIT: Doesn't look like a concern 👍 ).
• Additional context in comments should be added regarding the long-term fix:
  • It replaces the short-term fix parameters as workarounds.
  • It's unclear when Debian Bookworm will update Postfix package with the available fix, but we can mention the min version needed to toggle the long-term fix.

EDIT: smtpd_data_restrictions review feedback has been deferred to a separate PR, with tracking issue: #3728

---

Overview for smtpd_data_restrictions:

• smtpd_data_restrictions = reject_unauth_pipelining should probably remain on Amavis config, even though it'd be technically redundant (since it would now inherit from main.cf). Reasoning is for maintenance, where this is an explicit Amavis config (official docs), and DMS main.cf could change again as review feedback implies. Better for maintenance visibility to retain it.
• smtpd_data_restrictions = reject_unauth_pipelining on main.cf still may make sense, regardless of Amavis integration.
  • Presently we have this check at earlier restriction phases (client and recipient), but those seem unnecessary and can be dropped in favor of deferring to the data restriction which Postfix docs seem to indicate would be sufficient. Only difference would be rejecting due to the restriction slightly earlier (at the RCPT TO SMTP command).
  • Potentially breaking, but only in the sense that a client is not respecting waiting their turn at the DATA command, but for some reason was respectful earlier 🤷‍♂️

[polarathene]

Note that this exists already on smtpd_client_restrictions and smtpd_recipient_restrictions, introduced 7 years ago in #165

Both of those restrictions occur before smtpd_data_restrictions (section describes order), and probably should be removed in favor of smtpd_data_restrictions (based on information that follows below)

---

The PR that introduced reject_unauth_pipelining to the those two restrictions has a dead link, and while it does seem common enough to see in configs online, I'm not sure if they're correct. This comment also advises moving the restriction to smtpd_data_restrictions.

Amavis config for Postfix clearly communicates this configuration requirement for smtpd_data_restrictions:

• https://www.ijs.si/software/amavisd/README.postfix.txt
• https://wiki.archlinux.org/title/Amavis#Quick_start
• https://help.ubuntu.com/community/PostfixAmavisNew#Postfix_integration

The Postfix docs note that it is applicable during any SMTP command context, but also mentions:

With Postfix 2.6 and later, the SMTP server sets a per-session flag whenever it detects illegal pipelining, including pipelined HELO or EHLO commands.
The reject_unauth_pipelining feature simply tests whether the flag was set at any point in time during the session.

Since smtpd_data_restrictions is pretty much at the end of the session, I think that is why earlier reject_unauth_pipelining restrictions aren't likely to provide any benefit? 🤷‍♂️ (other than rejecting at an earlier stage, which should technically only be RCPT TO due to default smtpd_delay_reject=yes, which we for some reason set explicitly)

---

Also related I think is the postscreen Command pipelining test (Postfix docs for postscreen):

Unlike the Postfix SMTP server, postscreen(8) does not announce support for ESMTP command pipelining.
Therefore, clients are not allowed to send multiple commands.

With postscreen_pipelining_enable = yes, postscreen(8) detects zombies that send multiple commands, instead of sending one command and waiting for the server to reply.

This test is opportunistically enabled when postscreen(8) has to use the built-in SMTP engine anyway. This is to make postscreen(8) logging more informative.

The postscreen_pipelining_action parameter specifies the action that is taken next.

[polarathene]

Might be worth adding some context of the restriction.

Suggested change

	smtpd_data_restrictions = reject_unauth_pipelining
	# Block clients that speak too early (don't wait their turn).
	smtpd_data_restrictions = reject_unauth_pipelining

Keeping it in main.cf should be fine AFAIK, Amavis will inherit it and it may still make sense when Amavis is not used? (although I kinda prefer to keep it explicit on the Amavis snippet, instead of implicitly inferring it from main.cf)

---

The documented short-term workaround for the SMTP smuggling vulnerability is:

Suggested change

	smtpd_data_restrictions = reject_unauth_pipelining
	smtpd_data_restrictions = reject_unauth_pipelining
	# This will conflict with existing line:
	# smtpd_discard_ehlo_keywords = silent-discard, dsn
	smtpd_discard_ehlo_keywords = chunking
	# Should replace original (and add contextual comment to PR for the dsn config)
	# Also requires checking that the logic for that PR isn't broken from any assumptions that prepending/appending 'chunking' here may break:
	# smtpd_discard_ehlo_keywords = chunking, silent-discard, dsn

but once we have v14 of DMS we'll have Postfix 3.7.6 and can use the original short-term workaround:

Suggested change

	smtpd_data_restrictions = reject_unauth_pipelining
	# Block clients that speak too early (don't wait their turn).
	# Amavis config expects this (implicitly inherits from main.cf)
	smtpd_data_restrictions = reject_unauth_pipelining
	# Security - Prevent "SMTP Smuggling" exploit
	# https://github.com/docker-mailserver/docker-mailserver/issues/3719
	smtpd_forbid_unauth_pipelining = yes
	# See prior suggestion for concerns
	smtpd_discard_ehlo_keywords = chunking

Only benefit then is to get this in for 13.1, but you've got it marked for v14? In which case the Postfix 3.7.6 fix is more appropriate.

[polarathene]

"When possible" could better clarify that the blocker is waiting on Debian to provide an updated package of Postfix (3.8.4, 3.7.9, 3.6.13 and 3.5.23). For Bookworm that'd be 3.7.9.

The comment also only refers to enabling the parameter, despite obsoleting the short-term workarounds.

---

The longterm-fix also references an optional 2nd parameter to configure:

smtpd_forbid_bare_newline = yes
smtpd_forbid_bare_newline_exclusions = $mynetworks

We shouldn't configure it by default in the image though (since Docker with IPv6 hosts but IPv4 only containers with default user-proxy: true daemon config) can be part of the trusted docker subnet (depending on our PERMIT_DOCKER ENV). Those that need it (like our test-suite) could enable it, and we can document that in our docs somewhere.

[polarathene]

Oh great, this messes with our test suite commands for sending and receiving e-mails, I think...

Technically an improvement 😝

We skipped it in the past by having Amavis disabled in the test suite. Now that you've lifted it up into default main.cf, it gets caught. Which helps illustrate that the current restriction on client and recipient restrictions wasn't too effective vs applying at the data stage 🤔

---

I do not have time now to fix this though...

Should be fixed if we migrate away from nc / openssl based to using swaks. This is available as a package in Debian. Provided most tests use your helper to send mail it should reduce the friction to swap to using swaks and something I've been meaning to do.

An easier workaround of course is to just use our own override support with postfix-main.cf (although that'd conflict with existing tests that provide their own).

EDIT: There is an alternative short-term workaround for SMTP smuggling that you can use instead (as per my review checklist), that'll avoid the test failure issue.

EDIT: The smtpd_data_restrictions addition to main.cf is still worthwhile, but can be deferred until swaks is adopted (tracking issue)

[georglauterbach]

Nice feedback, I will take care of all mentions over the course of the next days. This is marked for v14 still, so we do not need to fiddle with v13.1.0; makes my life easier.

[georglauterbach]

This PR currently depends on #3403 and #3732.

[casperklein]

fyi: I just noticed, that the debian11 postfix package was bumped to version 3.5.23 and introduced this new default values:

smtpd_forbid_bare_newline = no
smtpd_forbid_bare_newline_exclusions = $mynetworks
smtpd_forbid_unauth_pipelining = no
tls_config_file = default
tls_config_name =

However, the new version is not listed yet.

[georglauterbach]

Update: we are currently working on reviewing

• general: update base image to Debian 12 ("Bookworm") #3403 and
• tests: Use swaks instead of nc for sending mail #3732

which are both high-priority by now. Hence, a few breaking changes are going to be introduced with v14.0.0.

[polarathene]

Since the current base image now has an updated postfix with support for the same long-term fix we want to use, would it make sense to address that first and get v13.1.1 released? Or do you want to focus on v14 straight away?

[polarathene]

Updated to use the long-term fix discussed in prior review feedback.

Prior review feedback on the smtpd_data_restrictions = reject_unauth_pipelining is no longer required as part of this PR. I've reverted that, but we can have a separate PR handle that (add to main.cf, keep duplicate in postfix-amavis.cf).

---

Technically if we make a v13.1.1 release, while it's a fix against the attack, it may be breaking for clients unintentionally? Which is why the default is no for existing releases.

• Release as v13.1.1?
• Release as v13.2?
• Release with feature disabled, but add a postfix-main.cf override suggestion in changelog?
• Delay until v14 release?

---

UPDATE: I've had over 5 runs fail now with the Fail2Ban multiple failed logins test-case. Although failure is due to timeout:

Failing test

not ok 52 [Fail2Ban] ban ip on multiple failed login in 11659ms
# (from function `assert_success' in file test/test_helper/bats-assert/src/assert_success.bash, line 42,
#  in test file test/tests/parallel/set1/spam_virus/fail2ban.bats, line 82)
#   `assert_success' failed
#
# -- command failed --
# status : 1
# output : Timed out on command: _exec_in_container /bin/bash -c fail2ban-client status postfix-sasl | grep -F '172.17.0.5'
# --

docker-mailserver/test/tests/parallel/set1/spam_virus/fail2ban.bats

Lines 73 to 83 in 0889b0f

	@test "ban ip on multiple failed login" {
	CONTAINER1_IP=$(_get_container_ip "${CONTAINER1_NAME}")
	# Trigger a ban by failing to login twice:
	CONTAINER_NAME=${CONTAINER2_NAME} _send_email 'auth/smtp-auth-login-wrong' "${CONTAINER1_IP} 465"
	CONTAINER_NAME=${CONTAINER2_NAME} _send_email 'auth/smtp-auth-login-wrong' "${CONTAINER1_IP} 465"
	# Checking that CONTAINER2_IP is banned in "${CONTAINER1_NAME}"
	CONTAINER2_IP=$(_get_container_ip "${CONTAINER2_NAME}")
	run _repeat_in_container_until_success_or_timeout 10 "${CONTAINER_NAME}" /bin/bash -c "fail2ban-client status postfix-sasl | grep -F '${CONTAINER2_IP}'"
	assert_success
	assert_output --partial 'Banned IP list:'

I can only assume this is due to smtpd_forbid_bare_newline = yes, as the expectation is for CR LF not LF, and I believe the PR that added .gitattributes normalized all our nc input files to LF... yet this is the only test-case failing 🤷‍♂️

Reply with Error: bare <LF> received and disconnect when a remote SMTP client sends a line ending in <LF>, violating the RFC 5321 requirement that lines must end in <CR><LF>.

The failed login attempts were presumably rejected before they'd log anything that Fail2Ban monitors to ban the IP. A workaround in the test would be to leverage PERMIT_DOCKER=network to be excluded from the restriction.

#3732 should be compatible, and could be part of a v13.2 release?

[polarathene]

The default is actually $mynetworks already, perhaps we want to disable that? Or rely on the default of PERMIT_DOCKER=none making that unnecessary?

Only concern is due to Docker and IPv6 capable hosts mistakenly routing through to an IPv4 only DMS container (due to Docker daemon user-proxy: true default config), where it'll be trusted in $mynetworks that cover the Docker network gateway address in a subnet (eg: x.y.z.1).

[polarathene]

Resolved. TODO added.

---

CI cache prevented package updates

Tests presently fail due to the new parameter not being recognized. The image was not built with newer packages as our cache fallback provided all earlier layers of the Dockerfile:

Failing test snippet

https://github.com/docker-mailserver/docker-mailserver/actions/runs/7390048599/job/20104276072?pr=3727#step:6:195

not ok 76 [Rspamd] (full) Postfix's main.cf was adjusted in 251ms
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert_output.bash, line 194,
#  in test file test/tests/parallel/set1/spam_virus/rspamd_full.bats, line 66)
#   `assert_output 'rspamd_milter = inet:localhost:11332'' failed
#
# -- output differs --
# expected (1 lines):
#   rspamd_milter = inet:localhost:11332
# actual (2 lines):
#   rspamd_milter = inet:localhost:11332
#   postconf: warning: /etc/postfix/main.cf: unused parameter: smtpd_forbid_bare_newline=yes
# --

docker-mailserver/Dockerfile

Line 204 in 0889b0f

COPY target/postfix/main.cf target/postfix/master.cf /etc/postfix/

I'll purge the CI cache to fix.

[georglauterbach]

I'm still in favor of merging #3403 and #3732 first, then merge this PR, and then release v14.0.0. But I will not block on another solution either; someone will need to take care of this though...

[polarathene]

I'm still in favor of merging #3403 and #3732 first, then merge this PR

See my recent review update. Fail2Ban is failing consistently from this change.

• I suspect with swaks it's a non-issue, tests will pass.
• Since the swaks PR doesn't introduce any breaking changes, I think it may be worth merging that, then this exploit fix assuming tests pass. If tests here pass, then you can choose to go ahead with merging the base image update before this PR if you prefer that.

We can get v14 out quickly, so I don't mind. It may benefit users to stage this out as v13.2 before the base image update, but hopefully neither change will cause any new issues to troubleshoot 👍

[georglauterbach]

I like this route:

1. Merge swaks (requires "just" a review)
2. Merge this (if tests pass) (should be easy as well)
3. Maybe merge OAUTH2 (requires final second review "only")
4. Release v13.2
5. Merge base image update