Fix GHSL-2024-288 #8811
Fix GHSL-2024-288 #8811
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginHandler
participant Settings
User->>LoginHandler: Submit login form
LoginHandler->>LoginHandler: Authenticate user
alt Successful authentication
LoginHandler->>Settings: Retrieve DEFAULT_PAGE
LoginHandler-->>User: Redirect to "/" + DEFAULT_PAGE + "/"
else Failed authentication
LoginHandler-->>User: Render login page with error message
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
sickchill/views/authentication.py (2)
Line range hint
9-13: Critical: GET method still vulnerable to open redirectWhile the POST method has been secured, the GET method still uses the user-controlled 'next' parameter for redirection, leaving it vulnerable to the same security issue.
Apply this fix to make it consistent with the POST method:
def get(self, next_=None): - next_ = self.get_query_argument("next", next_) if self.get_current_user(): - self.redirect(next_ or "/" + settings.DEFAULT_PAGE + "/") + self.redirect("/" + settings.DEFAULT_PAGE + "/") else: t = PageTemplate(rh=self, filename="login.mako") self.finish(t.render(title=_("Login"), header=_("Login"), topmenu="login", login_error=login_error))
Line range hint
7-30: Consider additional security improvementsWhile the open redirect fix is good, there are several security-related improvements to consider:
- The
login_errorglobal variable could lead to race conditions in concurrent scenarios- Direct password comparison might be vulnerable to timing attacks
- Consider adding rate limiting for failed login attempts
Suggested improvements:
- Move
login_errorto session storage- Use constant-time comparison for passwords
- Implement rate limiting based on the IP logging you already have
Would you like me to provide implementation examples for any of these security improvements?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
sickchill/views/authentication.py(1 hunks)
🔇 Additional comments (1)
sickchill/views/authentication.py (1)
31-31: Security fix looks good!
The change correctly addresses the open redirect vulnerability by removing the user-controlled redirect parameter and using a hardcoded default page instead.
|
User controllable variables are fine. No matter what you put in there you can't access SC without logging in. It's a single user system. If I'm wrong about that, show me how you can access anything without logging in or using a cookie or bypass method outside of this parameter. IMHO this is a false positive. |
This issue does not lead to an attacker accessing anything without logging in. This issue allows for redirecting a user to an attacker-controlled website and could be used in phishing attempts. The impact and PoC for this issue is described in the report I sent to you via email in issue number 5. I don't want to share more details in here, because it would be not in line with coordinated disclosure policy.
I take that SC gets a lot of bogus reports. This is a real bug (see PoC in the report), but it does not lead to accessing anything without logging in. That said, if you don't consider this a bug, feel free to close this PR. |
|
I'll accept the PR, but there is no way an attacker can exploit this without a login. If you can't login, the next parameter can't be used by anyone. |
2 participants
Proposed changes in this pull request:
change the login page to redirect to
settings.DEFAULT_PAGEinstead of to thenextparameter, which can be user-controlled. Please see GHSL-2024-288 (issue 5 in the report) for more information.PR is based on the DEVELOP branch
Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
Read contribution guide
Summary by CodeRabbit
New Features
Bug Fixes