mail-in-a-box / mailinabox

Mail-in-a-Box helps individuals take back control of their email by defining a one-click, easy-to-deploy SMTP+everything else server: a mail server in a box.
https://mailinabox.email/
Creative Commons Zero v1.0 Universal
13.78k stars 1.43k forks source link

Small patch to solve all those security issues #1766

Open woodholly opened 4 years ago

woodholly commented 4 years ago

Extension patch with security examples. This patch does not remove or disable anything, but extends nginx configs with shortcuts to user defined nginx configurations. At the same time it contains example (not active) configs to protect mailinabox web server from unauthorized access with additional layer of basic http authorization.

Patch is very small and can be applied to already installed mailinabox but I want it in upstream. 0001-adds-optional-user-defined-nginx-configuration-files.patch.txt

How to apply patch: git am < 0001-adds-optional-user-defined-nginx-configuration-files.patch.txt

JoshData commented 4 years ago

This looks like it would be better posted on the forum as documentation for anyone who wants to make these sorts of changes.

If you want to talk through whether or not we'll include it upstream, please write out a list of the changes in a reply so I can address each, or open separate issues or pull requests. Thanks.

woodholly commented 4 years ago

I don't think so. Without those shortcuts in nginx configurations you are forced to appy the patch after each update and open your web server unprotected during the update, so these shortcuts should be somewhere in the code. I don't think there is better way to protect open web server but by basic http authorization. This can shrink attack surface to only nginx itself and ssl. It protects from bugs in PHP, miab admin panel, Nextcloud, webmail.

I would like to see others opinions about it. This patch can solve these issues without additional modifications: #1745 #1696 #1601

Patch is very small, it adds only 4 reasonable lines and each is commented, you want me to duplicate it as text here ?

woodholly commented 4 years ago

This #1646 can be addressed too.

JoshData commented 4 years ago

I just want you to list here what the goals are and their associated changes to make it easier for me to respond to and for others to follow the conversation without having to read the patch file.

woodholly commented 4 years ago

I see, well, it has two goals / solves two problems: 1) Make mailinabox nginx configuration customizable, so anybody who wants to change something in it would be able to do it without modifying mailinabox code.

2) Give an option to protect mailinabox from web related bugs

Problem 1 is solved by modifying mailinabox nginx configs, I have added shortcuts to snippets/

Problem 2 depends on Problem 1 and is solved by adding example scripts to snippets/ which protect all web access by basic http auth (.htpasswd) and a secret cookie. Read patch comments for details.

JoshData commented 4 years ago

Make mailinabox nginx configuration customizable

There is already a hidden feature at https://github.com/mail-in-a-box/mailinabox/blob/master/management/web_update.py#L179 to inject additional nginx config lines from files under the user's control. I didn't read the patch carefully enough to know if what you are proposing works differently or if there's a reason to have both methods, which is a question that would need to be figured out first. Either way, if we make this an official feature, it will need documentation somewhere.

Give an option to protect mailinabox from web related bugs

You're going to have to be more specific about what problem you are trying to solve.

woodholly commented 4 years ago

https://github.com/mail-in-a-box/mailinabox/blob/master/management/web_update.py#L179 it looks like unusable, it provides no way to define global changes

zatricky commented 4 years ago

Same as one of Josh's suggestions, I also suggest opening pull requests ; one for each issue addressed. As is, the discussion covers two separate issues that should separated as such.

A pull request is easier to examine and verify compared to a patch attachment. Thus I wouldn't be surprised if nobody properly examines the patch you gave.

woodholly commented 4 years ago

Sorry guys, I don't have time, hope someone will test it and will create correct pull request or replace it with new patch to make mailinabox nginx configurable.