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

[fix] Secure cookie setting #103

Merged
merged 1 commit into from
Nov 4, 2018
Merged

Conversation

frju365
Copy link
Member

@frju365 frju365 commented Aug 25, 2018

Need test... And I'm not LUA dev at all ;) (try to learn it)

Source :
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
One protection against CSRF BUT not supported by all browser... so need to be cumulate with other protections.

@frju365 frju365 changed the title [fix] CVE CSRF with cookie setting [fix] CSRF with cookie setting Aug 25, 2018
@frju365 frju365 changed the title [fix] CSRF with cookie setting [fix] Secure cookie setting Aug 25, 2018
"; Secure"
"; Secure"..
"; HttpOnly"..
"; SameSite=Strict"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really needs testing as other websites being the sso might uses the cookie and that might break the whole SSO.

Copy link
Member

@zamentur zamentur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this kind of things could be in a testing release, to evaluate possibility to make a version with it.

@frju365
Copy link
Member Author

frju365 commented Aug 26, 2018

Otherwise, i've begun to think (:P ) how we could implement CSRF protection in the form with token generation.

Copy link
Member

@alexAubin alexAubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soo, let's merge this in the next release with a testing before releasing as stable ?

I don't really know how to get feedback / spot issues otherwise ...

@alexAubin alexAubin added this to the 3.3.x milestone Nov 1, 2018
@alexAubin alexAubin merged commit b68ebc0 into YunoHost:stretch-unstable Nov 4, 2018
@randomstuff
Copy link

AFAIU SameSite=Lax should be very fine.

@randomstuff
Copy link

AFAIU, SameSite=Strict breaks links from one domain to another one (eg. links frome the YunoHost home page to SSOwat-protected apps installed in another domain).

@frju365 frju365 deleted the patch-1 branch November 10, 2018 22:25
@alexAubin
Copy link
Member

AFAIU, SameSite=Strict breaks links from one domain to another one (eg. links frome the YunoHost home page to SSOwat-protected apps installed in another domain).

Thanks @randomstuff , I was indeed able to see that it breaks multi-site. Moved to SameSite=Lax in 7be6e76

@frju365
Copy link
Member Author

frju365 commented Nov 20, 2018

merci @alexAubin @randomstuff

@randomstuff
Copy link

I turns out this does not prevent CSR on the admin interface. The session.id and session.hashes cookies are not affected (they are set by moulinette) and I can still CSRF the admin interface:

HTTP/1.1 200 OK
Server: nginx/1.10.3
Date: Mon, 26 Nov 2018 00:35:30 GMT
Content-Type: text/html; charset=UTF-8
Content-Length: 9
Connection: keep-alive
X-SSO-WAT: You've just been SSOed
Set-Cookie: SSOwAuthRedirect=;; Path=/yunohost/sso/; Expires=Thu, 01 Jan 1970 00:00:00 UTC; Secure; HttpOnly; SameSite=Lax ;;
Access-Control-Allow-Origin: *
Set-Cookie: session.hashes="!Kt8mtVS9Mw+7vx+hiKoDPQ==?gAJVDnNlc3Npb24uaGFzaGVzcQF9cQJVB2RlZmF1bHRxA1goAAAAY2I3MzBhMjVlZTAwN2M0MzFiZjYxOGJmMTE4ZTljNTk2MzlkN2Q2MHEEc4ZxBS4="; secure
Set-Cookie: session.id=9d50d49ebce43ba0e864dd2f7eccca828178e95e; secure
Strict-Transport-Security: max-age=63072000; includeSubDomains; preload
Content-Security-Policy: upgrade-insecure-requests
Content-Security-Policy-Report-Only: default-src https: data: 'unsafe-inline' 'unsafe-eval'
X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
X-Download-Options: noopen
X-Permitted-Cross-Domain-Policies: none
X-Frame-Options: SAMEORIGIN
Strict-Transport-Security: max-age=31536000;

@randomstuff
Copy link

It does block the CSRF in vpnclient.

@frju365
Copy link
Member Author

frju365 commented Nov 26, 2018

I didn't find cookie generation in Moulinette. An idea where it is ?

@randomstuff
Copy link

It's in _ActionsMapPlugin.login() and _ActionsMapPlugin.logout() in api.py.

@randomstuff
Copy link

Bottle v0.12 – the version shipped in stretch – does not support SameSite cookie. The dev version does.

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

Successfully merging this pull request may close these issues.

5 participants