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

Discussion: Too much and/or sensitive information in (mostly API) responses #934

Closed
elarlang opened this issue Mar 20, 2021 · 31 comments · Fixed by #2155
Closed

Discussion: Too much and/or sensitive information in (mostly API) responses #934

elarlang opened this issue Mar 20, 2021 · 31 comments · Fixed by #2155
Assignees
Labels
4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review V4 Temporary label for grouping authorization related issues _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

elarlang commented Mar 20, 2021

Had it in my todo-list but as WSTG has this topic under discussion I make link/placeholder here as well:
OWASP/wstg#727 by @pinkLagoon

Problem: in response from an application there is too much information which may be also sensitive. Happens, when developers are JSON-encoding entire dataset or object and give it for HTTP response.

Risks:

  • sensitive information leakage
    • access control issue (V4)
  • unneeded data in traffic (which is not used in frontend)
    • too many fields in entries, example: only titles are used but entire dataset is provided by API
    • too many rows, example: only 10 rows "per page" are displayed, but entire list is provided by API

Lists in the beginning are small, but if someone "is testing" with every field with max-length value (text fields getting problematic quite fast) - like few hundred MB to download by API most likely is causing some problems for serving the content or handling the content on the client side - availability loss.

Proposal:

  • new requirement(s)
    • todo/discuss - category
    • todo/discuss - requirement text
  • Levels: 1, 2, 3
  • CWE - probably CWE-200
@elarlang
Copy link
Collaborator Author

Category and requirement text - probably direction should be current requirement 8.1.3:

V8.1.3 Verify the application minimizes the number of parameters in a request, such as hidden fields, Ajax variables, cookies and header values.

@jmanico
Copy link
Member

jmanico commented Mar 25, 2021

I do not think it's enough to say "just minimize parameters, etc". What you're really asking is that we only include data in JSON (1) that the user has access to and (2) that the application actually needs.

And I'm confused, the original post is about responses, but the last comment was about requests?

@elarlang
Copy link
Collaborator Author

8.1.3 was taken as an example, where (to what category) to place it in ASVS. Requirement itself is for response.

@jmanico
Copy link
Member

jmanico commented Mar 25, 2021

I personally see this as a pure access control requirement. And it's a core principle. I suggest 4.1 or 4.3

@elarlang
Copy link
Collaborator Author

Yes, part of that is authorization problem and initially I was thinking about this problem more like V4.* problem.

I have seen examples where frontend needs list of titles, but entire entity datasets are provided ended up responses with size of many MB.

@jmanico
Copy link
Member

jmanico commented Mar 25, 2021

I am with you 100% and think this is a great idea for a new requirement! Even if the user has access to the data, it should not be sent to the client unless the user has access AND the app actually needs that data! :)

@csfreak92
Copy link
Collaborator

+1 on this requirement, I actually saw a badly designed/implemented app where the whole API is sending everything to the front-end to make the application's response "faster" but it ended up including a bunch of sensitive information that should never be there in the first place.

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

cmlh commented Aug 13, 2021

Should "sensitive" be inserted into ASVS Level 1 to mirror the use of the term "sensitive" within the description of ASVS Level 2?

@elarlang
Copy link
Collaborator Author

todo, check requirement 13.1.3 as potential duplicate or place to merge, if we can find some solution here:

V13.1.3 Verify API URLs do not expose sensitive information, such as the API key, session tokens etc.

@jmanico
Copy link
Member

jmanico commented Nov 12, 2021 via email

@elarlang
Copy link
Collaborator Author

This is not a problem with API’s it's a problem with any Url due to referrer headers, logs, browser history and similar.

this is what you described, is covered by 8.3.1

V8.3.1 Verify that sensitive data is sent to the server in the HTTP message body or headers, and that query string parameters from any HTTP verb do not contain sensitive data.

the problem is not the method how sensitive data is sent to client, the problem is the fact that sensitive data is exposed to the client.

@elarlang
Copy link
Collaborator Author

elarlang commented Feb 8, 2022

Access control part maybe can be merged to current 1.4.5 (this one need to be moved to V4):
Verify that attribute or feature-based access control is used whereby the code checks the user's authorization for a feature/data item rather than just their role. Permissions should still be allocated using roles.

@tghosth
Copy link
Collaborator

tghosth commented Feb 23, 2022

Which of the following problem options are we suggesting that this is:

  1. An Authorization problem where the user is receiving data in the API that they should not have permissions to access?
  2. An efficiency issue where data is being returned that the user has permissions to but is more data than they need for the current functionality?

@tghosth
Copy link
Collaborator

tghosth commented Dec 12, 2022

V4 Access Control at the JSON attribute level, i.e. you can get the user's firstname and last name but not their password hash, even if it is in the same conceptual object/entity.

Business Logic - Verify that data pagination is enforced at the server side so that the amount of data that can be accessed at once is limited, matching the limit enforced at the front-end.

Too many fields (which the user is permitted to access) seems a little too specific and niche and probably not a serious issue.

@set-reminder 5 weeks @tghosth to look at this.

@octo-reminder
Copy link

octo-reminder bot commented Dec 12, 2022

Reminder
Monday, January 16, 2023 12:00 AM (GMT+01:00)

@tghosth to look at this.

@tghosth tghosth assigned tghosth and unassigned tghosth and elarlang Dec 12, 2022
@tghosth tghosth removed the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Dec 12, 2022
@tghosth
Copy link
Collaborator

tghosth commented Apr 3, 2023

@elarlang should we work on this now or wait for the full access control reorg?

@elarlang
Copy link
Collaborator Author

elarlang commented Apr 5, 2023

@elarlang should we work on this now or wait for the full access control reorg?

We can work on this now but I prefer we take one topic and cover it all at once. Otherwise at the moment I should put my time (again) to investigate all the background and history for give answer and then it waits again few months... and we need to start again from the start.

@tghosth tghosth added 4b Major-rework These issues need to be part of a full chapter rework and removed 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos labels May 31, 2023
@elarlang
Copy link
Collaborator Author

elarlang commented Sep 3, 2024

Problem 1 - the application returns data, the user does not need to see - unintended (potentially sensitive) information leakage

It is not an authorization decision problem - a user may have access to read the object (e. g. user), but not the served attribute of that object (e. g. password hash)

Needs a separate requirement. The requirement can be in V4 or V8.

Problem 2 - possible to ask too many rows to the output - bypassing anti-automation, resource-demanding response building (potential DoS vector)

In general, it's a missing (business logic) input validation problem and seems to be covered by:

11.1.3 Verify that business logic limits and validations are implemented as per the application's documentation.

To have a separate requirement to say "validate the rows-per-page parameter" seems a bit too detailed, but at the same time, it is a really widespread problem (based on my experience).

Problem 3 - too much (not needed) data in the output - resource-demanding response buildng (potential DoS vector)

We have the requirement:

11.2.2 Verify that the application has anti-automation controls to protect against excessive calls to application functionality which could result in mass data exfiltration, junk data creation, resource quota exhaustion, rate limit breaches, out-of-band communication flooding, denial of service, overuse of an expensive resource, etc.

Although pointed out problem is not how to avoid the call to resource-demanding response building, but how to avoid that such unnecessary resource-wasting functionality as potential Denial-of-Service vectors exist.

Do we consider the problem covered by 11.2.2 or is it worth separate mention somehow?

@jmanico
Copy link
Member

jmanico commented Sep 4, 2024

I read this thread carefully. I think the access control issue is really important and should go into the access control re-org.

The performance issue to too much data is a DOS issue that is really easy to fix. I don't think DOS is a really big deal in most apps, even though it is in some. I would suggest we focus on the access control issue and drop DOS issues for the sake of brevity.

I think the access control issues is super critical. The DOS issues described is rarely very critical (but can be on a rare occasion).

@tghosth
Copy link
Collaborator

tghosth commented Sep 8, 2024

Problem 1

Does the 4th bullet of this comment solve this @elarlang?

Problem 2

Yeah I think that is covered by 11.1.3 and you could also argue that it is covered by the updated 5.1.3.

Problem 3

Feels like this is covered by 11.2.2 and maybe also the updated 5.1.3.

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 9, 2024

Does the 4th bullet #2065 (comment) solve this @elarlang?

The linked requirement is added to V1.4 Access Control Architecture via #2065 / #2123

# Description L1 L2 L3 CWE
1.4.7 [ADDED] Verify that access control documentation defines the rules for access control decision-making, specifying user and subject attributes, resource attributes, and relevant environmental factors involved in the process.

I would say that is not cover the problem statement:

Problem 1 - the application returns data, the user does not need to see - unintended (potentially sensitive) information leakage

1.4.7 asks to describe the attributes, based on what you going to make access control decisions. "Problem 1" needs to point out attributes, that you can or can not see/access.

@tghosth
Copy link
Collaborator

tghosth commented Oct 9, 2024

Ok well hopefully @EnigmaRosa will see this as part of here work on V4, and make sure that the access control requirements cover all the various dimensions that I mentioned here: #2065 (comment)

  • Is the user allowed to interact with this particular entity type (e.g. orders in an eCommerce system)
  • Is the user allowed to perform this particular action on this entity type (e.g. can they create an order or only view an order)
  • Is the user allowed to interact with this particular instance of this entity type (e.g. can they only see or modify orders for their department but not other departments)
  • Is the user allowed to interact with these particular fields of this particular entity type (e.g. they can view order details but they are not allowed to see the standard discount amount)

@elarlang
Copy link
Collaborator Author

elarlang commented Oct 9, 2024

As I pointed out previously (#934 (comment)) - if we talk about V4 as "access control decisions", then the requirement is not a V4 requirement. But I'm not sure there is better place for that.

A seed for future development:

Verify that the application returns only the data the user has permission to see. For example, the API response does not return a full object with attributes that contain values the user has no permission to see although having permission to see the data object.

@EnigmaRosa
Copy link
Contributor

I would think that the language in 1.4.7, specifically "documentation defines the rules for access controls" meets the dimensions @tghosth pointed out. I don't know if those points can go unsaid, if we should outline them in guidance, or create another verification requirement surrounding the dimensions (which I feel would be redundant).

With regards to the specific concern @elarlang posed, I like the requirement he proposed as a 4.1.x or 4.2.x

@tghosth
Copy link
Collaborator

tghosth commented Oct 14, 2024

Ok so let's leave the dimensions in section guidance for either 1.4.x or 4.x.x. Can you open a PR for that @EnigmaRosa?

@elarlang I'd be comfortable for your proposed requirement to go into 4.1.x or 4.2.x as Shanni suggested. Do you agree?

@elarlang
Copy link
Collaborator Author

@elarlang I'd be comfortable for your proposed requirement to go into 4.1.x or 4.2.x as Shanni suggested. Do you agree?

Well, it expects that V4.1 and V4.2 have clear definition, what kind of requirements belong there (#2139).

By principle, returning too much data is not about an authorization decision, but just an incorrect behavior from the application. It is pretty much the same problem as mass parameter assignment (ASVS v4.0.3-5.1.2) that we placed to "V10.4 Defensive Coding" as V10.4.4.

@EnigmaRosa
Copy link
Contributor

Should section guidance go right before the verification requirements?

@elarlang I didn't realize we now have a defensive coding section, your requirement might fit in there more.

@tghosth
Copy link
Collaborator

tghosth commented Oct 15, 2024

Should section guidance go right before the verification requirements?

See my comment in @2139 #2139 (comment)

@tghosth
Copy link
Collaborator

tghosth commented Oct 15, 2024

@elarlang can you PR your suggestion into V10.4 as I think that is a sensible suggestion

@elarlang elarlang self-assigned this Oct 16, 2024
@elarlang elarlang added 4) proposal for review Issue contains clear proposal for add/change something 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Oct 16, 2024
@elarlang elarlang linked a pull request Oct 16, 2024 that will close this issue
@elarlang elarlang added 6) PR awaiting review and removed 4) proposal for review Issue contains clear proposal for add/change something 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Oct 16, 2024
@elarlang
Copy link
Collaborator Author

Goes in via #2155 , changed "see" to "access" from previous proposal.

# Description L1 L2 L3 CWE
10.4.5 [ADDED] Verify that the application returns only the data the user has permission to access. For example, the API response does not return a full object with attributes that contain values the user has no permission to access although having permission to access the data object.

elarlang added a commit that referenced this issue Oct 16, 2024
* #934 - return only data that user has permission to access

* Wording tweaks

---------

Co-authored-by: Josh Grossman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review V4 Temporary label for grouping authorization related issues _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