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

V3.2.3 Session storage #843

Closed
bretik opened this issue Sep 23, 2020 · 117 comments
Closed

V3.2.3 Session storage #843

bretik opened this issue Sep 23, 2020 · 117 comments
Assignees
Labels
3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos

Comments

@bretik
Copy link

bretik commented Sep 23, 2020

3.2.3 states "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."

I see this was changed in #553 - I would like to reopen the discussion about session storage being a secure location for storing session identifiers.

  1. NIST 800-63B 7.1:
    Secrets used for session binding SHOULD NOT be placed in insecure locations such as HTML5 Local Storage due to the potential exposure of local storage to cross-site scripting (XSS) attacks.

I believe HTML5 Local Storage here refers to both window.localStorage and window.sessionStorage from the "Web Storage API", since both are prone to XSS

  1. HTML5 Security Cheat Sheet
    Also known as Offline Storage, Web Storage....
    A single Cross Site Scripting can be used to steal all the data in these objects, so again it's recommended not to store sensitive information in local storage.

The HTML5 Security Cheat Sheet refers to sessionStorage only in one place and states, that it is better to use sessionStorage instead of localStorage in case where you do not need persistence.

  1. 8.2.2
    Verify that data stored in browser storage (such as HTML5 local storage, session storage, IndexedDB, or cookies) does not contain sensitive data or PII.
@jmanico
Copy link
Member

jmanico commented Sep 23, 2020 via email

@jmanico
Copy link
Member

jmanico commented Sep 23, 2020 via email

@elarlang
Copy link
Collaborator

elarlang commented Sep 23, 2020

And where you going to hold your JWT tokens? :)

My argumentation on that topic: #696 (comment) and it's waiting for ASVS v4.1

@jmanico
Copy link
Member

jmanico commented Sep 23, 2020 via email

@elarlang
Copy link
Collaborator

This change is no-no-no.

If you make this change, it's clear duplicate for section 3.4 or it has basically meaning: "check section 3.4". There is no need for this kind of requirement.

Seems, that you have not taken your time to read my comment and discussion in other issue on that topic, so I copy-paste it here, from #696:

In practice, maybe we need to reorganise the idea for this requirement with V3.2.3.

V3.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.

It has the same problem - use correct settings for cookie or use HTML5 session storage. And we reach to one setting - is HttpOnly flag for cookie important or not. In other words - should session token be readable for JavaScript or not. Should we have this extra mitigation for "XSS situations" or not. If you have XSS on site... your active session is usable for attacker via your browser anyway.

I think, we need to check separately "Cookie-based" and "Token-based" session management for those requirements.

If we have "Cookie-based Session management"

  • V3.1.1 should have meaning "do not use session token anywhere else then httponly flagged cookie"
  • V3.2.3 should not allow using session token in session storage in this case

If we have "Token-based Session management"

  • V3.1.1 should have meaning "do not spread session token to URL"
    • but if the value used by JavaScript with extra header (like Authorization) for API it is anyway readable for JavaScript, do you display it to the output does not even matter in practice
    • meaning is covered by V8.3.1
  • V3.2.3 basically says, store it in session storage. But here is another conflict - if we consider session token as sensitive data, then it does not match with V8.2.2

V8.2.2 Verify that data stored in client side storage (such as HTML5 local storage, session storage, IndexedDB, regular cookies or Flash cookies) does not contain sensitive data or PII.

And then there are solutions, where Token is used in cookie... where we should have question, do you use token-based session management correctly.

My current conclusions/ideas:

  • I don't see point for current V3.1.1
    • if needed, modify "V8.3.1 Verify that sensitive data (including session token) ..."
  • I don't see point for current V3.2.3
    • Add new requirement to Cookie-based Session Management with meaning "session token is allowed to send only with httponly cookies" (but isn't it too strict?)
    • If token-based session should be able to use cookies, move current V3.2.3 to Token-based requirement. If token-based session should not use cookie, use only "session storage part" from V3.2.3

@bretik
Copy link
Author

bretik commented Sep 24, 2020

Sorry, I should have reacted to your comment before creating the PR. This change is only quick fix to remove the unsecure recommendation. #696 will not be resolved by this change.

If token-based session should not use cookie, use only "session storage part" from V3.2.3

If we take NISTs term "Secrets used for session binding" as both cookie-based and token-based, we cannot recommend using session storage, it would be in direct conflict with that recommendation.

Add new requirement to Cookie-based Session Management with meaning "session token is allowed to send only with httponly cookies" (but isn't it too strict?)

Requirements in V3.4 do not give me options how to store the session cookie other than a cookie with HttpOnly, Secure and SameSite attributes, with __Host prefix and precise path

If token-based session should be able to use cookies, move current V3.2.3 to Token-based requirement

I agree with the statement, that requirement V3.2.3 will be redundant for V3.4 after the change (but not in conflict with it) and could be moved to V3.5.

@jmanico
Copy link
Member

jmanico commented Sep 24, 2020 via email

@elarlang
Copy link
Collaborator

elarlang commented Sep 24, 2020

If session token handling is done with cookies, then it's no-brainer to not talk about keeping this value in sessionStorage or localStorage.

Now, if session token is sent to API with bearer header (for example), then where you going to keep session token value (or JWT) on the client-side? With this change you need to solve that situation also.

From attacking perspective - IMO stealing session token and then using it yourself is a bit yesterday. I could automate victim browser with some nice XSS "main in the browser" payload and make victim browser just a gateway. Immediate and effective. You can force victim browser to do everything without making requests to some application from your own IP. I recommend to use HttpOnly for session cookies, but on the other hand - you just need to be sure that you don't have an XSS, otherwise you are <beep> anyway.

@ropnop
Copy link

ropnop commented Sep 24, 2020

Came here from Twitter, but wanted to chime in in a more organized fashion:

I agree the wording on 3.2.3 may be problematic, since it seems to imply that HTML5 Session Storage is secure, or as secure, as Cookies. But I'm worried that by only recommending Cookies to store session information in the browser, it dictates requirements that will need to go in the 1.3 Session Management Architectural Requirements, and then also heavily affects the implementation of 3.5 Token-based Session Management. If the only recommended way of storing tokens is via a Cookie, then the only recommended way of accepting a token has to be via a Cookie.

The problems with a Cookie-only approach are:

  • You inherit all the associated cookie security issues (e.g. CSRF). Of course there are mitigations for it, and 3.4 addresses cookie security, but if you don't use cookies you don't even need to think/worry about this
  • Cross-domain authenticated API calls are impossible.

Personally I think that Cookies introduce too much security debt we've been trying to clean up for years. They essentially bypass the SOP and are over-scoped by default (entire domain, not origin). Even with SameSite there are "gotchas" you have to be careful of, which then lead to the Public Suffixes list (e.g. github.io, amazonaws.com, etc). As a backend architect, I'd rather be explicit in allowing authentication material from a whitelisted set of origins and headers via CORS for my services than have to read values from cookies.

I wish there was a better way to keep a secret in a browser, and I do agree that from a purely client side perspective HttpOnly cookies offer more protection than HTML5 storage - but architecturally I don't agree that moving to Cookie-only session management is a security best practice ASVS should be recommending. Just my $0.02 👍

@jmanico
Copy link
Member

jmanico commented Sep 24, 2020 via email

@ropnop
Copy link

ropnop commented Sep 24, 2020

Well said - I do think that is a point we politely disagree on. I don't believe there is no point in protecting it - only that protecting data post compromise (i.e. post XSS) is a defense in depth measure and not a first layer security control. I do believe XSS and CSRF are "first order" vulnerabilities that need to be dealt with first and foremost. Using cookies adds a second layer of defense to XSS, but at the cost of opening the door to CSRF. I guess my argument is XSS is always going to be a threat no matter what; if we can eliminate one class of vulnerability entirely (CSRF) we can spend more effort mitigating the other.

@jmanico
Copy link
Member

jmanico commented Sep 24, 2020 via email

@elarlang
Copy link
Collaborator

elarlang commented Sep 24, 2020

This debate point is, since I can abuse a cookie with XSS there is no point in protecting it.

No. The point is - if you going to remove this sessionStorage options for session tokens (which completely make sense and it's covered also in my proposals), you need to keep in mind that it's not the only solution. We need to provide solution for cross-origin API also.

@jmanico
Copy link
Member

jmanico commented Sep 24, 2020 via email

@jmanico
Copy link
Member

jmanico commented Sep 24, 2020 via email

@tghosth
Copy link
Collaborator

tghosth commented Sep 24, 2020

Hi, when it comes to session storage and cookies, I really really don't think we should be mandating one and banning the other. They both have valid use cases.

@jmanico
Copy link
Member

jmanico commented Sep 25, 2020 via email

@jmanico
Copy link
Member

jmanico commented Sep 25, 2020 via email

@mackowski
Copy link

What about leaving HTML 5 session storage in the description but mark it as a valid option for L1 and L2 apps but mandate cookies for L3 as it adds additional layer of the security.
It will make clear that in some cases session storage is a valid option but if you want the highest level of security use browser cookies.
I see both arguments and I just try to find compromise that will not harm security. If ASVS does not mandate for example "3.2.4 | Verify that session token are generated using approved cryptographic algorithms" for L1 apps than storing session id in session storage also seems to me reasonable.

@elarlang
Copy link
Collaborator

I conclude my comments and context here:

  • context here is ASVS v4.0.2 - no requirements removals, no new requirements, no major meaning changes for requirements. ASVS does not say "if you don't use cookies, use (only) SAML". Till it's that way, you can not close alternative solutions. I think you need to plan your changes for ASVS v4.1.
  • in this context making proposed change, it's basically allows to use only cookies for session tokens. As security person, I don't mind. But I can really well understand, that it does not cover widespread solutions (for example, where session token is kept in sessionStorage (or in JavaScript variable) and value is used as Bearer).
  • XSS or wihtout XSS, if we can do this second layer defence extra security (httponly cookies), then we need to use it. I don't fight against that. On the other hand, without XSS it does not giving anything extra, with XSS it's just requires different technique. In this context it does not deserve the main attention.
  • with CORS you can send cookies to other domain - pre-condion is, that is other domain is going to write that cookie (because you can not write cookie from one domain for another). Maybe it's not always the case? And if you want to set some requirement for that, then it's probably belongs to "Architecture" section.

I think that's it from me, otherwise it's going to loop :)

@jmanico
Copy link
Member

jmanico commented Sep 28, 2020 via email

@mackowski
Copy link

Yes, web workers and JS closures makes a lot of sense in that scenario.

@jmanico
Copy link
Member

jmanico commented Sep 29, 2020 via email

@bretik
Copy link
Author

bretik commented Sep 29, 2020

I already did update the PR, see #844 (comment)

tghosth added a commit that referenced this issue Sep 30, 2020
Changes made by @bretik but originally targeted to 4.0.2 instead of 4.1.
@tghosth
Copy link
Collaborator

tghosth commented Sep 30, 2020

Sorry but regardless of my feelings on this change, it breaks the rules for 4.0.2 because it changes what is acceptable under requirement 3.2.3. An app that previously passed this requirement because it used session storage would now fail this requirement.

Andrew's original rules for 4.0.2:

bug fixes only. Will require some work to back port the fixes from master. The only thing I would countenance here is duplicate issue removal, spelling fixes, and clarifications.

I have opened #846 instead but I want to take the time to read through this thread before I merge it.

@tghosth
Copy link
Collaborator

tghosth commented Sep 30, 2020

I dug into this a bit more.

This blog post from @philippederyck would seem to indicate that web workers are also vulnerable to XSS. (hi @philippederyck I would be keen to hear your thoughts on this thread.)

This just leaves us with JavaScript closures.

Overall, I am concerned that we are mandating a very complex mechanism for a L1 control. I would be more comfortable allowing session storage for L1 but not for L2+.

@jmanico
Copy link
Member

jmanico commented Sep 30, 2020 via email

@tghosth
Copy link
Collaborator

tghosth commented Apr 7, 2021

I think this is a single requirement and we just need to make sure that it is clear that a session token is not stored in local storage. If you propose wording I can take a look :)

@jmanico
Copy link
Member

jmanico commented Apr 7, 2021

There is no harm putting session tokens in local storage; there is no way to protect a token in the browser. Even a cookie can easily be abused.

@elarlang
Copy link
Collaborator

elarlang commented Apr 8, 2021

@jmanico - 2 random things which comes to my mind with that:

  • if user closes a browser and then another user / an attacker re-opening it again, session token kept in localStorage keeps possibility to continue with previous session (if token is not yet expired), sessionStorage does not keep
  • if we watch from "cache" point of view, we disallow to save sensitive data to browser cache. Also we ask to clean up DOM after logout. The same way we should support cleanup sensitive data from browser storage (on closing browser), because tokens usually contains some kind of sensitive data.

Difference is not huge, but difference exists.

@jmanico
Copy link
Member

jmanico commented Apr 8, 2021

  • Since we already suggest token expiration and token logout in ASVS, I don't think the long term storage should cause harm. I need to check the requirements but tradition session management calls for IDLE timeout ABSOLUTE timeout and LOGOUT immediate revocation. If those are implemented, then localStorage risks are fully mitigated.

  • Tokens should not store sensitive data like PII or similar. They should only contain basic AuthN claims and a signature.

And Elar, I think your point is fair. I am trying to push my opinion hard here because in the face of XSS, all browser storage will allow token theft or abuse. I would much rather developers focus on good XSS defense and proper token expiration instead of worrying about token browser storage issues.

@elarlang
Copy link
Collaborator

elarlang commented Apr 8, 2021

sessionStorage vs localStorage question is not related with XSS - for XSS they are equal. Analyze the situation when "there is no XSS on that site".

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).

If we don't make the difference here, we could remove all client-side cache requirements, clean-up DOM etc requirements as well. Just to point out - there is difference and it makes sense to recommend using sessionStorage for session tokens instead of localStorage.

@jmanico
Copy link
Member

jmanico commented Apr 8, 2021

Well I like to keep my expired tokens in sessionStorage so when the user visits the site again I can pre-fill the username and still track the fact that they use the public parts of my site.

I do not think a browser-close event should be needed, I think session idle, absolute and logout events are best to handle session limits.

And I am pushing the boundary here just to try out this side of the debate. I tend to recommend session storage as well, but I have been told by other experts it is not a good piece of advice so I'm trying out that side of the debate.

@elarlang
Copy link
Collaborator

elarlang commented Apr 8, 2021

I would like to get the argumentation behind this "I was told"

@jmanico
Copy link
Member

jmanico commented Apr 8, 2021

XSS defeats all, and session tokens should have idle timeout, absolute timeout and logout revocation.

@jsulinski
Copy link
Contributor

jsulinski commented Apr 8, 2021

I would also like to better understand the rationale behind these changes. I was under the impression that the discussion was closed, so was focused on ensuring things were consistent rather than accurate.

This Github Issue mentions a Twitter thread with more background, could you share that @jmanico? I searched your Twitter feed and there are many discussions on the topic, but I couldn't find the one specific to this thread.

I reviewed some of the existing articles that advocate against httpOnly cookies (ie, [1] [2]).

The arguments tend to boil down to:

  1. XSS is bad and httpOnly cookies don't completely mitigate XSS
  2. Exfiltration of tokens is uncommon and therefore not of concern
  3. Cookies create new problems (CSRF)

My opinion is that defense in depth is important and we should utilize any viable tools we have in defense. httpOnly cookies (and web workers) don't solve every problem related to XSS, but they do create road blocks/speed bumps, and when used in combination with a strong CSP, could mitigate the impact of some XSS attacks.

Several of the articles suggest that if an attacker has XSS with an httpOnly cookie, they're effectively as powerful as having the token itself. As far as I understand, this is incorrect. A strong CSP can still complicate both the capabilities of XSS and exfiltration of any data, which would not apply to a token stolen from browser storage (granted, there may be ways around this, including DNS exfiltration). A stolen token also won't trigger CSP errors, which would be a good indicator that you have an XSS problem.

Exfiltration of tokens being uncommon does not seem like a good argument as in cases where capabilities of XSS are limited by CSP, exifltration may introduce a bypass. Exfiltration could also help to bypass rate limits.The one caveat I can add here is that it is possible that exfiltrated tokens will alert the defender based on IP, atypical request patterns, client agent, or other metadata.

I won't address the CSRF topic as it has been discussed at length and I think we're largely in consensus that CSRF protection is pretty simple. This point is probably moot.

My conclusion mirrors Auth0's suggestion, I think:

httpOnly cookies & BFFs > web workers > closures > sessionStorage > localStorage

From a personal perspective, I can say that since 3.2.3 was removed, explaining the disparity between ASVS and every other standard/best practice/cheat sheet (including OWASP's Cheat Sheets) has been challenging, especially because we use ASVS as our core methodology and disagree with this particular guidance.

[1] https://portswigger.net/research/web-storage-the-lesser-evil-for-session-tokens
[2] https://www.gnucitizen.org/blog/why-httponly-wont-protect-you/

@jsulinski
Copy link
Contributor

jsulinski commented Apr 8, 2021

And I am pushing the boundary here just to try out this side of the debate. I tend to recommend session storage as well, but I have been told by other experts it is not a good piece of advice so I'm trying out that side of the debate.

What explanation did they provide? This seems pretty clear cut for the reasons Elar pointed out above. Idle timeout, absolute timeout, and logout revocation don't necessarily mitigate against the shared computer scenario. sessionStorage adds one more defense mechanism here: browser storage cleanup on closure.

@jmanico
Copy link
Member

jmanico commented Apr 8, 2021 via email

@jmanico
Copy link
Member

jmanico commented Apr 8, 2021 via email

@jsulinski
Copy link
Contributor

jsulinski commented Apr 8, 2021

Can you suggest a PR that will bring things back in alignment for you James? I though you were happy with these changes. I am hesitant to engage in more debate here - can you suggest specific PR’s?

I am happy with the changes to the extent that they are at least consistent and ASVS is not conflicting with itself currently. That is the most important thing.

To the question of what guidance ASVS should be providing, there seem to be several different camps in the recent comments:

  1. Most restrictive. Prefer httpOnly cookies over other browser storage.
  2. Somewhat restrictive. Prefer sessionStorage, httpOnly cookies, closures, or web workers over localStorage.
  3. Unrestricted. Any browser storage is fine.

Based on my understanding of the issue currently, I'm in group 1. I think Elar, Josh and Sjoerd are in group 2. I think you're in group 3.

I'm happy to research and discuss further. I don't know how decision-making occurs for this project, but we're approaching a point where a decision should be made (I thought this already took place). I do think that because ASVS, the Testing Guide, Cheat Sheets and 800-63 are so tightly intertwined, it's probably best to explain that decision if it differs.

I don't think making the recommendation based on level is necessarily the right call (for this version) as much of L2 and L3 require access to code to verify. This is why I previously suggested an L0.5 (or similar) to support "light" pentests. Perhaps in ASVS 5 there can be four levels where L1 is an undemanding environment that can be checked externally (and in this case would not require httpOnly cookies), L2 is for pentesting companies that require following all available best practices (including using httpOnly cookies), and L3 and L4 are the levels that require code review.

PRs should probably come after a decision has been made.

@elarlang
Copy link
Collaborator

elarlang commented Apr 9, 2021

Current situation from my point of view - I made PR to improve the situation and resolve the conflict between different requirements, but like I wrote right after that (#843 (comment)), this requirement (8.2.2) still have space for improvements, and I have pointed out, what to achieve (#843 (comment)).

Personally I prefer httpOnly cookies over browser storage and personally I think, in most cases JWT is not in correct use (there is no need to use JWT, because usage is not stateless), but from ASVS point of view does not matter, what I personally prefer - ASVS need to cover all technologies.

@elarlang
Copy link
Collaborator

elarlang commented Apr 9, 2021

My point is a stolen token is still active even when you delete it from storage. We need to expire tokens to provide security.

Those are different vectors. We can not fix stealing token from client-side with shortening token expire time. Yes, it lowers the likelyhood and makes attack time-window smaller, but it does not fix it. It's another problem.

@elarlang
Copy link
Collaborator

Current requirement 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.

Latest goals to achieve (#843 (comment)):

@elarlang elarlang assigned jsulinski, jmanico and elarlang and unassigned jmanico Jul 20, 2021
@elarlang elarlang added the 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos label Jul 20, 2021
@jsulinski jsulinski removed their assignment Aug 7, 2021
@jmanico
Copy link
Member

jmanico commented Sep 24, 2021

The original posters requests have been long merged. Please open a new issue if lingering concerns remain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos
Projects
None yet
Development

No branches or pull requests