Conversation
…rl parameter will use the current request as the return url.
if (FormsAuthentication.RequireSSL) { | ||
xsrfCookie.Secure = true; | ||
} | ||
this.requestContext.Response.Cookies.Add(xsrfCookie); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really require a new GUID and cookie? ASP.NET has already assigned a session cookie to the client, hasn't it?
For example, DotNetOpenAuth's OAuth 2 client class uses the preassigned session ID for this exact same XSRF protection. Check it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Levi recommends creating a new Guid instead of using the Asp.net session id.
Levi can explain the reason.
On May 15, 2012, at 6:09 AM, "Andrew Arnott" reply@reply.github.com wrote:
@@ -148,6 +159,20 @@ public OpenAuthSecurityManager(HttpContextBase requestContext)
// attach the provider parameter so that we know which provider initiated
// the login when user is redirected back to this page
uri = uri.AttachQueryStringParameter(ProviderQueryStringName, this.authenticationProvider.ProviderName);
+
// Guard against XSRF attack by injecting session id into the redirect url and response cookie.
// Upon returning from the external provider, we'll compare the session id value in the query
// string and the cookie. If they don't match, we'll reject the request.
string sessionId = Guid.NewGuid().ToString();
uri = uri.AttachQueryStringParameter(SessionIdQueryStringName, sessionId);
var xsrfCookie = new HttpCookie(SessionIdCookieName, sessionId);
if (FormsAuthentication.RequireSSL) {
xsrfCookie.Secure = true;
}
this.requestContext.Response.Cookies.Add(xsrfCookie);
Does this really require a new GUID and cookie? ASP.NET has already assigned a session cookie to the client, hasn't it?
For example, DotNetOpenAuth's OAuth 2 client class uses the preassigned session ID for this exact same XSRF protection. Check it out
Reply to this email directly or view it on GitHub:
https://github.com/AArnott/dotnetopenid/pull/145/files#r823146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a session on behalf of the application when the application didn't explicitly ask for one is generally considered rude. It has several side effects: (a) the application now has to worry about state management where previously it didn't, and (b) requests from the client to the server are now serialized, hampering performance. Using a random GUID gives much of the same benefit without these drawbacks.
There is a downside: you're still open to XSRF attacks where your site is target.domain.com and I am running code under evil.domain.com. But I think we're OK with this, since any mission-critical site will most certainly not allow potentially malicious code to run under the same domain. Though unfortunately I keep needing to remind the Windows team of this fact.. :|
-----Original Message-----
From: Luan Nguyen
Sent: Tuesday, May 15, 2012 6:13 AM
To: Andrew Arnott
Cc: Levi Broderick
Subject: Re: [dotnetopenid] Add protection against XSRF attacks (#145)
Levi recommends creating a new Guid instead of using the Asp.net session id.
Levi can explain the reason.
On May 15, 2012, at 6:09 AM, "Andrew Arnott" reply@reply.github.com wrote:
@@ -148,6 +159,20 @@ public OpenAuthSecurityManager(HttpContextBase requestContext)
// attach the provider parameter so that we know which provider initiated
// the login when user is redirected back to this page
uri = uri.AttachQueryStringParameter(ProviderQueryStringName, this.authenticationProvider.ProviderName);
+
// Guard against XSRF attack by injecting session id into the redirect url and response cookie.
// Upon returning from the external provider, we'll compare the session id value in the query
// string and the cookie. If they don't match, we'll reject the request.
string sessionId = Guid.NewGuid().ToString();
uri = uri.AttachQueryStringParameter(SessionIdQueryStringName, sessionId);
var xsrfCookie = new HttpCookie(SessionIdCookieName, sessionId);
if (FormsAuthentication.RequireSSL) {
xsrfCookie.Secure = true;
}
this.requestContext.Response.Cookies.Add(xsrfCookie);
Does this really require a new GUID and cookie? ASP.NET has already assigned a session cookie to the client, hasn't it?
For example, DotNetOpenAuth's OAuth 2 client class uses the preassigned session ID for this exact same XSRF protection. Check it out
Reply to this email directly or view it on GitHub:
https://github.com/AArnott/dotnetopenid/pull/145/files#r823146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that ASP.NET sessions are lazily created upon the first use of HttpContext.Session
? Interesting. I can see your point then.
As for the domain name issue, I'm concerned about that drawback. Windows Azure, in particular, makes it trivially easy (in fact it's the dfeault) to set up a production site at *.cloudapp.net
, which as you know means everyone is sharing a second-level domain name. I know there is guidance that folks not use this default, but nevertheless this, and *.appspot.com, and others, make it easy to do. Is there no way to make the HTTP cookie local to the exact host name rather than scoped just to the domain?
Add protection against XSRF attacks
string sessionId = Guid.NewGuid().ToString(); | ||
uri = uri.AttachQueryStringParameter(SessionIdQueryStringName, sessionId); | ||
|
||
var xsrfCookie = new HttpCookie(SessionIdCookieName, sessionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that setting HttpOnly=true on this would be a great mitigation against XSS attacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'll fix it.
No description provided.