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

Use HttpOnly attribute for vibe.http.session by default #368

Closed
ilya-stromberg opened this issue Oct 28, 2013 · 27 comments · Fixed by #373
Closed

Use HttpOnly attribute for vibe.http.session by default #368

ilya-stromberg opened this issue Oct 28, 2013 · 27 comments · Fixed by #373

Comments

@ilya-stromberg
Copy link
Contributor

We should use HttpOnly attribute by default for sessions.
It helps to prevent XSS attacks because session Id will not be available for JavaScript. Also, it's something wrong if you really need to use session Id from JavaScript.
http://en.wikipedia.org/wiki/HTTP_cookie

Also, we should use Secure attribute for SSL connections by default. It allows to send session Id only via SSL.

ilya-stromberg added a commit to ilya-stromberg/vibe.d that referenced this issue Nov 4, 2013
s-ludwig added a commit that referenced this issue Nov 4, 2013
Use `HttpOnly` attribute for `vibe.http.session` by default, see #368
@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

Regarding the Secure attribute, I agree that this should be done, but the startSession signature needs to be adjusted a bit. I'll think about this some more later today.

@ilya-stromberg
Copy link
Contributor Author

Regarding the Secure attribute, I agree that this should be done, but the startSession signature needs to be adjusted a bit.

Note that we can use proxy like Nginx. For example, Nginx will be use SSL connection for client, and Vibe.d will use no-SSL connection for local network. So, in that case we need option to force using Secure attribute.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

So, in that case we need option to force using Secure attribute.

Sure, the current startSession("/", true) syntax for this would be kept for a while. But I'd introduce a new overload that takes e.g. an option enum, f.ex.: startSession("/", SessionOption.secure|SessionOption.httpOnly) or startSession("/", SessionOption.noSecure). The default would be SessionOption.httpOnly and Secure would be inferred from req.fullURL.schema or from the X-Forwarded-Protocol header, if neither secure, nor noSecure are present.

After a while, the old boolean based overload would then get deprecated.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

Actually, since using the X-Forwarded-Protocol header wouldn't really enhance security (as it can be easily manipulated), I'll leave that out and only use the direct connection to infer the default...

@ilya-stromberg
Copy link
Contributor Author

The default would be SessionOption.httpOnly and Secure would be inferred from req.fullURL.schema or from the X-Forwarded-Protocol header

No, it's dangerous. I can change URL schema from https to http very easy. Also, it's easy to change HTTP header. So, correct default logic should be something like this:

Have we got SSL support for Vibe.d (SSL certificate, SSL port and etc.)?
if yes, have we got no-SSL support?

  • if yes, infer Secure attribute from URL scheme
  • if no, use only Secure attribute

Also, it would be nice to have the same options for all cookies. So, we should name it CookieOption and add CookieOption.defaultSecure.

@ilya-stromberg
Copy link
Contributor Author

Also, we should add warning if people use both SSL and no-SSL connections.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

There are a few problems with that approach. The first is backwards compatibility, even my suggestion would break certain use cases, but it would be fairly non-intrusive. The other issue is that the fact that there is an HTTPS listener doesn't mean that it's related to the listener used to start the session. However, assuming that both listeners are related, your approach seems to be exactly the same. If there is only SSL support, there is no way to generate a non-SSL URL and if both SSL and no-SSL are supported, you still infer it from the request protocol.

Of course, the programmer needs to make sure that the login request (or whatever starts the session) happens over HTTPS whenever both HTTP and HTTPS are possible, but if that isn't the case, the session ID is the smallest problem.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

Also, we should add warning if people use both SSL and no-SSL connections.

I don't know. It may be perfectly fine to have both. Going by that logic it would make more sense to warn when only no-SSL is present.

@ilya-stromberg
Copy link
Contributor Author

If there is only SSL support, there is no way to generate a non-SSL URL

Not exactly. Vibe.d can't accept no-SSL request if it have only SSL support. But client CAN sent the data via no-SSL protocol and hacker can analyze it.

So, if hacker can send no-SSL request (for example, via JavaScript) and have traffic sniffer he CAN compromise the session Id. Note that HttpOnly attribute doesn't help in this case because browser will send session Id even if it have HttpOnly attribute. We MUST use Secure attribute to prevent this attack.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

How would that no-SSL request result in a started session if it isn't accepted by vibe.d?

@ilya-stromberg
Copy link
Contributor Author

Hacker doesn't need vibe.d result. He need browser request.

Also, hacker can use additional exploits to substitute HTTP server - DNS security issues come to mind first:
http://en.wikipedia.org/wiki/Domain_Name_System#Security_issues

Also, BGP protocol had issues. So, we shouldn't think that it's impossible.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

Huh? But we are talking about inferring the Secure value here. Of course the whole Secure feature is not strong security. If the hacker can compromise the browser, a proxy server in the path, or substitute the server using DNS exploits, there is nothing that can be done about it by changing the way Secure is inferred. Unless I'm really missing something here.

@ilya-stromberg
Copy link
Contributor Author

But we are talking about inferring the Secure value here.

Yes, we are. But hacker can use multiply issues.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

So how exactly would a hacker manipulate the Secure default inference?

@ilya-stromberg
Copy link
Contributor Author

He can access only to encrypted data. If hacker send no-SSL request browser will NOT send session Id.

It means that hacker will NOT receive session Id If we assume that SSL is secure.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

Sorry, I still don't understand where you see the problem with my proposal (to use the request protocol for inferring Secure). So the setup is as following:

server: vibe.d with "/login" starting the session. "/login" is only available using HTTPS requests. Secure is inferred from the request protocol (i.e. HTTPS).

browser -> hacker -> server

What can the hacker now do to talk the server into sending no Secure attribute? (All other kinds of attacks are out of our reach, or at least don't have anything to do how we set Secure)

@ilya-stromberg
Copy link
Contributor Author

server: vibe.d with "/login" starting the session. "/login" is only available using HTTPS requests. Secure is inferred from the request protocol (i.e. HTTPS).

Yes, you are right. I thought we talk about two different cases, SSL-only requests or Secure attribute. Sorry for misunderstanding.

Question: what happens if I send HTTP request to the SSL port? Does Vibe.d prints a error message or parse it as no-SSL request?

@s-ludwig
Copy link
Member

s-ludwig commented Nov 4, 2013

Okay, I'll switch to using an enum then. We can still resolve any remaining issues afterwards before the next release (probably easier to reason about actual code, I guess).

what happens if I send HTTP request to the SSL port? Does Vibe.d prints a error message or parse it as no-SSL request?

It will fail with a local exception in the SSLStream constructor and close the connection before parsing the request, so there should be no danger.

@ilya-stromberg
Copy link
Contributor Author

Also, we should add warning if people use both SSL and no-SSL connections.

I don't know. It may be perfectly fine to have both. Going by that logic it would make more sense to warn when only no-SSL is present.

If I use no-SSL only, I agree with possible risk. Probably, I should use it only for non-critical purposes.
I must use SSL only if I need security.
It something strange If I use both no-SSL and SSL. It means that I have almost the same security level as no-SSL.

Maybe we should add two different warnings for both no-SSL only and no-SSL and SSL connections. But I agree that it isn't critical issue, and we can add documentation notice.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 5, 2013

It may be valid though to have both, if either the non-SSL version uses a completely different request handler, or if there is a router entry that makes certain routes unavailable for non-SSL access (could also require authentication at the same time):

auto router = new URLRouter;
router.get("/", &showHomepage);
router.get("/images/*", serveStaticFiles(...));
router.any("*", &enforceSSL); // all routes below require SSL
router.post("/login", &login);

void enforceSSL(HTTPServerRequest req, HTTPServerResponse res) {
    enforceHTTP(req.ssl, HTTPStatus.forbidden);
}
// ...

My feeling is just that on this level, there are so many things that can be done right or wrong, that emitting a warning may just be annoying if the programmer knows what he is doing, or worse, that it creates a false sense of security when the warnings have been avoided. Maybe this would be an opportunity for a small framework that is layered on top of the current low-level one. This framework could then for example provide the complete encryption/authentication/authorization pipeline and make sure that everything is done right.

s-ludwig added a commit that referenced this issue Nov 5, 2013
… an SSL connection. See #368.

Also introduces a new overload for HTTPServerResponse.startSession that takes SessionOption enum instead of boolean values.
@ilya-stromberg
Copy link
Contributor Author

No, you are wrong.
We can have Man-In-The-Middle attack in your example. See also #379.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 7, 2013

You mean a MiM attack that changes if we send Secure or not? If so, please tell how, I can't see it. Again, I'm talking solely about inferring the Secure attribute here. Everything else is a separate issue.

@ilya-stromberg
Copy link
Contributor Author

OK, assuming that we have Secure attribute. In that case hacker must be able to listen and modify Internet traffic (for example, public Wi-Fi).

Attack example:

  • User open http://example.com/ homepage. Note that we don't use SSL here. Hacker modify Vibe.d's answer and replace https://example.com/login link to the http://example.com/login.
  • User open http://example.com/login page. Hacker creates login page that looks like Vibe.d's login page.
  • User enter login and password. Hacker save this data and enter it to the Vibe.d. MiM attack was successful.

Everything else is a separate issue.

Note that without Secure attribute MiM attack will be easer - hacker must be able only to listen Internet traffic.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 7, 2013

Please reread my previous comments. We have to assume that the login page is only available using an SSL connection. If the hacker can sniff the login credentials, then who cares about the session ID?

@ilya-stromberg
Copy link
Contributor Author

If the hacker can sniff the login credentials, then who cares about the session ID?

Yes, you are right.

It may be valid though to have both, if either the non-SSL version uses a completely different request handler, or if there is a router entry that makes certain routes unavailable for non-SSL access (could also require authentication at the same time):

You tell me that it's possible to have both SSL and non-SSL protocols as valid configuration. I disagree with you and provide attack example. I don't tell that it's easy, but still possible in theory.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 7, 2013

I think it is a valid configuration, as long as critical pages are only available using SSL (e.g. using the method in #368 (comment)). But my point is that this is an issue that has nothing to do with the Secure attribute inference, but has a broader scope. And it's something where the framework can surely help a lot to not let developers run into such traps, so the discussion in #379 seems more relevant.

@ilya-stromberg
Copy link
Contributor Author

Yes, I agree that problem has broader scope.

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

Successfully merging a pull request may close this issue.

2 participants