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

encoded sensitive data (such as JWT) should not be logged #1919

Closed
elarlang opened this issue Mar 26, 2024 · 19 comments · Fixed by #2106
Closed

encoded sensitive data (such as JWT) should not be logged #1919

elarlang opened this issue Mar 26, 2024 · 19 comments · Fixed by #2106
Assignees
Labels
6) PR awaiting review V8 _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

elarlang commented Mar 26, 2024

access_token (also id_token) JWT may contain relatively sensitive information, such as person's full name, SSN, email, phone, address, etc.

At the same time, those are sent via URL parameters - that means JWT content is logged everywhere to access logs, browser history etc.

If we could put all this information as plain text to URL parameters, everyone would understand, it's nonsense to send information like a person's full name, SSN, email, phone, and address as URL parameters. If it is inside JWT, it's the same, just base64 encoded.

I know, there is OAuth response_mode=query, and I know there is OIDC logout id_token_hint - this is actually one reason to address this problem. The problem exists for both when the browser communicates directly with the authorization server or IdP.

At the moment we have requirements:

# Description L1 L2 L3 CWE
7.1.1 Verify that the application does not log credentials or payment details. Session tokens should only be stored in logs in an irreversible, hashed form. 532
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 tokens which should be stored in either cookies or sessionStorage. 922
8.3.1 [MODIFIED, MERGED FROM 3.1.1, 13.1.3] Verify that sensitive data is only sent to the server in the HTTP message body or headers and that the URL and query string do not contain sensitive information, such as an API key or session token. 598

Based on 8.3.1 - JWT, containing data about the user, must not sent in a URL parameter anyway.

7.1.1 and 8.2.2, as access_token is not a session token (although massively used as such), I recommend putting special spotlight on JWT (or abstractly "encoded data, such as in JWT") and mentioning them in 7.1.1 and 8.2.2

Visualized:

sensitive_information

@elarlang elarlang added V7 Temporary label for grouping logging related issues V8 labels Mar 26, 2024
@elarlang elarlang changed the title encoded sensitive data (JWT) should not be logged encoded sensitive data (such as JWT) should not be logged Mar 26, 2024
@tghosth tghosth added _5.0 - prep This needs to be addressed to prepare 5.0 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Apr 7, 2024
@EnigmaRosa
Copy link
Contributor

I feel like

If we could put all this information as plain text to URL parameters, everyone would understand, it's nonsense to send information like a person's full name, SSN, email, phone, and address as URL parameters. If it is inside JWT, it's the same, just base64 encoded.

is obvious to those using the ASVS, unless I'm mistaken. Is the clarifying language proposed that necessary? (this is a genuine question - are there ASVS users who would need to be reminded about JWTs?)

@elarlang
Copy link
Collaborator Author

This is the danger when you know the topic and you going to assume, that "this is so basic, everyone must know it" (it's good to teach web application security to developers, it gives a reality check).

I know, there is OAuth response_mode=query, and I know there is OIDC logout id_token_hint - this is actually one reason to address this problem. The problem exists for both when the browser communicates directly with the authorization server or IdP.

I'm pretty sure OAuth and OIDC developers also know what is JWT, but it is still carried from one party to another in the URL parameter.

In short, yes, I think it requires special attention (otherwise I would not have opened the issue). The issue is open to collecting opinions from others, and I got yours, thank you :)

@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

First, I love the meme. I do prefer the Geordi La'Forge version though 🤣

Secondly, I think that @EnigmaRosa is correct that people should really know the risks of storing a JWT and the fact that this is sensitive data. However, I can see how this specific case might slip through the cracks.

It sounds like a subtle clarification in one of the requirements should be sufficient.

@elarlang do you have a specific proposal?

@elarlang
Copy link
Collaborator Author

elarlang commented May 2, 2024

Suitable requirement is 8.3.1, also related 8.3.5.

Maybe a special mention in the chapter text is enough, something like "Just encoding sensitive data (classical solution base64, including as JWT) is the same as sending it as plain text."

@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

Can you open a PR on V8 and I will take it out of V7 for now?

@tghosth tghosth removed the V7 Temporary label for grouping logging related issues label May 2, 2024
@elarlang elarlang added 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR and removed 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Aug 16, 2024
@elarlang elarlang self-assigned this Aug 21, 2024
@jmanico
Copy link
Member

jmanico commented Sep 11, 2024

Good thread, I suggest:

8.2.2 [MODIFIED, MERGED FROM 3.2.3] Verify that data stored in browser storage (such as localStorage, sessionStorage, IndexedDB, cookies) as well as any use of JSON Web Tokens (JWT) does not contain sensitive data, with the exception of session information or identifiers.

I do not think we need to modify 7.1.1

@elarlang
Copy link
Collaborator Author

JWT is just an example of encoded data that potentially contains sensitive information.

The problem exists everywhere, where sensitive information is mentioned - URL, cache, client-side storage, etc.

Thinking more about it, maybe it can be probably solved only by the definition via documentation requirements. So one option is to "inject" it to 1.8.1 or 1.8.2, but when watching those did not have any idea how to solve it.

@tghosth
Copy link
Collaborator

tghosth commented Sep 13, 2024

I am not sure what the proposed change here is

@tghosth tghosth added the next meeting Filter for leaders label Sep 13, 2024
@elarlang
Copy link
Collaborator Author

I keep it open till we have sorted out related stuff from tokens and OAuth topics.

@TobiasAhnoff
Copy link

In #2046 I suggested

Verify that if the ID Token is sent to the front channel or a public client they should be encrypted or not contain any sensitive user data.

I think it is included in this discussion (and can be closed as a duplicate)

Verify that data stored in browser storage (such as localStorage, sessionStorage, IndexedDB, cookies) as well as any use of JSON Web Tokens (JWT) does not contain sensitive data, with the exception of session information or identifiers.

One more thing to note here, is that in example the .NET framework "by default" stores sensitive data (ID, access and refresh tokens) in encrypted cookies. So perhaps change to "with the exception of encrypted session information or session identifiers"?

@tghosth
Copy link
Collaborator

tghosth commented Sep 18, 2024

Ok so for the encoding problem, maybe we need to specify in 1.8.1

# Description L1 L2 L3 CWE
1.8.1 [MODIFIED, MERGED FROM 8.3.4, LEVEL L2 > L1] Verify that all sensitive data created and processed by the application has been identified and classified into protection levels, and ensure that a policy is in place on how to deal with sensitive data. It is important that this includes sensitive data that is being encoded in a recoverable form such as Base64, especially JWTs. 213

@tghosth
Copy link
Collaborator

tghosth commented Sep 18, 2024

@elarlang agreed with that as well.

Anyone with any other comments?

@tghosth
Copy link
Collaborator

tghosth commented Sep 18, 2024

@set-reminder in 4 days @tghosth to open a PR

Copy link

octo-reminder bot commented Sep 18, 2024

Reminder
Sunday, September 22, 2024 12:00 AM (GMT+02:00)

in @tghosth to open a PR

@tghosth tghosth removed the next meeting Filter for leaders label Sep 18, 2024
@elarlang
Copy link
Collaborator Author

It is important that this includes sensitive data that is being encoded in a recoverable form such as Base64, especially JWTs.

I propose update:

Note that this includes sensitive data that is being encoded in a recoverable form such as Base64, including JWTs.

@jmanico
Copy link
Member

jmanico commented Sep 18, 2024 via email

@elarlang
Copy link
Collaborator Author

elarlang commented Sep 19, 2024

Update to be technically correct

Note that this includes sensitive data that is being encoded in a recoverable form such as Base64 and JWT.

Copy link

octo-reminder bot commented Sep 21, 2024

🔔 @tghosth

in @tghosth to open a PR

@elarlang elarlang added the 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR label Sep 22, 2024
@tghosth tghosth linked a pull request Sep 22, 2024 that will close this issue
@tghosth
Copy link
Collaborator

tghosth commented Sep 22, 2024

Opened #2106

@tghosth tghosth added 6) PR awaiting review and removed 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 labels Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6) PR awaiting review 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.

5 participants