Skip to content

nixos/roundcube: security improvements #77532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jan 22, 2020
Merged

nixos/roundcube: security improvements #77532

merged 3 commits into from
Jan 22, 2020
+59 −20

Conversation

symphorien
Copy link
Member

Motivation for this change
  • database password is world readable in the store
  • php update script is ran as root
Things done
  • If the database is local, use postgres peer authentication. Otherwise, use a password file.
  • Leave database initialisation to postgresql.ensure*.
  • Leave /var/lib/roundcube creation to systemd.
  • Run php upgrade script as unpriviledged user.
  • initialize the des_key (the installer does it, but nixos bypasses the installer...)
  • En passant, fix warning about mime types database

This should be backward compatible:

  • des_key is only used for cookies apparently. So changing it only logs users out.
  • changing the default user: /var/lib/roundcube is chowned by systemd
  • database owner does not change, and can still log in with peer auth even if it previously used a password

The nixos test still passes, with no modification.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @Ma27 @globin @Vskilet

Sorry, something went wrong.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 11, 2020
aanderse
aanderse reviewed Jan 12, 2020
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Fantastic work! Thanks for doing this.

I wonder if @Ma27 has an existing roundcube instance that could be used for testing... 🤔

@aanderse
Copy link
Member

Thanks @symphorien! Let's see what @Ma27 has to say and go from there. Looking over the code you get the 👍 from me, though. Great work!

@Vskilet
Copy link
Contributor

Vskilet commented Jan 13, 2020

Hoo thanks @symphorien ! I'll test now !

Vskilet
Vskilet approved these changes Jan 13, 2020
View reviewed changes
Copy link
Contributor

@Vskilet Vskilet left a comment

Choose a reason for hiding this comment

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

I tested my old config, the new one with the passwordfile and the peer authentication : it all works !
Great work 👍

@Ma27
Copy link
Member

Ma27 commented Jan 13, 2020

I wonder if @Ma27 has an existing roundcube instance that could be used for testing...

I do :)
If I have sufficient time, I can test this tomorrow :)

Ma27
Ma27 requested changes Jan 14, 2020
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for improving this module! Unfortunately I fonud some issues that should be addressed first :)

des_key is only used for cookies apparently. So changing it only logs users out.

Not necessarily: I observed on my personal instance that I didn't get logged out, but I couldn't connect to the mailservers (and had to hit "Logout" manually).

@symphorien
nixos/roundcube: do not write passwords to the store nor run php as root
52cf727 
If the database is local, use postgres peer authentication.
Otherwise, use a password file.

Leave database initialisation to postgresql.ensure*.
Leave /var/lib/roundcube creation to systemd.
Run php upgrade script as unpriviledged user.
@symphorien
Copy link
Member Author

Thanks for your review. I fixed the syntax errors you found.

I don't know what to do about the des_key problem. Any idea?

@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 18, 2020
@symphorien
nixos/roundcube: don't use the default and insecure des_key
b6e6bec 
The php installer creates a random one, but we bypass it, so we have
to create one ourselves.

This should be backward compatible as encryption is used for session
cookies only: users at the time of the upgrade will be logged out but
nothing more.

https://github.com/roundcube/roundcubemail/blob/259b7fa0650fea9320b38cb17c4e80497acae7a3/config/config.inc.php.sample#L73
@symphorien
nixos/roundcube: provide path to mime.types file
b5d692e 
fixes this warning:
WARNING: Mimetype to file extension mapping doesn't work properly!
@symphorien
Copy link
Member Author

On freenode, I was advised to empty the table called session. This is implemented in the current version of the PR. Would you mind testing another time, @Ma27 ?
Users are logged out when /var/lib/roundcube/des_key is created, so you should remove this file beforehand.

Ma27
Ma27 approved these changes Jan 22, 2020
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Deployed to my roundcube instance and confirmed that truncating the session table if no des-key exists works fine 👍

The changes seem fine to me and since there are two more 👍, this should be good to go, so thanks a lot!

@Ma27 Ma27 merged commit 2d9e51a into NixOS:master Jan 22, 2020
@symphorien symphorien deleted the roundcube branch February 5, 2020 20:24
symphorien added a commit to symphorien/nixpkgs that referenced this pull request Feb 5, 2020
@symphorien
nixos/roundcube: add release notes for NixOS#77532
4600fe6
@symphorien symphorien mentioned this pull request Feb 5, 2020
Merged
Ma27 added a commit that referenced this pull request Feb 6, 2020
@Ma27
Merge pull request #79292 from symphorien/roundcube-rl
351b515 
nixos/roundcube: add release notes for #77532
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse

@Ma27 Ma27

@Vskilet Vskilet

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@symphorien @aanderse @Vskilet @Ma27