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

8.2.2 advice on sessionStorage breaks multi-tab web application use unlike cookies #2010

Closed
jmanico opened this issue Aug 4, 2024 · 40 comments · Fixed by #2051
Closed

8.2.2 advice on sessionStorage breaks multi-tab web application use unlike cookies #2010

jmanico opened this issue Aug 4, 2024 · 40 comments · Fixed by #2051
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR V3 V8 _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@jmanico
Copy link
Member

jmanico commented Aug 4, 2024

The advice to use sessionStorage is really non-standard. It breaks multi-tab browser use.

You can see us talking about this in the cheatsheet series here OWASP/CheatSheetSeries#1458

I suggest we allow localStorage for sessions, but only if the token has both idle and absolute timeout so long terms token storage is less risky.

@Sjord
Copy link
Contributor

Sjord commented Aug 6, 2024

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

Related:

@Sjord
Copy link
Contributor

Sjord commented Aug 6, 2024

In one application I use, the session token is indeed not shared across tabs. When you open a new tab with the application, you are redirected to the SSO provider and automatically logged in into the application. So while the application's token is stored in sessionStorage, it still works with multi-tab browsing because of the interaction with the SSO provider.

@tghosth
Copy link
Collaborator

tghosth commented Aug 6, 2024

I suggest we allow localStorage for sessions, but only if the token has both idle and absolute timeout so long terms token storage is less risky.

So does this restriction automatically rule out storing a JWT in local storage? @jmanico

@tghosth tghosth added _5.0 - prep This needs to be addressed to prepare 5.0 V3 V8 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Aug 6, 2024
@jmanico
Copy link
Member Author

jmanico commented Aug 6, 2024 via email

@jmanico
Copy link
Member Author

jmanico commented Aug 7, 2024

In one application I use, the session token is indeed not shared across tabs. When you open a new tab with the application, you are redirected to the SSO provider and automatically logged in into the application. So while the application's token is stored in sessionStorage, it still works with multi-tab browsing because of the interaction with the SSO provider.

This is very compelling info, thank you for this.

@tghosth
Copy link
Collaborator

tghosth commented Aug 11, 2024

Hi @Tib3rius, I believe you originated this request. As @Sjord pointed out above, there has already been a lot of discussion on this. Nevertheless, I take your point about practicality.

If we talk about using "Local Storage but with tokens that have short expiration times".

  1. What do you consider to be a short expiration time
  2. Do you have a suggestion on how do we handle the refresh token pattern (i.e. where would a refresh token be stored) or is the expectation that users will reauthenticate after the short expiration time?

@elarlang
Copy link
Collaborator

In one application I use, the session token is indeed not shared across tabs. When you open a new tab with the application, you are redirected to the SSO provider and automatically logged in into the application. So while the application's token is stored in sessionStorage, it still works with multi-tab browsing because of the interaction with the SSO provider.

This is vulnerability itself and I addressed it via #1815

# Description L1 L2 L3 CWE NIST §
3.2.5 [ADDED] Verify that creating a session for the application requires the user's consent and that the application is protected against a CSRF-style attack where a new application session for the user is created via SSO without user interaction.

@elarlang
Copy link
Collaborator

The advice to use sessionStorage is really non-standard. It breaks multi-tab browser use.

Why we should mandate multi-tab browser use? sessionStorage is made for a reason - to be more secure than localStorage. For example, can you use reflected XSS or clickjacking against authenticated users or not - do you need to access the correct tab or it work in any opened tab?

I really don't like the idea of saying that they are equal or it does not matter which one of them you use.

@Tib3rius
Copy link

I just wanted to comment here to let you know I've been at DEF CON + some vacation time after but I'll be back tomorrow so will get involved in the discussion then. I think there are some good points that have been brought up already. I'll share my full thoughts tomorrow.

@Tib3rius
Copy link

OK only just getting around to responding here, I mostly blame covid, but apologies.

@tghosth

If we talk about using OWASP/CheatSheetSeries#1458 (comment).
What do you consider to be a short expiration time

I would say it depends on the nature of the app, but an hour would be OK for most. Banking apps etc. could be 15-20 minutes.

Do you have a suggestion on how do we handle the refresh token pattern (i.e. where would a refresh token be stored) or is the expectation that users will reauthenticate after the short expiration time?

I don't really have a suggestion for this tbh. I assume right now the recommendation is to store it in session storage? If that is the case, I guess technically we could still keep it there, the idea being that even though the refresh token is in session storage, if a user does open the app in a new tab, they should be authenticated via the local storage token, which could then be used to fetch a refresh token? That may cause other issues however, especially if we only expect a refresh token to be generated at login, etc.

@elarlang

Why we should mandate multi-tab browser use?

This is less about "mandating" multi-tab browser use, and more about providing realistic security recommendations that apps can use. That is to say, I have no issue with keeping the current recommendations providing we make it clear that they only work for single-tab apps. The larger point is that we should also provide security recommendations for apps which people expect to work in multiple tabs. It is fine, IMO, to state categorically that those security recommendations are less secure / less ideal, but I imagine opening links in new tabs is a feature expected by quite a lot of users, and may also be part of a developer's design for the app.

I really don't like the idea of saying that they are equal or it does not matter which one of them you use.

I agree, and I don't think I was trying to imply that. Clearly session storage is more secure because it's only accessible by a single tab, rather than all tabs with the same origin. However, that increase in security comes with a usability tradeoff that affects session management and makes the current recommendations practically impossible to implement in most apps due to user / dev expectations.

@jmanico
Copy link
Member Author

jmanico commented Aug 21, 2024

The comments from @Tib3rius are astute and well thought-out and makes me re-think our recommendation to use sessionStorage.

@elarlang
Copy link
Collaborator

elarlang commented Aug 21, 2024

Our options here are to say:

  1. It is ok to store session tokens in localStorage - to support multi-tab functionality and to lose security mechanisms provided by sessionStorage

OR

  1. Use sessionStorage over localStorage - you don't have multi-tab support but you have an extra protection (tab scope + tab lifetime) provided by sessionStorage

I think this is not a general decision we can make for ASVS to make everyone happy and this is something to be decided by the application owner based on threat-modeling or risk-analysis (vs functionality expectations).

@Sjord
Copy link
Contributor

Sjord commented Aug 21, 2024

this is something to be decided by the application owner

I think the ASVS should not make a recommendation, but just state what it allows. Currently it says:

with the exception of session tokens which should be stored in either cookies or sessionStorage.

But we could make that less of a recommendation with:

with the exception of session tokens which may be stored in either cookies or sessionStorage.

@tghosth
Copy link
Collaborator

tghosth commented Aug 25, 2024

OK, I spent ages going back through old discussion and guidance on this.

In summary, I think we need to stop being prescriptive about this particular point because there is no one true answer and any secure system requires a combination of controls.

Instead, we should aim for a requirement which details the goals that need to be achieved rather than implementation detail.

I would therefore suggesting bringing back the original requirement 3.2.3 in a less prescriptive way. We would also need to modify 8.2.2 to avoid any contradictions.

My DRAFT suggestion would be:

# Description L1 L2 L3 CWE NIST §
3.2.3 [MODIFIED] Verify that the application's session management model uses a combination of controls around secure storage, identifier expiration and identifier rotation to avoid session identifiers being stolen through attacks such as cross-site scripting or direct access to the browser. 539 7.1
# Description L1 L2 L3 CWE
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 accordance with the application's session management model. 922

I would also suggest providing the following supplemental background guidance which talks about different storage mechanisms:

@tghosth tghosth added the next meeting Filter for leaders label Aug 25, 2024
@jmanico
Copy link
Member Author

jmanico commented Aug 25, 2024 via email

@elarlang
Copy link
Collaborator

I'm ok to make 8.2.2 more abstract.

I can not see the point or the need for proposed 3.2.3.

@jmanico
Copy link
Member Author

jmanico commented Aug 25, 2024

I think that is very reasonable suggestion from @elarlang - I like the "new" abstract 8.2.2 as well.

@tghosth
Copy link
Collaborator

tghosth commented Aug 26, 2024

@elarlang @jmanico I just feel like we should say something about session identifier storage, no?

@Tib3rius
Copy link

@elarlang

I can not see the point or the need for proposed 3.2.3.

I think currently it says session tokens should only be stored in secured cookies or session storage. Whereas the proposed 8.2.2 implies that session tokens can be stored in local storage provided the session management model allows for it. That would seem like the contradiction @tghosth hinted at.

@jmanico
Copy link
Member Author

jmanico commented Aug 26, 2024 via email

@elarlang
Copy link
Collaborator

I would therefore suggesting bringing back the original requirement 3.2.3 in a less prescriptive way

The original 3.2.3:

Verify the application only stores session tokens in the browser using secure methods such as appropriately secured cookies (see section 3.4) or HTML 5 session storage.

Proposed 3.2.3 - it is quite far away from the original one and tries to "fix the world" without actually fixing anything:

Verify that the application's session management model uses a combination of controls around secure storage, identifier expiration and identifier rotation to avoid session identifiers being stolen through attacks such as cross-site scripting or direct access to the browser.

  • combination of controls - does not say, what it is—testability problem.
  • identifier expiration - JWT specific, it sends the wrong signal if cookies are used, to use cookie expiration as session lifetime
  • identifier rotation to avoid session identifiers being stolen - how rotating defends identifiers being stolen via XSS? It is against fixation for cookies and for a lifetime for JWT
  • if there is XSS, you most-likely can steal tokens. If you can not steal, you can force victim browser to make the requests you want using valid session values and mechanism
  • direct access to browser - if you can access the browser, you can access whatever is there

@elarlang
Copy link
Collaborator

@elarlang @jmanico I just feel like we should say something about session identifier storage, no?

I'm not against the idea to bring back 3.2.3 or to address the problem. I'm against proposed new 3.2.3. Maybe let's start with the question: what problem it should solve and then we can find a suitable requirement text.

@jmanico
Copy link
Member Author

jmanico commented Aug 26, 2024 via email

@elarlang
Copy link
Collaborator

elarlang commented Aug 26, 2024

Hey Tiberius, would you be willing to work on https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html

This issue is spin-off from OWASP/CheatSheetSeries#1458 (comment) (linked by yourself) so you can carry another project-specific questions there.

@ryarmst
Copy link
Contributor

ryarmst commented Aug 26, 2024

Regarding 3.2.3, based on related discussions, it seems recommending a Session Storage approach to mitigate XSS has been abandoned due to the fact that XSS is essentially "Game over" as I believe it was said. Therefore, the remaining concern appears to be browser persistence of session identifiers as per @elarlang's comment here:

Difference comes, when users closes a browser without properly log out (it may also happen with hard reset like computer crash, losing electricity etc). If you then reopen the same workstation - question is, should valid token be available in that situation or not (we don't take in account browser own recovery mechanisms here).

Session expiration/timeout does mitigate the risk (as captured by other requirements), but perhaps there is a case for some (L3?) apps to be more strict in client-side storage of sessions?

@tghosth
Copy link
Collaborator

tghosth commented Aug 26, 2024

it seems recommending a Session Storage approach to mitigate XSS has been abandoned due to the fact that XSS is essentially "Game over" as I believe it was said

@ryarmst So I think that it is less a case of XSS being game over and more the fact that from a functionality perspective it is problematic to store the identifier in session storage because it doesn't persist even between different browser tabs.

Maybe let's start with the question: what problem it should solve and then we can find a suitable requirement text.

@elarlang so I think we started off by solving the problem of 'how do you "securely" store session tokens' but I guess the real problem to solve is 'how do you prevent session tokens being stolen and misused' which makes me think that short lived tokens is a better option although that then leads to questions around the refresh token pattern...

perhaps there is a case for some (L3?) apps to be more strict in client-side storage of sessions?

@ryarmst I agree conceptually but I am not what that stricter guidance would look like. It also occurs to me that the main problem with local storage is that it is long lived which is in some ways irrelevant because the identifier should be expired at the server side anyway.

@ryarmst
Copy link
Contributor

ryarmst commented Aug 26, 2024

@tghosth To clarify, my original point was just confirming that the problem to address is not XSS. As far as multi-tab support, I mentioned here that there are apps already that accept this limitation (admittedly, these are annoying to use) BUT there may also be patterns to implement multi-tab support with Session Storage (disclaimer: I have not thoroughly investigated these and have never encountered them in actual apps).

Regarding your point about server side expiration, you could similarly say that Content-Security-Policy is irrelevant because XSS issues should be addressed where the injection vulnerability occurs. I think strictly controlled client storage of session identifiers is similarly a defense in depth approach (for the most part). That said, CSP by comparison is certainly a more effective control practically and implementing proper session expiration should be much easier than mitigating all XSS vectors.

@tghosth
Copy link
Collaborator

tghosth commented Aug 27, 2024

@ryarmst thanks, that is useful to have a couple of examples for that.

So based on that, we could bring back 3.2.3 but make it L3 only. What do others thing @jmanico @elarlang @Tib3rius.

The proposal could be as follows:

# Description L1 L2 L3 CWE NIST §
3.2.3 [MODIFIED] Verify that the application only stores session identifiers in the browser using secure methods such as session storage, web workers, JavaScript closures or appropriately secured cookies. 539 7.1
# Description L1 L2 L3 CWE
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 for which guidance is provided in the session management chapter. 922

@jmanico
Copy link
Member Author

jmanico commented Aug 27, 2024

I did some additional thinking here and have a lot of suggestions. I'll keep it brief but please consider. If this too far off-track please disregard. I think this is really hard to get right.

3.2.3: Verify that the application only stores session identifiers in the browser using secure methods such as session storage, secured cookies (with Secure, HttpOnly, and SameSite attributes), and ensure that any JavaScript handling session data is protected against cross-site scripting (XSS) attacks through proper output encoding and Content Security Policies (CSP). Web workers and JavaScript closures should be used to isolate sensitive computations, but not for long-term session identifier storage.

8.2.2: Verify that data stored in browser storage (such as localStorage, sessionStorage, IndexedDB, or cookies) does not contain sensitive data. If sensitive data must be stored, ensure it is encrypted and integrity-protected, and that it is only stored temporarily with appropriate expiration mechanisms. Exceptions are made for session identifiers, provided their handling follows other guidance in the session management section, including the use of secure cookies and robust protection against cross-site scripting (XSS) attacks.

@tghosth
Copy link
Collaborator

tghosth commented Aug 27, 2024

I think for 3.2.3 I prefer the shorter text as other requirements are covered elsewhere.

For 8.2.2 I think the idea of allowing encrypted data is interesting. It leads to a key management problem but may still be worthwhile a a cut out, I think I would carve this out as a separate question, opened #2029.

@ryarmst
Copy link
Contributor

ryarmst commented Aug 27, 2024

What about abstracting it to the level of the problem (just focusing on locally persisted identifiers here):

3.2.3: Verify that active session identifiers are not persisted when the associated browsing contexts are closed, such as when a user closes their browser or restarts their system.

This does not impose a mechanism and the active qualifier provides some additional flexibility.

@tghosth
Copy link
Collaborator

tghosth commented Aug 29, 2024

So that is a possibility although that doesn't necessarily highlight the specific problem of session theft.

Also, would this still be L3 requirement only @ryarmst?

@ryarmst
Copy link
Contributor

ryarmst commented Aug 29, 2024

Good point, I'll suggest the following then, which I think makes the purpose more clear:

3.2.3: Verify that active session identifiers are not persisted when the associated browsing contexts are closed, such as when a user closes their browser or restarts their system, preventing client-side session theft following context termination.

In terms of setting the level (or adding this requirement at all), I would like to hear more feedback from others. This particular issue has an extensive discussion history (mostly relating to Session Storage). The requirement above avoids mandating an implementation, but may be impractical in some scenarios.

@jmanico
Copy link
Member Author

jmanico commented Aug 29, 2024

What about remember me type functionality? Even several high risk sites (like Google) keeps the session persisted almost permanently and then requires re-auth for admin level functions. I don't think closing the browsing context should always kill a session, especially when re-auth is in play properly.

Another example is a popular payment app. I can stay logged in for months and I just need to re-auth before issuing a payment.

And I don't mean to keep throttling these conversations. I just find that almost any advice on session management is hard.

@ryarmst
Copy link
Contributor

ryarmst commented Aug 29, 2024

@jmanico does it make sense to add an exception for when sessions must be persisted in this way, or would this make the requirement pointless? Should user informed consent for Remember Me be a factor? I didn't see a reference to Remember Me functionality in the ASVS, but this is an item we have reported for critical/sensitive applications:

Warning that use of the function is less secure and should only be used on a private system

The scenarios you mention mitigate the risk of extended/persisted sessions with re-auth/step up and I have also seen a number of apps use additional detection heuristics to identify session theft. Would it make sense to split requirements based on the intended lifetime of sessions?

@elarlang
Copy link
Collaborator

3.2.3: Verify that active session identifiers are not persisted when the associated browsing contexts are closed, such as when a user closes their browser or restarts their system, preventing client-side session theft following context termination.

I agree with that context and I have pointed out this myself, but I'm not sure how realistic it is nowadays, as browsers support "restore previous session" or "restore previous tab" functionality, that kind of ignores the point for "session cookies" or even sessionStorage mechanisms.

As we have separate sections for cookies and tokens, we can also consider specific requirements based on technology used.

@jmanico
Copy link
Member Author

jmanico commented Aug 29, 2024 via email

@ryarmst
Copy link
Contributor

ryarmst commented Aug 29, 2024

I agree with both of your points: this is a low value requirement and is challenging to ensure/implement in practice. @elarlang I will explore technology-specific requirements if any make sense.

To keep things moving, I will suggest we simply leave 3.2.3 as it was (deleted) for now.

@tghosth
Copy link
Collaborator

tghosth commented Sep 1, 2024

Ok, I think being non-prescriptive is the only thing that makes sense at this point and hopefully the other session management requirements will cover us.

As such, I think the final proposal is that 3.2.3 stays deleted and we modify 8.2.2 as follows:

# Description L1 L2 L3 CWE
8.2.2 [MODIFIED, MERGED FROM 3.2.3] 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. 922

Thanks all for a thoughtful discussion :)

Any further comments @elarlang @jmanico @ryarmst ?

@elarlang elarlang added 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR and removed next meeting Filter for leaders labels Sep 1, 2024
@jmanico
Copy link
Member Author

jmanico commented Sep 2, 2024

After this long and helpful conversation, I think your conclusion is spot on, @tghosth :)

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 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR V3 V8 _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants