meteor / meteor-feature-requests

A tracker for Meteor issues that are requests for new functionality, not bugs.
Other
89 stars 3 forks source link

[Accounts][security] Use cookies instead of localStorage for storing session token #372

Open StorytellerCZ opened 4 years ago

StorytellerCZ commented 4 years ago

An issue came up on the forums where Meteor app audit highlighted storage of session tokens in localStorage as a security issue. Setting up content security policy is not seen as enough to mitigate this issue. As such it was recommended to use cookies instead.

Requests #264 and #295 have some aspects of this as well.

The reasoning and additional information are in the above mentioned forum topic. I have created this issue to figure out how this change could be implemented. I think there are two options. Either add a new option via cookies or switch to cookies (or other storage method) altogether.

mitar commented 4 years ago

So the only issue here is that localStorage does not have equivalent to httpOnly? Because all other aspects of this list hold for both cookies and local storage?

I in fact prefer local storage because it is linked to origin exactly, while cookies are shared also for subdomains.

andsnw commented 4 years ago

So the only issue here is that localStorage does not have equivalent to httpOnly? Because all other aspects of this list hold for both cookies and local storage?

Yep, that looks like the only benefit in the scenario of XSS where the scope is limited to browser and authentication.

alan-pl commented 4 years ago

Also path, expiration, and design-intent

mitar commented 4 years ago

Path does not work really well with single-page apps or in-app routing. So not sure how does it apply here.

Expiration makes no sense because expiration can and should be done on the server side anyway. You should not trust or rely on client expiring cookies.

Design-intent is speculative. Reference missing.

jonlachlan commented 4 years ago

I in fact prefer local storage because it is linked to origin exactly, while cookies are shared also for subdomains.

According to this the default behavior does not include subdomains, unless you include Domain=<domain-value> directive:

Domain=<domain-value> |Optional
    Hosts to where the cookie will be sent.

        * If omitted, defaults to the host of the current document URL, not including subdomains.
        * Contrary to earlier specifications, leading dots in domain names (.example.com) are ignored.
        * If a domain is specified, subdomains are always included.
mitar commented 4 years ago

@jonlachlan Interesting. Didn't know that. This is neat. I hope browser have picked up this, it seems it is new specification?

There is though a small but sometimes important difference though between "host" and "origin". The later includes the schema and port. Local storage uses the origin. Host is broader.

Anyway, thanks for this info. Very useful.

stolinski commented 4 years ago

"WebKit will delete all local storage after 7 days" https://news.ycombinator.com/item?id=22683535

Obviously the title here is a bit sensationalist and not 100% accurate but there are some iffy new changes coming to how WebKit handles local storage.

filipenevola commented 4 years ago

Hi, I was thinking about this and I think one solution would be a code like this in the client:

import Cookies from 'js-cookie';

import { Meteor } from 'meteor/meteor';
import { Accounts } from 'meteor/accounts-base';

const LOGIN_TOKEN_KEY = 'Meteor.loginToken';
const LOGIN_TOKEN_EXPIRES_KEY = 'Meteor.loginTokenExpires';
const USER_ID_KEY = 'Meteor.userId';

const LOCAL_STORAGE_KEYS = [
  LOGIN_TOKEN_KEY,
  LOGIN_TOKEN_EXPIRES_KEY,
  USER_ID_KEY,
];

Meteor.startup(() => {
  Accounts.onLogin(() => {
    LOCAL_STORAGE_KEYS.forEach(key => {
      Cookies.set(key, Meteor._localStorage.getItem(key), {
        secure: true,
        sameSite: 'lax',
      });
    });
  });
});

Of course with more cases (logout, etc) and maybe in a more hacky way (I've found this code after I talked with @stolinski about this problem)

The code above copies localStorage data to cookies and then in the next request you could get them like below

app.get(
  '/log-cookies',
  Meteor.bindEnvironment((req, res) => {
    console.log('Cookies: ', JSON.stringify(req.cookies));
  })
);

Would result in a log (in the server) like this

Cookies:  {"Meteor.loginToken":"z2btvmYPHHRBm7TN3o55EfYqjBZi-pnCwM6BVvYT6aF","Meteor.loginTokenExpires":"Wed Jul 22 2020 11:14:22 GMT-0400 (Amazon Standard Time)","Meteor.userId":"6RQZeppKa53upoE8T"}

The proposed solution could be integrated into meteor base accounts package or even published as a third-party package (accounts-auth-cookies).

The only limitation that I see in this solution is that you can't get the auth information from the cookie in the same round-trip because we can't write cookies in a Websocket connection (https://stackoverflow.com/questions/39191517/set-cookie-inside-websocket-connection).

What do you think?

mullojo commented 4 years ago

That looks like a super easy solution for the cookies route @filipenevola πŸ‘

This is not a cookie solution, but I came across a really cool npm package πŸ“¦ for local browser storage that creates a universal api for the complexity of various browsers.

Github: https://github.com/sarahdayan/browserstore.js npm: https://www.npmjs.com/package/browserstore.js

The big drawback to cookies in my view is users who disable them or browsers settings that block them by default. And it seem that you are required to tell people about cookie usage & get their verification per GDPR standards (see here)

So it seem that cookies should be avoided if possible 😎 But this is definitely a personal choice.

stolinski commented 4 years ago

The big issue of avoiding cookies is not having the token available to for SSR

jonlachlan commented 4 years ago

@filipenevola

This looks to me like you're setting 3 cookies that are all session cookies (removed when browser/tab closes). Why not set 1 cookie with expires with the login token as the value, and omit the user id (as the client does not need it)?

Also, you'd gain some security benefits with httpOnly.

mjmasn commented 4 years ago

@mullojo just FYI localstorage === cookies as far as EU Cookie Law / GDPR is concerned

jonlachlan commented 4 years ago

@filipenevola

The only limitation that I see in this solution is that you can't get the auth information from the cookie in the same round-trip because we can't write cookies in a Websocket connection (https://stackoverflow.com/questions/39191517/set-cookie-inside-websocket-connection).

WebSocket is designed to work alongside HTTP, so Node has an httpServer.on('upgrade', callback) event listener. I haven't confirmed this but I think you can process cookie information before you set a stateful connection.

mullojo commented 4 years ago

It looks like some previous thought had gone into this Meteor design decision, so let's NOT just "Use cookies instead of localStorage for storing session token", as the title suggests in this issue.

Meteor wrote an article on it this exact topic called Why Meteor doesn’t use session cookies

Cookies generally have more security vulnerabilities, so they should be used more cautiously than localStorage, see the Meteor article for some detailed examples.

The Meteor article ended with comments that SSR might result in the Meteor packages needing to use cookies, but in addition to localStorage. But maybe SSR should still not store the users "Meteor.loginToken" in a cookie, unless it gets redesigned with some security expertise.

A user would have to be on your physical machine to exploit localStorage, so cookies would be equally vulnerable to that type of attach, and would possibly open up new types of sophisticated software attacks.

Using SSR seems to have it's trade-offs, so not everyone will use it. SSR is implemented differently with Blaze, React, Vue, Svelte, etc. I'm the most familiar with Vue's SSR package, which worked nicely in a test, but I'm not using it in any projects. Looking at the Vue SSR package dependencies, it relies on an updated fast-render package, which subsequently relies on a simple cookie package.

In the staringatlights:fast-render package πŸ“¦ there is a full security disclosure describing the extra security vulnerabilities that might be caused by replicating the session token in cookies. https://atmospherejs.com/staringatlights/fast-render#security

Cookies in Chrome Dev Tools https://developers.google.com/web/tools/chrome-devtools/storage/cookies They describe this sameSite: 'lax', as experimental πŸ›‘(caution)

jonlachlan commented 4 years ago

Cookies generally have more security vulnerabilities, so they should be used more cautiously than localStorage, see the Meteor article for some detailed examples.

This is completely unfounded and incorrect by my understanding.

mullojo commented 4 years ago

@jonlachlan, I'm summarizing from the Meteor Security Post, a couple of those extra known vulnerabilities are Cross-site request forgery (CSRF) & Cookie Tossing. Please read the article and you'll advance your understanding.

Emily Stark who wrote the article and also gave feedback on the fast-render package in the security link I refer to now works on the Google Chrome team in Security: https://emilymstark.com/. Her expertise is in client side cryptography for web apps with degrees from Stanford & MIT, so I think we should go with her understanding πŸ‘

jonlachlan commented 4 years ago

@mullojo I'll let you fall on your sword on this one.

StorytellerCZ commented 4 years ago

From the article I really like this proposal:

Meteor could require two tokens to authenticate a DDP connection: both a httpOnly cookie and a localStorage token.

mitar commented 4 years ago

So I still do not understand what we gain from having both the cookie and local storage, or why the cookie improves anything for single-page apps.

In comment above we determined that the only difference is:

So the only issue here is that localStorage does not have equivalent to httpOnly? Because all other aspects of this list hold for both cookies and local storage?

Yep, that looks like the only benefit in the scenario of XSS where the scope is limited to browser and authentication.

But for single-page apps this makes no sense. I want JavaScript to have access because this allows me to sign-in the user and sign-out the user without having to reload the app.

jonlachlan commented 4 years ago

But for single-page apps this makes no sense. I want JavaScript to have access because this allows me to sign-in the user and sign-out the user without having to reload the app.

Reloading isn't the issue, as client-side reactivity will handle rendering. The client only needs isLoggedIn for rendering. The token is for first-loads when establishing a stateful connection when a user returns, or for authorization of stateless http calls. Yes you can use Javascript and localStorage to do this, as well as you can use cookies which are sent with an http request, and can be available to JavaScript or can be restricted to not be available to JavaScript.

stolinski commented 4 years ago

So I still do not understand what we gain from having both the cookie and local storage, or why the cookie improves anything for single-page apps.

We need this to have the user data available for server side rendering.