Skip to content

fix: Drop special bits from Postfix maildrop/ and public/ directory permissions#3625

Merged
georglauterbach merged 5 commits intomasterfrom
maildrop-permissions
Nov 10, 2023
Merged

fix: Drop special bits from Postfix maildrop/ and public/ directory permissions#3625
georglauterbach merged 5 commits intomasterfrom
maildrop-permissions

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented Nov 7, 2023 •
edited by polarathene
Loading

Description

  1. Adjusts the K8s documentation, which requires allowPrivilegeEscalation: true as it turns out
  2. Adjusts permsissions on /var/spool/postfix/{maildrop,public}. This is more tricky, but docs: Kubernetes requies allowPrivilegeEscalation: true #3619 provides good insights. In short, we do not seem to require SUID/SGID/sticky bits on these directories as the binaries that place files in these directories already have the proper SGID bits set on them.

Fixes #3619

CC @innotecsol

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • This change is a documentation update

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

@georglauterbach
update K8s deployment
615fe6f 
Because `allowPrivilegeEscalation` controls SUID/SGID, we require it
when postdrop is invoked.
@georglauterbach
correct permissions for maildrop/public
1eb3928 
The reason our permissions previously worked out as that in setups where
SUID/SGID worked, the binaries used to place files in these directories
already have SGID set; the current set of permissions makes less sense
(as explained in this comment:
#3619 (comment))

Since the binaries used to place files inside these directories alredy
have SUID/SGID set, we do not require these bits (or the sticky bit) to
be set on the directories.
@georglauterbach georglauterbach self-assigned this Nov 7, 2023
@georglauterbach georglauterbach added this to the v13.0.0 milestone Nov 7, 2023
georglauterbach
georglauterbach commented Nov 7, 2023
View reviewed changes
Comment thread target/scripts/startup/setup.d/mail_state.sh Outdated
Comment on lines +108 to +111
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
Copy link
Copy Markdown
Member Author

@georglauterbach georglauterbach Nov 7, 2023

Choose a reason for hiding this comment

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

If maintainers think we should remove this altogether and just use the Debian defaults, I do not mind it either. But remember: these files are most likely in a persisted volume, hence, if we removed these lines, users would be stuck with the approach on master. Keeping these lines here is safer for now; we could add a TODO comment about removing this in future versions.

Copy link
Copy Markdown
Member

@polarathene polarathene Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actual review feedback comment (read + respond to this one)

Outdated

Why is my comment ref stripped away? It was more direct at conveying the information vs a long issue discussion?

If you want to replace the ref rather than add an additional one, perhaps link to a similar comment that communicates the intent to the maintainer/reader for context?

UPDATE: This series of review comments itself now provides a rather comprehensive overview of the topic. It may serve as a better reference link?

Question: What changed since the original #3149 PR to motivate this for you?:

If maintainers think we should remove this altogether and just use the Debian defaults, I do not mind it either.
Keeping these lines here is safer for now; we could add a TODO comment about removing this in future versions.

You can't drop this to rely on Debian defaults.

They're not carried over into a fresh volume mount for these locations IIRC. That was the whole point we explicitly added them 😅

# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
  • Doesn't look like there is any SUID bit involved at all?
    • Only setgid / SGID on the postdrop binary.
    • Possibly also postqueue (it has SGID set too), but I'm not sure about it's involvement with either dir.
  • I agree that these extra special bits (Sticky bit on maildrop/ and SGID on public/) don't seem to be necessary, at least for DMS (dropping them will however diverge from Debian and be inconsistent vs no volume mount for state).
Suggested change
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
# These permissions rely on the `postdrop` binary having the SGID bit set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/pull/3625
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"

NOTE: The sticky-bit on maildrop/ was likely historical from the previous PR referenced Postfix release notes. Debian just may never have adjusted to that on their end, or it may be for supporting third-party software like mailx writing to maildrop/

Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

Reference

All comments here below are a collection of references / notes.

I tried to splitting it out into several comments (easier to link to specific parts for future reference), and to make it easier to scan over by collapsing a fair amount of it.

Relevance of permission special bits

Relevancy of SGID and sticky bits for these locations based on prior PR comments:

Reference (click to expand)
  • My comment from the original PR references Postfix's own SGID check script.
    • UPDATE: find predicates breakdown from this postfix check script is covered in follow-up comment.
  • The permissions we set in the original PR was also to mirror what Debian 11 post-install scripts configured AFAIK?
    • Both @casperklein and myself confirmed that when a volume is not mounted to /var/mail-state (which previously lacked preserving the special bits during our startup script here), the permissions in the container for these locations were 1730 + 2710.
    • The linked PR comment by @casperklein also mentions special bits are missing if these locations recreated during Postfix startup if they were deleted prior.
    • While in another comment I reference postfix via Alpine installs:
      • Without a sticky bit (T / o1000) on maildrop/.
      • I did not mention if SGID (S / o2000) was omitted for public/, but it was presumably missing too.
    • UPDATE: I think one benefit of the original PR was to be consistent regardless if a volume is mounted to /var/mail-state?
  • Additionally my earlier review comment on that PR cites Postfix source again:
    • Where SGID is implied (for maildrop + public).
    • Similar for the postdrop executable (explicitly sets o2755).
    • UPDATE: Appears to be in regards to the intended group for SGID (setgid_group = postdrop) assigned to the postdrop + postqueue executables.

setgid_group

setgid_group is also used in post-install with documented description:

Quoted snippets from referenced docs

setgid_group

  • The group for mail submission and for queue management commands.
  • Its numerical group ID must not be used by any other accounts on the system, not even by the mail_owner account

post-install also referenced setgid_group in docs for create-missing, set-permissions and upgrade-permissions args:

create-missing

  • Create missing queue directories with ownerships and permissions according to the contents of $meta_directory/postfix-files and optionally in $meta_directory/postfix-files.d/*, using the mail_owner and setgid_group parameter settings from the command line, process environment or from the installed main.cf file.
  • This is required at Postfix start-up time.

set-permissions

  • Set all file/directory ownerships and permissions according to the contents of $meta_directory/postfix-files and optionally in $meta_directory/postfix-files.d/*, using the mail_owner
    and setgid_group parameter settings from the command line, process environment or from the installed main.cf file.
  • Implies create-missing.
  • This is required when installing Postfix from a pre-built package, or when changing the mail_owner or setgid_group installation parameter settings after Postfix is already installed.

upgrade-permissions

  • Update ownership and permission of existing files/directories as specified in $meta_directory/postfix-files and optionally in $meta_directory/postfix-files.d/*, using the mail_owner and setgid_group parameter settings from the command line, process environment or from the installed main.cf file.
  • Implies create-missing.
  • This is required when upgrading an existing Postfix instance.

EDIT: setgid_group is also a main.cf parameter, which sates postdrop is the default and changing it would require running postfix set-permissions.

Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

Postfix docs on maildrop/ dir and postdrop command

I also referenced the Postfix docs from my prior PR comment:

Reference (click to expand)

Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

postfix check and find predicates

Reference (click to expand)

find command predicates check the following via Postfix postfix-script (logic for postfix check command):

  • All files that are not of named pipe or socket type within the Postfix queue directory should have the same UID (! \( -type p -o -type s \) ! -user $mail_owner.
  • maildrop and public dirs in the Postfix queue directory should have the SGID intended group postdrop assigned (! -group $setgid_group).
  • postqueue and postdrop commands should have world executable (chmod ugo+x / o111) and have SGID bit (chmod g+s / o2000) permissions (! -perm -02111).

Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

For reference since I always forget this stuff 😅

# Permissions special bit example:
$ touch user group other
# SUID:
$ chmod u+s user
# SGID:
$ chmod g+s group
# Sticky-bit:
$ chmod o+t other

# Default 644 base permissions (`chmod u=rwx,go=r`)
$ eza --octal-permissions --long
2644 .rw-r-Sr-- 0 root  8 Nov 01:01 group
1644 .rw-r--r-T 0 root  8 Nov 01:01 other
4644 .rwSr--r-- 0 root  8 Nov 01:01 user

# Make world executable:
$ chmod ugo+x user group other
$ eza --octal-permissions --long
2755 .rwxr-sr-x 0 root  8 Nov 01:01 group
1755 .rwxr-xr-t 0 root  8 Nov 01:01 other
4755 .rwsr-xr-x 0 root  8 Nov 01:01 user

Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

Permissions - Historical context

In the original PR, I referenced this mailing list link:

Reference (click to expand)

  • The Postfix maintainer states that postfix set-permissions may not be sufficient for Debian due to their downstream packaging? But emphasizes that postfix-files conveys all permissions that Postfix requires to work correctly.

  • An earlier comment at that link notes that maildrop/ is leveraging SGID for access by related processes that may be run from a user that would otherwise lacks access into the directory.

  • The original author message at that link also uses postfix check to verify permissions which are caught as incorrect (EDIT: My bad, it said everything was ok, issue was with third-party software mailx).

  • In the same original PR comment that I shared this mailing list link, I referenced Postfix release notes (many years older than mailing list discussion):

    [snapshot-20020106] This release modifies the existing master.cf
    file. The local pickup service is now unprivileged, and the cleanup
    and flush service are now "public".
    Should you have to back out to a previous release, then you must
    ...
    2) chmod 755 /var/spool/postfix/public.
    To revert to a world-writable mail submission directory, chmod 1733 /var/spool/postfix/maildrop.

    So chmod 755 public (rwx-r-x-r-x) + chmod 1733 maildrop (rwx-wx-wx) was the previous permissions quite a while back.

    pickup and cleanup services in our own master.cf are unprivileged 👍

Now we have these directory permissions:

  • public/ with 710 only permits the group (g=x) to enter the directory to access content inside. The group being postdrop that leverages SGID bit on it's binary.
  • maildrop/ with 730, additionally allows the group to write (g=wx).
    • Programs like Postfix sendmail will use postdrop command (with its SGID permission bit) to write into maildrop/.
    • Additionally adding sticky-bit (T) to maildrop/ would only be useful if users were part of the postdrop group to protect against deletion/edits of files that they have ownership of in maildrop/ from other members of the shared group. But this is not the case; users temporarily are a member of postdrop group via the postdrop executable via setgid (SGID). So no benefit within DMS setups?

Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

Conclusion

The original PR commit message notes that we chose to match what Debian was using, which would be consistent regardless of a volume mount (which this script requires to run).

maildrop/ doesn't need sticky bit (T)

# Send mail as a different user (eg: As user `nobody`):
$ su -s /bin/bash -c 'sendmail -f [email protected] [email protected]' nobody <<< 'this is a test'
postdrop: warning: unable to look up public/pickup: No such file or directory

# Stored mail at `maildrop/` with permissions 644:
$ ls -l /var/spool/postfix/maildrop/
drwx-wx--T 1 postfix postdrop 4.0K Nov  8 05:15 .
drwxr-xr-x 1 root    root     4.0K May  3  2023 ..
-rwxr--r-- 1 nobody  postdrop  108 Nov  8 05:15 3B8FAB6DF

It's perhaps relevant if a third-party software was writing to this location, which is also when SGID is may be relevant (not a concern for DMS):

Reference (click to expand)

  • As the earlier referenced 2015 mailing discussion notes with mailx trying to write to this location.

    While postfix check shows no errors, when I run mailx from the command line I get warnings about an inability to write to /var/spool/postfix/maildrop.

    Postfix uses a very interesting trick of having the executables set the
    GroupID when being run as the user, and this allows them to get into
    directories when the user they are running as normally cannot.

  • On the Debian mailing list in 2001, it was noted that mailx had a security hole allowing users use the mail group, so the packagers removed setgid bit (SGID).

    • It also mentions that Solaris used a world writable maildrop location, which required sticky bit (which apparently elimitates the need for setgid mail programs).
    • In reference to /var/mail, they also encourage it to have sticky bit assigned to significantly reduce the mentioned gid=mail exploit. They suggest for Postfix with /var/mail to use 3775, advising against 2755 (SGID only).

public/ doesn't need SGID set (s)

Postfix docs for master.cf explain what private/ and public/ are for:

Private (default: y)

  • Whether a service is internal to Postfix (pathname starts with private/), or exposed through Postfix command-line tools (pathname starts with public/).
  • Internet (type inet) services can't be private.

Nothing about that seems to justify needing SGID then either? Especially since group permissions is execute only for the directory 🤷‍♂️

Additional context

In the prior PR, I noted that this location only contained unix sockets.

It should only be services defined in our master.cf with private column as n, where most type are unix / inet + 1 fifo + 1 unix-dgram.

Copy link
Copy Markdown
Member Author

@georglauterbach georglauterbach Nov 8, 2023

Choose a reason for hiding this comment

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

Question: What changed since the original #3149 PR to motivate this for you?:

  • You commented recently there that allowPrivilegeEscalation: false caused the issue for you.

    • But that PR itself fixed the same problem you encountered correct?

That's a good point, indeed.

I cannot answer this question TBH, because I don't know the answer 😆 I can only state that the setup with this PR makes way more sense. I do not know why I stopped seeing this error, I can only guess: at some point, I started using a completely custom Postfix configuration - maybe this is the culprit.

If maintainers think we should remove this altogether and just use the Debian defaults, I do not mind it either.
Keeping these lines here is safer for now; we could add a TODO comment about removing this in future versions.

You can't drop this to rely on Debian defaults.

They're not carried over into a fresh volume mount for these locations IIRC. That was the whole point we explicitly added them 😅

👍🏼

# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.

  • Doesn't look like there is any SUID bit involved at all?

    • Only setgid / SGID on the postdrop binary.
    • Possibly also postqueue (it has SGID set too), but I'm not sure about it's involvement with either dir.

Indeed, it's only SGID.

  • I agree that these extra special bits (Sticky bit on maildrop/ and SGID on public/) don't seem to be necessary, at least for DMS (dropping them will however diverge from Debian and be inconsistent vs no volume mount for state).

NOTE: The sticky-bit on maildrop/ was likely historical from the previous PR referenced Postfix release notes. Debian just may never have adjusted to that on their end, or it may be for supporting third-party software like mailx writing to maildrop/

I am pretty sure too that the sticky bit is not required :)

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Nov 7, 2023

PS: I am now running this on my production machine to see whether there are any issues.

polarathene
polarathene requested changes Nov 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

  • 2 suggestions
  • 1 question: What changed since the original #3149 PR to motivate this for you?

I've heavily documented the topic in review comments for my own benefit while recapping on the prior PR. Should be helpful towards addressing: #3557 (comment)

Comment thread docs/content/config/advanced/kubernetes.md Outdated

securityContext:
allowPrivilegeEscalation: false
allowPrivilegeEscalation: true # required for SUID/SGID to work
Copy link
Copy Markdown
Member

@polarathene polarathene Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Since you look after the k8s docs, is this enough context to the user?

Would it be helpful to add extra context?:

Suggested change
allowPrivilegeEscalation: true # required for SUID/SGID to work
# Required to support SGID via `postdrop` executable
# in `/var/mail-state` for Postfix (maildrop + public dirs):
# https://github.com/docker-mailserver/docker-mailserver/pull/3625
allowPrivilegeEscalation: true

git blame will handle that implicitly, so up to you.

Original feedback exploring possibility of improvement

I think this setting might be a bit of a red flag to some users as a workaround?

The equivalent for Docker appears to be opt-in via --security-opt=no-new-privileges, although on a Docker host with SELinux enabled, that or similar restriction may already be default? 🤷‍♂️

Is this config of any help?:

  • fsGroup (NOTE: Support depends on volume driver)

    • By default, Kubernetes recursively changes ownership and permissions for the contents of each volume to match the fsGroup specified in a Pod's securityContext when that volume is mounted.
    • For large volumes, checking and changing ownership and permissions can take a lot of time, slowing Pod startup.
    • You can use the fsGroupChangePolicy field inside a securityContext to control the way that Kubernetes checks and manages ownership and permissions for a volume.

  • Paired with a Volume Subpath:

    Sometimes, it is useful to share one volume for multiple uses in a single pod.
    The volumeMounts.subPath property specifies a sub-path inside the referenced volume instead of its root.

Similar resource from IBM:

fsGroup: set as group ID/SGID on the mounted volume & added as supplemental group ID
Note: The fsGroup option must be supported by the storage provider for the volume. All files and directories created in this directory inherit the fsGroup ID through the special SGID bit.

supplementalGroups: (additional) supplemental groups for shared group access; must be set in the securityContext on pod level

A note on fsGroup:

  • The behavior of applying fsGroup by setting the fsGroup and SGID bit (drwxrwsrwx) on the mounted directory depends on the type of volume and its support of ownership management.
  • It differs for volumes managed by Kubernetes like EmptyDir {} and CSI volumes managed by a CSI driver.
  • By default, Kubernetes recursively changes ownership and permissions for the contents of each volume to match the fsGroup specified in a pod's securityContext when the volume is mounted.

If the two features make any sense to use together, this seems to provide a config example if helpful.

EDIT: I guess there is no granular "capability" that covers this? 😅

Use Case #3: Special Permissions (SUID, SGID)

  • Set User ID (setuid) and Set Group ID (sgid) are special permissions for executable files.
  • When these permissions are assigned to a file, the file to be executed assumes the privileges of the file's owner or group.
  • setuid bit changes a program effective uid (euid) upon execution.

Actually, you seem to already have applied that via SETGID? capability below?

CAP_SETGID:

setgid() sets the effective group ID of the calling process.
If the calling process is privileged (more precisely: has the CAP_SETGID capability in its user namespace), the real GID and saved set-group-ID are also set.

So was allowPrivilegeEscalation: false overriding allowing that capability? Or was it due to another process running instead of the root user / entrypoint? (EDIT: Overriding SETGID capability)

Copy link
Copy Markdown
Member

@polarathene polarathene Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Follow-up adjustment comment regarding vol subpath + fsGroup

Oh I see below that a volume subpath is already used. I was wondering if it'd be good for the maildrop + public dirs if the fsGroup feature caters to that with allowPrivilegeEscalation: false?

Only issue then is the GID won't be static (deterministic) unless we create it before installing the postfix package (like with ClamAV) I think?

Comment thread target/scripts/startup/setup.d/mail_state.sh Outdated
Comment on lines +108 to +111
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
Copy link
Copy Markdown
Member

@polarathene polarathene Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actual review feedback comment (read + respond to this one)

Outdated

Why is my comment ref stripped away? It was more direct at conveying the information vs a long issue discussion?

If you want to replace the ref rather than add an additional one, perhaps link to a similar comment that communicates the intent to the maintainer/reader for context?

UPDATE: This series of review comments itself now provides a rather comprehensive overview of the topic. It may serve as a better reference link?

Question: What changed since the original #3149 PR to motivate this for you?:

If maintainers think we should remove this altogether and just use the Debian defaults, I do not mind it either.
Keeping these lines here is safer for now; we could add a TODO comment about removing this in future versions.

You can't drop this to rely on Debian defaults.

They're not carried over into a fresh volume mount for these locations IIRC. That was the whole point we explicitly added them 😅

# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
  • Doesn't look like there is any SUID bit involved at all?
    • Only setgid / SGID on the postdrop binary.
    • Possibly also postqueue (it has SGID set too), but I'm not sure about it's involvement with either dir.
  • I agree that these extra special bits (Sticky bit on maildrop/ and SGID on public/) don't seem to be necessary, at least for DMS (dropping them will however diverge from Debian and be inconsistent vs no volume mount for state).
Suggested change
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
# These permissions rely on the `postdrop` binary having the SGID bit set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/pull/3625
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"

NOTE: The sticky-bit on maildrop/ was likely historical from the previous PR referenced Postfix release notes. Debian just may never have adjusted to that on their end, or it may be for supporting third-party software like mailx writing to maildrop/

Comment thread target/scripts/startup/setup.d/mail_state.sh Outdated
Comment on lines +108 to +111
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

Reference

All comments here below are a collection of references / notes.

I tried to splitting it out into several comments (easier to link to specific parts for future reference), and to make it easier to scan over by collapsing a fair amount of it.

Relevance of permission special bits

Relevancy of SGID and sticky bits for these locations based on prior PR comments:

Reference (click to expand)
  • My comment from the original PR references Postfix's own SGID check script.
    • UPDATE: find predicates breakdown from this postfix check script is covered in follow-up comment.
  • The permissions we set in the original PR was also to mirror what Debian 11 post-install scripts configured AFAIK?
    • Both @casperklein and myself confirmed that when a volume is not mounted to /var/mail-state (which previously lacked preserving the special bits during our startup script here), the permissions in the container for these locations were 1730 + 2710.
    • The linked PR comment by @casperklein also mentions special bits are missing if these locations recreated during Postfix startup if they were deleted prior.
    • While in another comment I reference postfix via Alpine installs:
      • Without a sticky bit (T / o1000) on maildrop/.
      • I did not mention if SGID (S / o2000) was omitted for public/, but it was presumably missing too.
    • UPDATE: I think one benefit of the original PR was to be consistent regardless if a volume is mounted to /var/mail-state?
  • Additionally my earlier review comment on that PR cites Postfix source again:
    • Where SGID is implied (for maildrop + public).
    • Similar for the postdrop executable (explicitly sets o2755).
    • UPDATE: Appears to be in regards to the intended group for SGID (setgid_group = postdrop) assigned to the postdrop + postqueue executables.

setgid_group

setgid_group is also used in post-install with documented description:

Quoted snippets from referenced docs

setgid_group

  • The group for mail submission and for queue management commands.
  • Its numerical group ID must not be used by any other accounts on the system, not even by the mail_owner account

post-install also referenced setgid_group in docs for create-missing, set-permissions and upgrade-permissions args:

create-missing

  • Create missing queue directories with ownerships and permissions according to the contents of $meta_directory/postfix-files and optionally in $meta_directory/postfix-files.d/*, using the mail_owner and setgid_group parameter settings from the command line, process environment or from the installed main.cf file.
  • This is required at Postfix start-up time.

set-permissions

  • Set all file/directory ownerships and permissions according to the contents of $meta_directory/postfix-files and optionally in $meta_directory/postfix-files.d/*, using the mail_owner
    and setgid_group parameter settings from the command line, process environment or from the installed main.cf file.
  • Implies create-missing.
  • This is required when installing Postfix from a pre-built package, or when changing the mail_owner or setgid_group installation parameter settings after Postfix is already installed.

upgrade-permissions

  • Update ownership and permission of existing files/directories as specified in $meta_directory/postfix-files and optionally in $meta_directory/postfix-files.d/*, using the mail_owner and setgid_group parameter settings from the command line, process environment or from the installed main.cf file.
  • Implies create-missing.
  • This is required when upgrading an existing Postfix instance.

EDIT: setgid_group is also a main.cf parameter, which sates postdrop is the default and changing it would require running postfix set-permissions.

Comment thread target/scripts/startup/setup.d/mail_state.sh Outdated
Comment on lines +108 to +111
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

Postfix docs on maildrop/ dir and postdrop command

I also referenced the Postfix docs from my prior PR comment:

Reference (click to expand)

Comment thread target/scripts/startup/setup.d/mail_state.sh Outdated
Comment on lines +108 to +111
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

postfix check and find predicates

Reference (click to expand)

find command predicates check the following via Postfix postfix-script (logic for postfix check command):

  • All files that are not of named pipe or socket type within the Postfix queue directory should have the same UID (! \( -type p -o -type s \) ! -user $mail_owner.
  • maildrop and public dirs in the Postfix queue directory should have the SGID intended group postdrop assigned (! -group $setgid_group).
  • postqueue and postdrop commands should have world executable (chmod ugo+x / o111) and have SGID bit (chmod g+s / o2000) permissions (! -perm -02111).

Comment thread target/scripts/startup/setup.d/mail_state.sh Outdated
Comment on lines +108 to +111
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

For reference since I always forget this stuff 😅

# Permissions special bit example:
$ touch user group other
# SUID:
$ chmod u+s user
# SGID:
$ chmod g+s group
# Sticky-bit:
$ chmod o+t other

# Default 644 base permissions (`chmod u=rwx,go=r`)
$ eza --octal-permissions --long
2644 .rw-r-Sr-- 0 root  8 Nov 01:01 group
1644 .rw-r--r-T 0 root  8 Nov 01:01 other
4644 .rwSr--r-- 0 root  8 Nov 01:01 user

# Make world executable:
$ chmod ugo+x user group other
$ eza --octal-permissions --long
2755 .rwxr-sr-x 0 root  8 Nov 01:01 group
1755 .rwxr-xr-t 0 root  8 Nov 01:01 other
4755 .rwsr-xr-x 0 root  8 Nov 01:01 user

Comment thread target/scripts/startup/setup.d/mail_state.sh Outdated
Comment on lines +108 to +111
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

Permissions - Historical context

In the original PR, I referenced this mailing list link:

Reference (click to expand)

  • The Postfix maintainer states that postfix set-permissions may not be sufficient for Debian due to their downstream packaging? But emphasizes that postfix-files conveys all permissions that Postfix requires to work correctly.

  • An earlier comment at that link notes that maildrop/ is leveraging SGID for access by related processes that may be run from a user that would otherwise lacks access into the directory.

  • The original author message at that link also uses postfix check to verify permissions which are caught as incorrect (EDIT: My bad, it said everything was ok, issue was with third-party software mailx).

  • In the same original PR comment that I shared this mailing list link, I referenced Postfix release notes (many years older than mailing list discussion):

    [snapshot-20020106] This release modifies the existing master.cf
    file. The local pickup service is now unprivileged, and the cleanup
    and flush service are now "public".
    Should you have to back out to a previous release, then you must
    ...
    2) chmod 755 /var/spool/postfix/public.
    To revert to a world-writable mail submission directory, chmod 1733 /var/spool/postfix/maildrop.

    So chmod 755 public (rwx-r-x-r-x) + chmod 1733 maildrop (rwx-wx-wx) was the previous permissions quite a while back.

    pickup and cleanup services in our own master.cf are unprivileged 👍

Now we have these directory permissions:

  • public/ with 710 only permits the group (g=x) to enter the directory to access content inside. The group being postdrop that leverages SGID bit on it's binary.
  • maildrop/ with 730, additionally allows the group to write (g=wx).
    • Programs like Postfix sendmail will use postdrop command (with its SGID permission bit) to write into maildrop/.
    • Additionally adding sticky-bit (T) to maildrop/ would only be useful if users were part of the postdrop group to protect against deletion/edits of files that they have ownership of in maildrop/ from other members of the shared group. But this is not the case; users temporarily are a member of postdrop group via the postdrop executable via setgid (SGID). So no benefit within DMS setups?

Comment thread target/scripts/startup/setup.d/mail_state.sh Outdated
Comment on lines +108 to +111
# These permissions rely on the `postdrop` binary (and alike) having proper SUID/SGID bits set.
# Ref: https://github.com/docker-mailserver/docker-mailserver/issues/3619
chmod 730 "${STATEDIR}/spool-postfix/maildrop"
chmod 710 "${STATEDIR}/spool-postfix/public"
Copy link
Copy Markdown
Member

@polarathene polarathene Nov 8, 2023

Choose a reason for hiding this comment

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

Conclusion

The original PR commit message notes that we chose to match what Debian was using, which would be consistent regardless of a volume mount (which this script requires to run).

maildrop/ doesn't need sticky bit (T)

# Send mail as a different user (eg: As user `nobody`):
$ su -s /bin/bash -c 'sendmail -f [email protected] [email protected]' nobody <<< 'this is a test'
postdrop: warning: unable to look up public/pickup: No such file or directory

# Stored mail at `maildrop/` with permissions 644:
$ ls -l /var/spool/postfix/maildrop/
drwx-wx--T 1 postfix postdrop 4.0K Nov  8 05:15 .
drwxr-xr-x 1 root    root     4.0K May  3  2023 ..
-rwxr--r-- 1 nobody  postdrop  108 Nov  8 05:15 3B8FAB6DF

It's perhaps relevant if a third-party software was writing to this location, which is also when SGID is may be relevant (not a concern for DMS):

Reference (click to expand)

  • As the earlier referenced 2015 mailing discussion notes with mailx trying to write to this location.

    While postfix check shows no errors, when I run mailx from the command line I get warnings about an inability to write to /var/spool/postfix/maildrop.

    Postfix uses a very interesting trick of having the executables set the
    GroupID when being run as the user, and this allows them to get into
    directories when the user they are running as normally cannot.

  • On the Debian mailing list in 2001, it was noted that mailx had a security hole allowing users use the mail group, so the packagers removed setgid bit (SGID).

    • It also mentions that Solaris used a world writable maildrop location, which required sticky bit (which apparently elimitates the need for setgid mail programs).
    • In reference to /var/mail, they also encourage it to have sticky bit assigned to significantly reduce the mentioned gid=mail exploit. They suggest for Postfix with /var/mail to use 3775, advising against 2755 (SGID only).

public/ doesn't need SGID set (s)

Postfix docs for master.cf explain what private/ and public/ are for:

Private (default: y)

  • Whether a service is internal to Postfix (pathname starts with private/), or exposed through Postfix command-line tools (pathname starts with public/).
  • Internet (type inet) services can't be private.

Nothing about that seems to justify needing SGID then either? Especially since group permissions is execute only for the directory 🤷‍♂️

Additional context

In the prior PR, I noted that this location only contained unix sockets.

It should only be services defined in our master.cf with private column as n, where most type are unix / inet + 1 fifo + 1 unix-dgram.

@polarathene polarathene mentioned this pull request Nov 8, 2023
Open
@polarathene polarathene changed the title scripts: permissions for maildrop and public directories (Postfix) fix: Drop special bits for Postfix maildrop/ and public/ directories Nov 8, 2023
@polarathene polarathene changed the title fix: Drop special bits for Postfix maildrop/ and public/ directories fix: Drop special bits from Postfix maildrop/ and public/ directory permissions Nov 8, 2023
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Nov 8, 2023

Not a single issue with these changes on my production machine so far.

@polarathene polarathene mentioned this pull request Nov 9, 2023
Merged
7 tasks
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 10, 2023

Documentation preview for this PR is ready! 🎉

Built with commit: 7008f66

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Nov 10, 2023 •
edited
Loading

All good on my machine; I will go ahead and merge this. 2 PRs remaining for v13.0.0 then.

@georglauterbach georglauterbach merged commit 2621449 into master Nov 10, 2023
@georglauterbach georglauterbach deleted the maildrop-permissions branch November 10, 2023 18:57
reneploetz pushed a commit to reneploetz/docker-mailserver that referenced this pull request Nov 14, 2023
@georglauterbach @polarathene @reneploetz
fix: Drop special bits from Postfix maildrop/ and public/ directo…
e478456 
…ry permissions (docker-mailserver#3625)

* update K8s deployment

Because `allowPrivilegeEscalation` controls SUID/SGID, we require it
when postdrop is invoked.

* correct permissions for maildrop/public

The reason our permissions previously worked out as that in setups where
SUID/SGID worked, the binaries used to place files in these directories
already have SGID set; the current set of permissions makes less sense
(as explained in this comment:
docker-mailserver#3619 (comment))

Since the binaries used to place files inside these directories alredy
have SUID/SGID set, we do not require these bits (or the sticky bit) to
be set on the directories.

* Apply suggestions from code review

---------

Co-authored-by: Brennan Kinney <[email protected]>
@polarathene polarathene mentioned this pull request Jun 21, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@polarathene polarathene polarathene approved these changes

@casperklein casperklein Awaiting requested review from casperklein

Assignees

@georglauterbach georglauterbach

Labels

area/documentation area/scripts kind/bug/fix A fix (PR) for a confirmed bug

Projects

None yet

Milestone

v13.0.0

Development

Successfully merging this pull request may close these issues.

docs: Kubernetes requies allowPrivilegeEscalation: true

2 participants

@georglauterbach @polarathene