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

EnsureValidTenantSession not needed for sites using subdomain as identification #40

Closed
lasseeee opened this issue Jun 7, 2020 · 5 comments

Comments

@lasseeee
Copy link
Contributor

lasseeee commented Jun 7, 2020

Am I right to assume that the EnsureValidTenantSession is not needed when the tenant is identified through subdomains, and when the login page is at the subdomain (subdomain.domain.com/login)?

This due to the nature of how browsers handle cookie domains when the SESSION_DOMAIN in Laravel is left at the default value null. I.e. the value defaults to the domain of the resource that was requested: subdomain.domain.com, essentially "preventing users from a tenant abusing their session to access another tenant", given they all identify through the subdomain.

@lasseeee lasseeee changed the title EnsureValidTenantSession not needed for sites using subdomain as indetification EnsureValidTenantSession not needed for sites using subdomain as identification Jun 7, 2020
@mpbarlow
Copy link
Contributor

mpbarlow commented Jun 11, 2020

I think the idea is a malicious user logged in as ID 123 on the subdomain1.yourapp.com could change their subdomain1 cookie to be subdomain2 via devtools, and when they go subdomain2.yourapp.com they'd then be logged in as that tenant's user 123.

The middleware prevents that by a. enforcing that every request results in the correct tenant being added to the session, and b. enforcing that the value supplied in the session matches what it's supposed to be.

The only time I think it wouldn't be needed is a setup where one user record can belong to multiple tenants. You'd have to implement the active tenant itself via the session (unless you can pass an explicit tenant ID on every request), so switching tenant would trigger handleInvalidTenantSession -- but you wouldn't need this middleware in that case, because there would only be one user 123 anyway.

@lasseeee
Copy link
Contributor Author

Thanks for the throughout reply. Wouldn't Laravel invalidate the cookie anyway if you attempted to change the cookie domain since they're signed upon creation, effectively preventing against these attacks?

@lasseeee
Copy link
Contributor Author

Hi again @mpbarlow. You mention users being able to change cookies to be valid for other subdomains through devtools. As far as I know all cookies created by Laravel are encrypted and signed to prevent this kind of attack https://laravel.com/docs/7.x/requests#cookies. Is there anything I've missed, or is this middleware truly unneeded in this use case?

@mpbarlow
Copy link
Contributor

mpbarlow commented Jun 21, 2020

I’m not sure of specifics as I didn’t end up using a domain-based tenancy system in my app, but my understanding of the issue is from Mohamed Said’s video series on the multitenancy (see the second video, “Session Hijacking”).

Mohamed is part of the Laravel core team, so I assume this is definitely something that can happen, even though I’m not sure of the exact circumstances!

That said, my understanding is that the cookie signing/encryption prevents the “body” of the cookie being modified, not the domain to which the cookie applies. Don’t quote me on that though!

@lasseeee
Copy link
Contributor Author

You're of course completely right! Thanks for reminding me of Mohamed's video.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants