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

Questions on rigidity of token-based session storage, "sessionStorage for token-based sessions" guidance in 8.2.2 #1141

Closed
drschulz opened this issue Dec 1, 2021 · 22 comments · Fixed by #1321
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet

Comments

@drschulz
Copy link

drschulz commented Dec 1, 2021

Hi all, I read through #843 and #1091 to see that the bleeding edge has some exceptions for token-based session storage.

From the change to 8.2.2 it appears (from my understanding) that two types of sessions are mentioned:

  • cookie-based sessions: Sessions where an application server sets the cookie with a Set-Cookie header (HTTP-Only), and the domain of the cookie is the recipient of authorized API requests. There is no reason why client JS would need to access the session, which is the basis for the clarification in 8.2.2.
  • token-based sessions: Tokens used by browser clients (delegated access token, for example) that are attached as Authorization headers to outbound CORS requests. Token storage for these sessions in the browser seems to be a necessary evil, and the guidance is to use sessionStorage.

My question is, the session distinction makes sense, but should sessionStorage be mentioned here at all?

Storage for token-based sessions comes up in the first place because of two persistence needs:

  1. Session retention across page refreshes
  2. Session retention across browser tabs of the same application

The sessionStorage exception only solves persistence need 1, but not persistence need 2. The only path forward for these types of applications is to initiate new token-based sessions per tab, which requires a new login/authorization per tab. This doesn't really make the client more secure from a storage perspective, since XSS can still steal tokens from sessionStorage, and also adds a lot of complexity to the app for session management. It, in my opinion, opens up even more questions (and potentially insecure solutions) about how to securely manage token-based sessions across tabs, which doesn't seem to be covered.

Should there be specific guidance about which browser storage to use for token-based sessions? All browser storage is inherently insecure because of XSS, so what benefit does sessionStorage provide that it is worth being called out?

Storage Susceptible to XSS Susceptible to Token Exfiltration Meets Persistence Need 1 Meets Persistence Need 2 Notes
Web Workers Yes No Accessible across tabs if SharedWorker is used. Potentially complex to set up.
In-memory Closures Yes No (?) - - Only available within current tab.
Session Storage Yes Yes - Only available within current tab.
Cookie - Samesite=Strict, Secure, not HTTP-Only (essentially "stricter" local storage) Yes Yes Expires when token expires if expiration is set (or additionally when browser is closed if set as a session cookie). If used purely for storage, does not open itself up for CSRF, because outbound requests require attached Auth Headers.
Local Storage Yes Yes Accessible within domain. No expiration, needs to be manually cleared by the client, but 8.2.3 covers this.

Should the distinction instead be something along the lines of:

Verify that data stored in browser storage (such as localStorage, sessionStorage, IndexedDB, or cookies) does not contain sensitive data, with the exception of cookie-based session tokens and token-based session tokens, with the former stored only in cookies, following V3.4.

Very curious on thoughts here. Also, let me know if there is another discussion somewhere discussing this.

@elarlang
Copy link
Collaborator

elarlang commented Dec 1, 2021

Nice homework for opening the issue :)

If you put token- based session token to cookie, you probably mixing 2 different ways how to handle it (and you should not do it). Token should not be in cookies, otherwise you'll get CSRF there as well.

Please read those materials and let us know, does it change your view on the topic:

@elarlang elarlang added the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Dec 1, 2021
@drschulz
Copy link
Author

drschulz commented Dec 2, 2021

Thanks Elar :), been doing a lot of reading. From what I understood, those articles discuss JWT in the context of a traditional web application, where cookie-based sessions would be used. Within that context, the points in that article make sense and I agree.

I'm more concerned with access tokens received from Single Page Apps and OAuth flows (or any "serverless" app), whether they're a JWT or not. Essentially, for SPAs that make API calls cross domain, they need to store the access token somewhere in the browser. The table listed above demonstrates that no browser storage options are immune to XSS issues (further outlined really nicely in https://www.pingidentity.com/en/company/blog/posts/2021/refresh-token-rotation-spa.html and https://pragmaticwebsecurity.com/talks/xssoauth.html) so I'm curious why sessionStorage is isolated as the go-to for this exception? Sort of along the lines of this article written last year, but perhaps opinions have changed?

Unless a proper BFF is implemented for the app in question, the only option left is some flavor of browser storage, and they're all insecure.

Also, I wanted to clarify that the cookie row in the table is implying that client JS is creating/setting the cookie, not a web server. In token-based applications with CORs API calls, I'm curious how CSRF is still a concern. If Domain A, web server or CDN, is not serving APIs for authorized data under its own domain (A), and the client is making API calls to Domain B with auth headers, where is the CSRF concern?

I suppose I'm just curious how sessionStorage + silent iframe token refreshes are a better recommendation than local storage or a non http-only cookie, specifically for token-based apps? All three are subject to exfiltration.

@elarlang
Copy link
Collaborator

elarlang commented Dec 2, 2021

XSS is not the only problem in security field :)

token-based session token storage in cookie vs localStorage vs sessionStorage discussion we had in issue #843 (comment). Please (re)read this one first (as my arguments are provided there). If there is some new angle to provide, we can discuss it here.

CSRF is not concern when extra request headers are involved, but my question here stays - why you need cookies AND headers in this case?

@willfarrell
Copy link

One idea I've been thinking on for a while is to store the session in memory inside of a service worker. SharedWorker are not supported in Safari, making Service Workers the only way to share cross-tabs. I've only see this approach mentioned once (https://blog.ropnop.com/storing-tokens-in-browser/#service-worker). Now that service worker have been in all browsers for a few years now, should this be considered as well for OWASP and ASVS?

@elarlang
Copy link
Collaborator

elarlang commented Dec 2, 2021

Service workers have been in discussion in #843.

I think from this current issue point of view, it's separate topic - if you store it in memory, it does not require exception in 8.2.2.

If you have clear idea for proposal, how service workers could be involved in ASVS, please open separate issue for that.

@drschulz
Copy link
Author

drschulz commented Dec 3, 2021

token-based session token storage in cookie vs localStorage vs sessionStorage discussion we had in issue #843 (comment).

Those points definitely make sense for local storage, especially when the browser closes without a logout/cleanup. I'm not saying that I'm advocating for the cookie approach, but if expiration is set as a browser session cookie by the client, it does not suffer the same post browser session issues of local storage (it would get removed on browser close).

why you need cookies AND headers in this case?

In that sort of approach, utilizing the cookie is purely a storage mechanism for the client and nothing else. The API calls would not to the domain with the cookie, but cross origin with a header (similar to your comment #843 (comment)). Not immune to XSS (among other attacks) but I thought it was worth mentioning in the table as a storage option for the client.

I suppose I'm wondering why any browser storage is an exception. To me, it seems like ASVS should either (for token-based sessions):

  1. be strict against all browser storage mechanisms (last three in the table), recommending closures/workers
  2. not be opinionated about the type of browser storage.

Since sessionStorage doesn't give any UX benefits over closures (besides reload retention), and is an easier attack vector, it seems strange that it's mentioned. This is why I was curious if there was a specific reason for the addition.

On the other hand, I wonder if the exception will hold now that third party cookies are soon going away, and SSO hidden iframe refresh protocols are starting to break, making new tabs and page refreshes an awkward experience at best (I even saw that Auth0 sdk now has a local storage option because of these problems). Things are looking pretty bleak for access token options in client JS 😬 .

The service worker idea is interesting and would be great to discuss in another issue.

@elarlang
Copy link
Collaborator

elarlang commented Dec 4, 2021

In that sort of approach, utilizing the cookie is purely a storage mechanism for the client and nothing else.

If you need it only for the client, why to store it in a cookie (which is sent to server with every request)?

If something is wrong or outdated in some requirement, then we need to change it. At the moment I don't see the reason, yet.

Take a look here as well: https://restfulapi.net/

Restful means, that you don't have any state - in other words, you should not have cookies involved.

You may have cookie-based session to SSO and token-based session to an application, but even in this case you don't need to store cookie-based session token to localStorage and token-based session token to cookie.

Recommending closures/workers may happen in the future, at the moment it does not seem realistic to require it. Personally I have not seen them in use in application which I pentest daily.

@jmanico
Copy link
Member

jmanico commented Dec 4, 2021 via email

@jmanico
Copy link
Member

jmanico commented Dec 7, 2021

Dear @drschulz and @elarlang I believe that once you have XSS in an app it's a critical event in relation to session functionality. I can use XSS to either steal or force the use of any browser-based session mechanism and overall I do not believe differentiating between whether or not the token can be stolen is very useful because even if I cannot steal a cookie I can always abuse it (ie: force its use) with XSS.

I also see use cases for storing session data in cookies due to some of their advanced protections. I also see validity with using LocalStorage (long term), SessionStorage (short term), and other mechanisms. Even closures and web workers can be abused with XSS, btw.

Now on that note, I think 8.2.2 is not reasonable as is.

8.2.2 [MODIFIED] Verify that data stored in browser storage (such as localStorage, sessionStorage, IndexedDB, or cookies) does not contain sensitive data, with the exception of cookie-based session tokens in cookies and token-based session tokens in sessionStorage. 922

And I suggest we change it to:

8.2.2 [MODIFIED] Verify that data stored in browser storage (such as localStorage, sessionStorage, IndexedDB, or cookies) does not contain sensitive data, with the exception of session artifacts. 922

Because ANY place you store tokens in the DOM is susceptible to XSS and I do not think we should be prescriptive about session storage in the browser when all of the options are bad in the face of XSS.

@elarlang
Copy link
Collaborator

elarlang commented Dec 7, 2021

As they are equal against XSS, please keep XSS away from the discussion. And we have had this discussion already: #843 (comment). So no point to go there again.

I do not believe differentiating between whether or not the token can be stolen is very useful because even if I cannot steal a cookie I can always abuse it (ie: force its use) with XSS.

Basically you say that cookie-based session tokens do not need httpOnly attribute? I don't agree with that. IF you have XSS then you can force victim browser to do whatever, but it's a bit more complicated compared to session hijacking/fixation. Risk is different as likelihood component is different, an attacker needs more time/information to make attack. If we can keep away one attack vector, it makes sense to do it.

If we now allow to store cookie-based tokens in every storage, it's again conflict between 8.2.2 and 3.4.* and we will start all over again with the same discussion from another angle like in #843 . My arguments on the topic are provided here: #843 (comment)

If there is need to allow token-based session token anywhere else than sessionStorage, then please provide arguments:

  • which are not related to XSS
  • "I'm using it that way" is not valid argument

Like I wrote already, if there is need to change 8.2.2, we do it. But we need to have as bullet-proof arguments as we can get do not start this discussion again and again.

Before any new comments, please investigate arguments so far in #843, #1091 and in this issue.

@jmanico
Copy link
Member

jmanico commented Dec 7, 2021

 Basically you say that cookie-based session tokens do not need httpOnly attribute? I don't agree with that. 

Yes, to a real adversarial attacker HTTPOnly is useless to stop them from doing critical harm to your app and users. You can still XSS them and abuse the session and force that user to make any request you want them to, with authentication. In fact, stealing the cookie is worse because then you have a signature of that cookie being used on a real IP. So it's even better to abuse the token vs. stealing it.

HTTPOnly is useless when it comes to real-world defense.

As they are equal against XSS, please keep XSS away from the discussion

With respect, I want to encourage everyone to keep talking and discussing.

@jmanico
Copy link
Member

jmanico commented Dec 7, 2021

And I'm still remiss because the phrase "...exception of cookie-based session tokens in cookies and token-based session tokens in sessionStorage..." does not make sense to me. It's a combination of poor English (due to the repetition) and sends the wrong message about the storage of client-side artifacts.

@GavinRay97
Copy link

@drschulz

The sessionStorage exception only solves persistence need 1, but not persistence need 2. The only path forward for these types of applications is to initiate new token-based sessions per tab, which requires a new login/authorization per tab. This doesn't really make the client more secure from a storage perspective, since XSS can still steal tokens from sessionStorage, and also adds a lot of complexity to the app for session management. It, in my opinion, opens up even more questions (and potentially insecure solutions) about how to securely manage token-based sessions across tabs, which doesn't seem to be covered.

I'm not convinced that sessionStorage is any more secure than localStorage provided that the other guidelines are followed, but for what it's worth a way around the multi-tab issue is by syncing sessionStorage securely between tabs using localStorage as a cross-tab event emitter:

// Taken from https://blog.guya.net/2015/06/12/sharing-sessionstorage-between-tabs-for-secure-multi-tab-authentication/
if (!sessionStorage.length) {
    // Ask other tabs for session storage
    console.log("Calling getSessionStorage")
    localStorage.setItem("getSessionStorage", String(Date.now()))
}

window.addEventListener("storage", (event) => {
    console.log("storage event", event)
    if (event.key == "getSessionStorage") {
        // Some tab asked for the sessionStorage -> send it
        localStorage.setItem("sessionStorage", JSON.stringify(sessionStorage))
        localStorage.removeItem("sessionStorage")
    } else if (event.key == "sessionStorage" && !sessionStorage.length) {
        // sessionStorage is empty -> fill it
        const data = JSON.parse(event.newValue)
        for (let key in data) {
            sessionStorage.setItem(key, data[key])
        }
    }
})

@tghosth
Copy link
Collaborator

tghosth commented Apr 26, 2022

Cookies and session storage provide an expiration mechanisms to make sure tokens do not live on forever, even when the application exits unexpectedly. Local storage does not have that which is why we make the differentiation.

We know that there are other possible mechanisms like service workers but they seem to be too complicated to make widespread adoption likely at this point.

@jmanico I would suggest the following to try and maintain both clarity and accuracy.

[MODIFIED] Verify that data stored in browser storage (such as localStorage, sessionStorage, IndexedDB, or cookies) does not contain sensitive data, with the exception of session identifiers which should be stored in either cookies or sessionStorage.

@jmanico
Copy link
Member

jmanico commented Jun 23, 2022

Token should not be in cookies, otherwise you'll get CSRF there as well.

This is not a reasonable suggestion at all. Cookies have significant protections, including automatic same-site CSRF protection, secure flagging, host or secure cookie labels, HTTPOnly, and more that makes them a compelling way to store tokens in browser-based apps.

@jmanico
Copy link
Member

jmanico commented Jun 23, 2022

Cookies and session storage provide an expiration mechanisms to make sure tokens do not live on forever, even when the application exits unexpectedly. Local storage does not have that which is why we make the differentiation.

We know that there are other possible mechanisms like service workers but they seem to be too complicated to make widespread adoption likely at this point.

@jmanico I would suggest the following to try and maintain both clarity and accuracy.

[MODIFIED] Verify that data stored in browser storage (such as localStorage, sessionStorage, IndexedDB, or cookies) does not contain sensitive data, with the exception of session identifiers which should be stored in either cookies or sessionStorage.

I like this suggestion, Josh.

@jmanico
Copy link
Member

jmanico commented Jun 23, 2022

The complete change would be:

8.2.2 | [MODIFIED] Verify that data stored in browser storage (such as localStorage, sessionStorage, IndexedDB, or cookies) does not contain sensitive data, with the exception of session identifiers which should be stored in either cookies or sessionStorage. | ✓ | ✓ | ✓ | 922

tghosth added a commit that referenced this issue Jul 26, 2022
@tghosth tghosth linked a pull request Jul 26, 2022 that will close this issue
@tghosth
Copy link
Collaborator

tghosth commented Jul 26, 2022

Opened PR to resolve this, @jmanico please review :)

@elarlang
Copy link
Collaborator

elarlang commented Aug 8, 2022

current PR is in conflict with HttpOnly requirement (3.4.2).

  • the point of 3.4.2 is that session token should not be readable for javascript
  • 8.2.2 now allows to store session token in localStorage and sessionStorage which is readable for JavaScript

If you use Authorized header for sending session token, it must be readable for JavaScript - in this case it does not matter, where it is stored, because in XSS situation it is readable anyway.

If you use session cookie, then httpOnly flag gives extra defence - you can not read the token out in XSS situation. Yes, you can force victim browser to make requests etc, but for that you need to have some extra time and knowledge, it's more complicated/slower attack scenario to build than simple-stupid session takeover by stealing the token.

But I feel this topic and discussion is in endless loop long time already :)

@tghosth
Copy link
Collaborator

tghosth commented Aug 24, 2022

Hi @elarlang

We are not making a blanket rule that "session token should not be readable for javascript" because that would not be possible :) Therefore, that is not the point of 3.4.2.

The point 3.4.2 is that when a session token is stored in a cookie, it should be marked as HttpOnly which is practically speaking a defense in depth control but a very easy one to implement.

As such, I do not believe 3.4.2 is in conflict with 8.2.2.

What do you think?

elarlang pushed a commit that referenced this issue Oct 28, 2022
@regoravalaz
Copy link

In the end, there is no secure mechanism.

If you use refresh tokens in Cookies and web token in Storage, as recommended, what will be achieved is complicate the process of accessing the information.

That means that what ultimately matters is the motivation of the attacker. How motivated are you to invest time in resolving both sides of the proposal.

Because if you are really motivated, you can use the two techniques to handle the 2 values that are being saved in Coockies and Storage.

I think we need to work on a radical change in the Web so that this does not continue to be a problem for dedicated or lazy attackers.

@elarlang
Copy link
Collaborator

I don't know how you ended up here when the issue is 1.5 years closed, but note that your comments most likely are ignored in closed issues.

When we talk about OAuth refresh tokens, those should never be sent to the client anyway:

In this issue context - if you have an JavaScript execution, you can not automate victim's browser without ever knowing the session token, so I agree with:

In the end, there is no secure mechanism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants