Backport 2.10.0 changes #7230
Merged
Backport 2.10.0 changes #7230
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Redis can require basic password authentication to further limit who can access it. We need to set a password in `/etc/redis/redis.conf` and then ensure all clients pass it when authenticating. For most of our code, this is straightforward since we can set the password in config.py and access it through `SecureDropConfig`. However the `rqworker` has its own config file that we haven't needed until now, so we also create a `rq_config.py` with just the redis password. The ansible playbook now generates a 32-byte password for redis, sets it in config.py, rq_config.py, and /etc/redis/redis.conf, and then restarts the redis-server systemd service. The postinst generates a 32-byte password in the same manner, writes it to the same set of files, and then restarts the redis-server service. This ended up requiring the most changes, since I removed the inline Python code from the Makefile into a separate `dev-config` script, which now also handles creation of rq_config.py. We set the password in /etc/redis/redis.conf and then start the redis-server service. It's also written to `/tmp/redispasswd` for easy fetching in tests. testinfra tests verify that that unathenticated access fails and that the configured `rq_config.py` password works. This addresses SEC-01-008 WP3: "Read+Write Access to Redis via UnAuth Telnet (Low)". Fixes <freedomofpress/securedrop-security#97>.
Co-authored-by: Cory Francis Myers <cory@freedom.press>
Set password for redis access
Collateral changes here address cases where tests have:
1. done app.get("/logout") without checking either the response code or
the result of the actual logout operation; or
2. assumed there will be exactly one button, belonging to exactly one
form, on any given page.
Co-authored-by: Kunal Mehta <leogktm@debian.org>
We require that all users have a strong diceware password by generating it ourselves for them to use. However, we accept any diceware-looking sent in the POST request, meaning that a user can deliberately set a weak or fixed password. (It would be difficult for an attacker to do so since they'd need to both bypass the onion service authentication and CSRF.) To fix this, upon generating a new password for users, we save a hash of it in the user's session. When they save the password, we validate it against the hash and if matches, allow it to be saved. This ends up being mostly straightforward (outside of tests), requiring modifications to utils.set_diceware_password() and the creation of utils.set_pending_password() and utils.verify_pending_password(). Tests are a bit more complicated because we rely on being able to send arbitrary POST strings to the correct endpoint, so it needs to manually be set in the session. New tests for this functionality are added as well. This addresses "SEC-01-002 WP4: Possible Account Takeover via Weak Password Policy (Low)" Fixes <freedomofpress/securedrop-security#94>.
Validate user provided same password back to server
See <https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#controlling_third-party_cookies_with_samesite> for an explanation of how SameSite="Strict" works. We just need to set the correct Flask config value and it automatically takes care of the rest. The journalist session code has some conditional code to support older versions of Flask that we no longer care about. Tests verify that `SameSite=Strict` is set on the session cookie in both the JI and SI. Note that the `http.cookie_jar` module doesn't support the SameSite option directly (<python/cpython#88629>) so we need to use the `get_nonstandard_attr()` method to access the value. Refs <freedomofpress/securedrop-security#53>.
thread: freedomofpress/securedrop-server-security#9 (comment) thread: freedomofpress/securedrop-server-security#9 (comment) Co-authored-by: Kunal Mehta <legoktm@debian.org>
fix(`/logout`): require `POST` for CSRF protection
Set SameSite="Strict" on all cookies for more CSRF protection
The `/admin/2fa` endpoint has an IDOR vulnerability in which changing the value of the `uid` parameter reveals that user's TOTP secret. The similar `/account/2fa` endpoint allows a user to see their own TOTP secret, which we'd like to avoid as well. Address that by requiring the TOTP secret to be checked to be passed as a form parameter. This allows to keep the existing workflow while removing the abilty to get arbitrary users' TOTP secrets. Practically the `/2fa` endpoint is split into `/verify-2fa-totp` and `/verify-2fa-hotp` endpoints that now each have their own template and function. Both contain documentation that explains what is provided by the user and what is looked up in the database. Tests needed an update because various endpoints no longer redirect. Note that no functional test changes were needed because there are no user-facing behavioral changes. This addresses "SEC-01-001 WP4: Arbitrary 2FA Enrollment via IDOR (medium)". Fixes <freedomofpress/securedrop-security#92>.
Don't allow admins to look up arbitrary users' TOTP secrets via the web
[2.10.0] SecureDrop 2.10.0~rc1
SecureDrop 2.10.0
In a future 2.10.1 upgrade, the global chown over /var/www/securedrop would've blown away rq_config.py's root:www-data ownership, breaking read access for www-data. Add an exclusion, just like the existing one for config.py.
Ensure rq_config.py permissions are restored on next upgrade
rocodes
approved these changes
Sep 19, 2024
There was a problem hiding this comment.
- Visual review: commits match those staged privately for release per https://securedrop.org/news/securedrop-2_10_0-released/
- CI is passing
- Confirming 3ddc876
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
Ready for review
Description of Changes
This is a backport of all the changes released in 2.10.0 that were developed privately (it is a branch merge because we need to merge everything, and not cherry-picks like we normally do for the changelog). See https://securedrop.org/news/securedrop-2_10_0-released/ for more details.
Testing
How should the reviewer test this PR?
Deployment
Any special considerations for deployment? n/a