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

[Security Bug] Open Redirection Vulnerability at Login Page #5265

Closed
humblelad opened this issue Apr 19, 2020 · 3 comments · Fixed by #5375
Closed

[Security Bug] Open Redirection Vulnerability at Login Page #5265

humblelad opened this issue Apr 19, 2020 · 3 comments · Fixed by #5375
Assignees
Milestone

Comments

@humblelad
Copy link

Describe the bug
It is possible to redirect a user to an attacker owned domain and trick the user. I am investigating on the potential chaining of this bug to perform other attacks.

Expected behavior
Redirection to external urls must not be allowed.

To Reproduce
This occurs at the main login page when the user enters an invalid username/password.
The url gets changed to
http://localhost:1501/authentication/login?old=http%3A%2F%2Flocalhost%3A1501%2Fhome
An attacker can change the url(to any attacker owned domain which may mimic submitty interface) to something like
localhost:1501/authentication/login?old=http%3A%2F%2Flgoogle.com
and trick redirection to google.com users after they enter their creds.

Additional context
This can be prevented either by-

  1. Adding regex checks on url and whitelisting the url.
  2. Completely removing GET request based url redirection.

I am trying to fix this but thought to create this issue for others to contribute the fix if interested.

@MasterOdin
Copy link
Member

MasterOdin commented Apr 19, 2020

I think we can just insert something like:

        if (!is_null($old) && !Utils::startsWith($old, $this->core->getConfig()->getBaseUrl())) {
            $old = null;
        }

at https://github.com/Submitty/Submitty/blob/master/site/app/controllers/AuthenticationController.php#L75 and https://github.com/Submitty/Submitty/blob/master/site/app/controllers/AuthenticationController.php#L95 as I would like to retain the behavior of redirecting the user after they login, and without making it complicated to copy/paste a URL if need be.

@humblelad
Copy link
Author

ok if that works. :) ILYK for any bypasses.

@elihschiff elihschiff self-assigned this May 11, 2020
@elihschiff
Copy link
Member

Is there ever a time when we want the php to redirect users to external websites? It seems like automatically redirecting users to external websites is a bad idea anyway. There might be some way in the future where somebody finds a different way to redirect users to an external domain (other than on login) and only putting the check in the login code would not stop that. If we do want to have the ability to allow redirecting to external websites then we don't have a choice, but if we don't ever want to redirect users to external websites, I think it might be better to add that check here which will stop all redirects.

I can even have it thrown an exception so if a future developer tries an external redirect, it will let them know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment