Skip to content
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.

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

[v1.0.0b] [Task] - Improve Application Security #1197

Open
7 of 16 tasks
hay-kot opened this issue May 3, 2022 · 19 comments
Open
7 of 16 tasks

[v1.0.0b] [Task] - Improve Application Security #1197

hay-kot opened this issue May 3, 2022 · 19 comments
Labels
task General Task that needs to be completed v1 Version 1 Issue/PR

Comments

@hay-kot
Copy link
Collaborator

hay-kot commented May 3, 2022

What is the problem this task addresses?

There are several areas that Mealie could improve overall security and practices

User Related

  • Password Complexity Rules
    • Between 8 and 128 Characters
    • Contains at-least a number or a symbol
  • Support 2FA
  • Lock Account w/ Automatic Reset on Bad Login Attempts - security: implement user lockout #1552
    • Lock account after 5 bad login attempts (24 Hours - Configurable)
    • Allow reset by admin
  • Require Email Verification
    • Can be disabled by env

Docker Runtime

  • Can run containers as non-root
    • Ensure file permissions are preserved

General

  • Log failed login attempts in Fail 2 Ban Supported format - Add failed login & IP to log #2365
  • Define Security event in database and log/display events on frontend (Admin)
    • Locked account events
    • Multiple password reset request Events
    • ???

Proposed/Possible Solution(s)?

@hay-kot hay-kot added task General Task that needs to be completed v1 Version 1 Issue/PR labels May 3, 2022
@carif
Copy link

carif commented May 4, 2022

  • Between 8 and 128 Characters

Why is there a max. password length?

  • Support 2FA

Why only 2FA and not MFA?

My ideas:

  • biometric authentification, e.g. try to detect if the mouse is moved by a user or a machine.
  • Captcha support
  • Not sure if it is already done, but if not: store the password via a strong cryptographic hash value.
  • Not sure if it is already done, but if not: store: Use a password salt. The salt value should be unique per user and should be random. E.g. Android generates a random number, hashes the value and adds the hash as the password salt at the end of each password.
  • Not sure if it is already done, but if not: Generate a password pepper value. This value is not saved in the database that contains the hash values and there is only one pepper value for all users. (both this and the above salt value should be used)
  • Make sure one cannot attack the system via the recipe scraper or when the user enters some malicious code in some field, whether it be the log-in field or the recipe ingredient field.

@hay-kot
Copy link
Collaborator Author

hay-kot commented May 4, 2022

Why is there a max. password length?

To prevent denial of service attacks since password fields are publicly exposed. Someone could spam the server with excessively long passwords and block compute space on the server. 128 seemed like a reasonable limit for any password, even generated by a password manager. I want to make it clear that this has NOTHING to do with storage requirements.

Why only 2FA and not MFA?

Because I'm only one man. 2FA is a reachable goal. If someone wants to implement MFA by all means.

biometric authentication, e.g. try to detect if the mouse is moved by a user or a machine.
Captcha support

I don't think either of these can be done without external services, which is why I didn't list them.

Not sure if it is already done, but if not: store: Use a password salt. The salt value should be unique per user and should be random. E.g. Android generates a random number, hashes the value and adds the hash as the password salt at the end of each password.

We use the well regarded passlib library to handle hashing and verification.

Not sure if it is already done, but if not: Generate a password pepper value. This value is not saved in the database that contains the hash values and there is only one pepper value for all users. (both this and the above salt value should be used)

Maybe this is something we could add. I have a feeling implementing this would cause problems with migrations if someone wanted to move a system they could easily forget to move their pepper or something.

Make sure one cannot attack the system via the recipe scraper or when the user enters some malicious code in some field, whether it be the log-in field or the recipe ingredient field.

Luckily we just treat the HTML and JSON scraped as text so the attack vector here should be pretty small. I'm sure this is an area of concern, but this one's pretty low on the list.

Thanks!

@carif
Copy link

carif commented May 7, 2022

Maybe this is something we could add. I have a feeling implementing this would cause problems with migrations if someone wanted to move a system they could easily forget to move their pepper or something.

I don't think that is a huge problem, but certainly something to think about.
The reason why I don't think that this is a huge problem is, that a user can also forget to migrate certain environment variables or settings stored in external files when migrating, e.g. settings for e-mail or the postgresql database password.

Maybe this article can be used as a useful resource. ;)
This paper, this library and this paper might be useful as well.

@gorrilla10101
Copy link

gorrilla10101 commented May 8, 2022

Can you remove the inline styling as well? It's the only part of the Content-Security-Policy header that currently needs to have hash-based exceptions.

Here is a Mozilla site that can scan a site for http vulnerabilities and explains them https://observatory.mozilla.org/

@hay-kot
Copy link
Collaborator Author

hay-kot commented May 8, 2022

Can you remove the inline styling as well?

That's not something I'm going to work on given the scope of the issues, there's more critical security improvements that my time is better spent on. Willing to accept a PR on this is someone wants to go through all the components, but I suspect it will be a pretty time consuming task.

@gorrilla10101
Copy link

That's not something I'm going to work on given the scope of the issues, there's more critical security improvements that my time is better spent on. Willing to accept a PR on this is someone wants to go through all the components, but I suspect it will be a pretty time consuming task.

Don't think it will wind up being too bad, only had 6 that I had to add hashes for. I brought it up since security was being discussed. I'll give it a stab and see if I can resolve them. Will be a good opportunity to look through code anyways.

@gorrilla10101
Copy link

gorrilla10101 commented May 8, 2022

Suggestion for authentication
I would like to see mealie implement OIDC authentication utilizing the PKCE flow. This would allow me to integrate with Keycloak, identity server, azure ad, 0Auth, or any of the other auth servers. It could potentially alleviate the need for mealie to support 2FA and MFA natively, as well.

I haven't looked into the implementation of mealie's authentication system yet, so might be a large lift. If this is a direction you would like to go, I can help/discuss it. I joined the discord channel too, so hopefully, I can start contributing.

@hay-kot
Copy link
Collaborator Author

hay-kot commented May 12, 2022

I would like to see mealie implement OIDC authentication utilizing the PKCE flow. This would allow me to integrate with Keycloak, identity server, azure ad, 0Auth, or any of the other auth servers. It could potentially alleviate the need for mealie to support 2FA and MFA natively, as well.

There have been dozens of requests for this over the last two years and no one has offered to do any of the work around it. I'm not opposed to the feature, but it's not something I would use or am particularly interested in. I'm happy to support the efforts of someone who wants to take this on and write a robust integration with adequate tests. If you or anyone else wants to tackle SSO in some form I suggest you follow the feature requests form and lay out a clear plan on how it would be implemented.

@Drumstickx
Copy link
Contributor

Drumstickx commented Jun 12, 2022

Some input regarding Docker security:

  1. I run mealie in a rootless docker environment without any issues.
  2. Security with Docker containers could be improved by only granting required Linux capabilities to the containers (see also the related section in the Docker Docs)

I commonly drop all capabilities for all containers and then check what operations fail and subsequently add the capabilities required for the application to work properly. With mealie I ended up with the following capabilities:

frontend:
  [...]
  cap_drop:
    - ALL
  cap_add:
    - NET_BIND_SERVICE
api:
  [...]
  cap_drop:
    - ALL
  cap_add:
    - CHOWN
    - DAC_OVERRIDE

Note that the granted capabilities are all included in the default capability set used by Docker. Therefore, we improve security even if the DAC_OVERRIDE capability is quite powerful in my opinion (Bypass file read, write, and execute permission checks.).

The NET_BIND_SERVICE is only required when binding to privileged ports (= < 1024). However, caddy does throw permission denied errors without it even if it is not configured to use privileged ports. I did not yet find the root cause for this behavior.

@legobuild
Copy link

Suggestion for authentication I would like to see mealie implement OIDC authentication utilizing the PKCE flow. This would allow me to integrate with Keycloak, identity server, azure ad, 0Auth, or any of the other auth servers. It could potentially alleviate the need for mealie to support 2FA and MFA natively, as well.

I haven't looked into the implementation of mealie's authentication system yet, so might be a large lift. If this is a direction you would like to go, I can help/discuss it. I joined the discord channel too, so hopefully, I can start contributing.

Not sure if you’ve had any progress yet, but I’d love to help test OIDC. I am currently moving from Keycloak to Authentik and could try out both.

@m00nwtchr
Copy link

To avoid having to tackle the more complex OpenID Connect stuff, reverse proxy/header authentication could be used instead / as a stop gap. Basically you run the application behind a reverse proxy, and the proxy authenticates the user and adds Remote-User and Remote-Groups (if applicable) headers to the request to tell the app which user is logged in, the app often has a "trusted proxies" configuration which prevents other sources from using this to bypass auth.

@sandstormkeshav
Copy link

sandstormkeshav commented Nov 9, 2022

I run mealie in a rootless docker environment without any issues.

@Drumstickx would you mind sharing how you got mealie to run rootless? I tried using PUID/PGID or adding the user directive in my docker compose but I can't get the container to start.

@Drumstickx
Copy link
Contributor

I run mealie in a rootless docker environment without any issues.

@Drumstickx would you mind sharing how you got mealie to run rootless? I tried using PUID/PGID or adding the user directive in my docker compose but I can't get the container to start.

I use a setup where Docker is running without root privileges in the userspace. I guess you are trying to run "rootless" within the mealie container? I have not yet tried that.

@kenske
Copy link

kenske commented Jan 8, 2023

The initial list looks great! One thing I would add to it is avoiding the use of a default admin password, maybe making it a required environment variable or requesting it on first startup.

@patrick-motard
Copy link

Docker Runtime

Can run containers as non-root

Ensure file permissions are preserved

The documentation says that UID and GID can be set via env vars. Is this not the case? I'm running into permission issues when running the pass word reset script in the container and I'm wondering if it's related to this.

@boc-the-git
Copy link
Collaborator

I'm running into permission issues when running the pass word reset script in the container and I'm wondering if it's related to this.

What are the issues?
Providing a report via the standard bug template would help people comment.

@patrick-motard
Copy link

@boc-the-git I appreciate the quick response. I'll submit a bug ticket and link it to this thread via an issue mention.

@patrick-motard
Copy link

@boc-the-git While writing up the bug ticket I was able to solve my issue. In my case, the bind mount where the postgres data is stored had it's ownership overwritten by Open Media Vault on the server where the files live. I'm still trying to track down why OMV is doing this. I suspect it has something to do with NFS or SMB.

I run Mealie in Docker Compose. The Mealie container is run with user/group 1000:1000, but the postgres container is run as root. The password reset script is run by exec-ing into the Mealie container as root and running a python script that needs access to the db files bound to the postgres container. So i set the ownership of the postgres bind mount folder to root.
Given the following in my compose file:

  postgres:
    ...
    volumes:
      - /somepath/mealie_db:/var/lib/postgresql/data
    container_name: postgres

I ran the following on the host:

chown -R root:root /somepath/mealie_db/

Restarted the containers in the stack, then was able to run the password reset script without permission issues.

Sorry for cluttering this thread.

@boc-the-git
Copy link
Collaborator

Glad you solved it @patrick-motard, and thanks for providing an explanation of how in case it helps someone in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task General Task that needs to be completed v1 Version 1 Issue/PR
Projects
None yet
Development

No branches or pull requests

10 participants