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

Prevent cross-site access with JSONP #903

Closed
Sjord opened this issue Feb 18, 2021 · 31 comments · Fixed by #1818
Closed

Prevent cross-site access with JSONP #903

Sjord opened this issue Feb 18, 2021 · 31 comments · Fixed by #1818
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review V50 Group issues related to Web Frontend _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@Sjord
Copy link
Contributor

Sjord commented Feb 18, 2021

JSONP is a method to provide cross-site access. If this is misconfigured, it makes it possible for any site on the internet to access information from the target page. It seems the ASVS currently lacks a verification requirement for this attack vector.

E.g. Exploiting JSONP and Bypassing Referer Check

We could make this more general and say that an application shouldn't use sensitive information from within JavaScript. For example, if an application has a file profile.js that has var username = "your_current_username";, any other site can load that script and figure out your username. It seems the ASVS does not currently have a requirement against this.

@jmanico
Copy link
Member

jmanico commented Mar 12, 2021

JSONP is a major antipattern and should not be used, it allows for CSP bypass.

Can you give us a PR with a new requirement to avoid this and we can work on getting it rolled into main?

Reference: https://blog.fullstacktraining.com/why-jsonp-shouldnt-be-used/

@Sjord
Copy link
Contributor Author

Sjord commented Mar 18, 2021

I would suggest something like this:

Verify that sensitive information is not present in JavaScript or JSONP responses, to prevent cross-origin access to that information.

@jmanico
Copy link
Member

jmanico commented Mar 18, 2021 via email

@Sjord
Copy link
Contributor Author

Sjord commented Mar 19, 2021

Where should this go? Perhaps somewhere in V8?

@jmanico
Copy link
Member

jmanico commented Mar 19, 2021

I think V8.2 is reasonable. @elarlang ?

@elarlang
Copy link
Collaborator

I need to investigate this topic more before I'm able to give recommendations. V8.2 does not seem the way to go - it's more "do not store sensitive data on the client side" which is not the problem at the moment.

@jmanico
Copy link
Member

jmanico commented Mar 19, 2021

JSONP is a menace to society and I would support a negative requirement so it's clear we need to avoid it

@elarlang elarlang self-assigned this Jul 14, 2021
@elarlang elarlang added the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Jul 14, 2021
@elarlang
Copy link
Collaborator

elarlang commented Aug 11, 2021

Some more ideas on that category-topic - it's not sensitive data leakage from the point of view - this user's browser has permission to access it and can see it anyway (no extra leakage)

The problem is - JavaScript code, which is readable with Cross-Origin requests, contain (sensitive) data.

And there are 2 ways how to fix it:

  • remove data from JavaScript code
  • restrict access for Cross-Origin requests (similar to defense against CSRF)

The problem exists, when request is made with authenticated/authorised user and response contains information which is only meant for them. It is most likely the case for cookie-based sessions as well.

For non-authenticated users there is requirement like 2.10.4.

I watch this issue as "Cross-Origin Reading (Request)", and CSRF as "Cross-Origin Writing (Request)". So, those should belong together to one category and we don't have suitable one at the moment.

Or

  • Cross-Origin Confidentiality - attacker forces victim's browser to read data from targeted site (XSSI)
  • Cross-Origin Integrity - attacker forces victim's browser to make data change request in targeted site (CSRF)

Those were thoughts at the moment, still need more investigation from my side.

@cmlh
Copy link
Contributor

cmlh commented Aug 12, 2021

Can I recommend that any specific older legacy web technology such as JSONP be put into an ASVS fork as there is no fix/control for JSONP bypassing CSP and developers will argue this works as intended?

I believe we may need another create another issue to discuss sensitive data in cleartext JSON. That said, isn't JSON Web Encryption (JWE) the appropriate control and if so the test case should be the "Invalid Curve Attack"?

@elarlang
Copy link
Collaborator

Can I recommend that any specific older legacy web technology such as JSONP be put into an ASVS fork as there is no fix/control for JSONP bypassing CSP and developers will argue this works as intended?

I think we should show the way to go, not discussing was the way so far correct.

I believe we may need another create another issue to discuss sensitive data in cleartext JSON. That said, isn't JSON Web Encryption (JWE) the appropriate control and if so the test case should be the "Invalid Curve Attack"?

Check #934

@cmlh
Copy link
Contributor

cmlh commented Aug 13, 2021

I think we should show the way to go, not discussing was the way so far correct.

ASVS is not intended to force a developer to change web technologies when a control exists, otherwise we'll alienate developers who implement JSONP.

@Sjord
Copy link
Contributor Author

Sjord commented Aug 19, 2021

Cross-Origin Confidentiality

Another issue that falls under this category is cross-site search. The attacker performs searches using CSRF requests, and times the responses to determine whether a result is found. This is not always prevented by frameworks with built-in CSRF tokens, since search is considered an idempotent operation, and can use GET (search.php?q=something). CSRF protection is often only enabled on POST requests.

Can I recommend that any specific older legacy web technology such as JSONP be put into an ASVS fork as there is no fix/control for JSONP bypassing CSP and developers will argue this works as intended?
ASVS is not intended to force a developer to change web technologies when a control exists, otherwise we'll alienate developers who implement JSONP.

I don't understand what you are trying to say. Do you agree that we need an ASVS requirement that prohibits JSONP? Or do you want the ASVS to be compatible with existing systems that use JSONP?

@jmanico
Copy link
Member

jmanico commented Aug 19, 2021 via email

@cmlh
Copy link
Contributor

cmlh commented Aug 19, 2021

I don't understand what you are trying to say. Do you agree that we need an ASVS requirement that prohibits JSONP? Or do you want the ASVS to be compatible with existing systems that use JSONP?

I am seeking to make ASVS diverse and inclusive for all web technologies and therefore the most secure implementation of JSONP within legacy web applications i.e. "Or do you want the ASVS to be compatible with existing systems that use JSONP?"

@tghosth
Copy link
Collaborator

tghosth commented Feb 9, 2022

If we focus on JSONP, is there any problem with @Sjord's suggestion above?

Verify that sensitive information is not present in JavaScript or JSONP responses, to prevent cross-origin access to that information.

To me it seems to handle the specific issue nicely. If we agreed that this needs to go somewhere, we can then decide where.

@elarlang
Copy link
Collaborator

In general agree with proposal, category choice depends on #1230. At the moment best category seems to be 13.1 ("V13 API and Web Service" > "V13.1 Generic Web Service Security")

Verify that sensitive information is not present in JavaScript or JSONP responses, to prevent cross-origin access to that information.

Some ideas/questions:

  • Should it contain attack name Cross-Site Script Inclusion (XSSI)?
  • Is it in general clear, what means "JavaScript response" or "JSONP response"? For me it is, but I'm worried, is it interpretable multiple ways.
  • cross-origin vs Cross-Origin
  • is "to that information" necessary?

@tghosth
Copy link
Collaborator

tghosth commented Feb 23, 2022

How about this:

Verify that JSONP functionality is not enabled anywhere across the application and that sensitive information is not present in JavaScript files to avoid Cross-Site Script Inclusion (XSSI) attacks.

@jmanico
Copy link
Member

jmanico commented Feb 23, 2022

How about this:

Verify that JSONP functionality is not enabled anywhere across the application and that sensitive information is not present in JavaScript files to avoid Cross-Site Script Inclusion (XSSI) attacks.

I like it. Since these are different ideas and fundamental ones, I'd split them into two requirements.

@tghosth
Copy link
Collaborator

tghosth commented Feb 23, 2022

ok so like:

Verify that JSONP functionality is not enabled anywhere across the application to avoid Cross-Site Script Inclusion (XSSI) attacks.
Verify that sensitive information is not present in JavaScript files to avoid Cross-Site Script Inclusion (XSSI) attacks.

@jmanico
Copy link
Member

jmanico commented Feb 23, 2022 via email

@tghosth
Copy link
Collaborator

tghosth commented Feb 23, 2022

awaiting outcome of #1230

@tghosth tghosth added 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Feb 23, 2022
@tghosth tghosth added the _5.0 - prep This needs to be addressed to prepare 5.0 label Sep 14, 2022
@elarlang
Copy link
Collaborator

elarlang commented Dec 1, 2023

Verify that sensitive information is not present in JavaScript files to avoid Cross-Site Script Inclusion (XSSI) attacks.

I prepare for PR and re-analyzing those requirements:

  • "sensitive data" is a bit vague - maybe authenticated data?
  • "JavaScript" is a bit vague - technically JSON is also JavaScript
  • "file" is a bit too strict - usually, when sensitive data is part of JavaScript content, it's a dynamically built response, not a file.

Need some wording and grammar help here, but the direction is maybe something like:
Verify that authenticated data is not present in the HTTP response when it can be included as a JavaScript source to avoid Cross-Site Script Inclusion (XSSI) attacks.

Technical note: it's only problematic when the session token is transferred automatically with the same request, which in practice means if the session token is delivered in the Cookie header and:

  • the session cookie has the SameSite attribute set to None
  • OR to value Lax when an attack comes from Same-Site scope.

@elarlang elarlang added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos labels Dec 1, 2023
@elarlang elarlang added the next meeting Filter for leaders label Dec 15, 2023
elarlang pushed a commit to elarlang/ASVS that referenced this issue Dec 15, 2023
@elarlang
Copy link
Collaborator

elarlang commented Dec 21, 2023

Requirements are now in the document based on the last proposed wording:

V50.4 Cross-Site Script Inclusion

# Description L1 L2 L3 CWE
50.4.1 [ADDED] Verify that JSONP functionality is not enabled anywhere across the application to avoid Cross-Site Script Inclusion (XSSI) attacks.
50.4.2 [ADDED] Verify that sensitive information is not present in JavaScript files to avoid Cross-Site Script Inclusion (XSSI) attacks.

As pointed out in previous comment (#903 (comment)), requirements can be improved.

ping @tghosth , @Sjord

@tghosth
Copy link
Collaborator

tghosth commented Dec 28, 2023

How about:

# Description L1 L2 L3 CWE
50.4.2 [ADDED] Verify that data which should require authorization to access is not returned in executable JavaScript format to avoid Cross-Site Script Inclusion (XSSI) attacks.

@elarlang
Copy link
Collaborator

I think "executable JavaScript format" is misleading. Some may think it is only valid for some Electron apps etc. https://medium.com/swlh/javascript-executables-5644ead7016d

maybe direction like "is not returned in response for loading script resource"?

@tghosth
Copy link
Collaborator

tghosth commented Dec 28, 2023

You mean like this:

# Description L1 L2 L3 CWE
50.4.2 [ADDED] Verify that data which should require authorization to access is not returned in script resource responses to avoid Cross-Site Script Inclusion (XSSI) attacks.

@elarlang elarlang added 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR and removed 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos next meeting Filter for leaders labels Dec 28, 2023
elarlang added a commit that referenced this issue Dec 28, 2023
@elarlang elarlang linked a pull request Dec 28, 2023 that will close this issue
@jmanico
Copy link
Member

jmanico commented Dec 28, 2023 via email

@tghosth
Copy link
Collaborator

tghosth commented Dec 28, 2023

@elarlang can we maybe add a clarification like

# Description L1 L2 L3 CWE
50.4.2 [ADDED] Verify that data which should require authorization to access is not returned in script resource responses (such as JavaScript files) to avoid Cross-Site Script Inclusion (XSSI) attacks.

@elarlang
Copy link
Collaborator

which > that, brackets to commas
46dba30

@elarlang elarlang added 6) PR awaiting review and removed 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Dec 29, 2023
tghosth pushed a commit that referenced this issue Jan 18, 2024
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 4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review V50 Group issues related to Web Frontend _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