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

5.3.6 enhancement #1555

Closed
jmanico opened this issue Feb 17, 2023 · 22 comments · Fixed by #2121
Closed

5.3.6 enhancement #1555

jmanico opened this issue Feb 17, 2023 · 22 comments · Fixed by #2121
Assignees
Labels
6) PR awaiting review V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@jmanico
Copy link
Member

jmanico commented Feb 17, 2023

Suggest clarifying:

5.3.6 [MODIFIED] Verify that the application protects against JSON injection attacks. (C4) 75

with

5.3.6 [MODIFIED] Verify that the application protects against JSON injection attacks by sanitizing JSON before JSON parsing. (C4) 75
@elarlang
Copy link
Collaborator

If it moves to sanitize, then it must move to another category (5.2) as well

@elarlang
Copy link
Collaborator

Also, we have opened one separate issue for JSON topic, maybe we should get a bit wider overview there in place: #1538

@tghosth
Copy link
Collaborator

tghosth commented Mar 14, 2023

I have merged the comments into #1538 where this issue will be handled more widely and will close this issue for now.

@tghosth tghosth closed this as completed Mar 14, 2023
@tghosth tghosth closed this as not planned Won't fix, can't repro, duplicate, stale Mar 14, 2023
@tghosth tghosth reopened this Jul 24, 2024
@tghosth
Copy link
Collaborator

tghosth commented Jul 24, 2024

I don't feel this requirement got fully handled in #1538 so I am reopening this.

It feels to me like encoding is a better option than "sanitizing".

5.3.6 | [MODIFIED] Verify that the application protects against JSON injection attacks by encoding untrusted input before including it in a JSON object. | ✓ | ✓ | ✓ | 75
-- | -- | -- | -- | -- | --

What do you think @jmanico?

@ryarmst
Copy link
Collaborator

ryarmst commented Jul 24, 2024

Considering the existence of other data interchange formats, would it make sense to instead have a general requirement for protecting against data interchange format injection attacks? Or, are JSON and XML (as in 5.3.10) simply prevalent enough to warrant their own specific requirements?

@jmanico
Copy link
Member Author

jmanico commented Jul 24, 2024

I like Elar's idea of a more general requirement that handles JSON, XML, YAML and any other data format. They all need to be well formed according to the specs to avoid data interchange format injection attacks and parser attacks.

@tghosth
Copy link
Collaborator

tghosth commented Jul 24, 2024

So this requirement is less about processing a whole JSON or XML object and more about taking some input and using it to build a JSON object ourselves. 5.3.10 is similar although I would argue that there are two injection scenarios there.

We have separate requirements for JSON schema validation 13.2.2 and XML schema validation 13.3.1.

@ryarmst
Copy link
Collaborator

ryarmst commented Jul 24, 2024

Exactly, I understand this not as a parsing or interoperability issue, but injection into an object we build with untrusted data. Schema validation may not necessarily solve the issue as an injection could produce an object that passes validation that nevertheless includes manipulations of data not intended to be modified.

Ignoring XPath for the moment (which could be considered categorically distinct), is there a reason for this type of injection to consider JSON, XML, and other formats separately with respect to requirements?

@tghosth
Copy link
Collaborator

tghosth commented Jul 28, 2024

So this is a key question for chapter V5.

I guess this all fits under the category of some form injection where data is incorporated in a way that it controls structure or semantic meaning or something else.

At this point, we cover many different examples of this with different requirements for each, see section 5.3.

The primary arguments for this would probably be that:

  1. The encoding required in each case would be very different.
  2. Having a single catch all requirement for lots of different injection scenarios would be very hard to both implement and verify.

So based on that argument, we would consider the injection scenarios in separate requirements.

Curious to hear what you think though.

@tghosth
Copy link
Collaborator

tghosth commented Sep 2, 2024

Any thoughts on this @ryarmst ?

@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet _5.0 - prep This needs to be addressed to prepare 5.0 V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements labels Sep 2, 2024
@elarlang
Copy link
Collaborator

elarlang commented Sep 2, 2024

I tried to understand what the issue was about, but I did not...

@tghosth
Copy link
Collaborator

tghosth commented Sep 18, 2024

At this point I'd like to close this out.

The initial ask was to clarify the protection for this requirement.

It feels to me like encoding is a better option than "sanitizing".

As such, I proposed:

5.3.6 [MODIFIED] Verify that the application protects against JSON injection attacks by encoding untrusted input before including it in a JSON object. 75

What do you think @jmanico?

@elarlang
Copy link
Collaborator

related, waiting for #1195

@elarlang elarlang assigned elarlang and unassigned jmanico Sep 18, 2024
@elarlang elarlang added the 4a) Waiting for another This issue is waiting for another issue to be resolved label Sep 18, 2024
@elarlang
Copy link
Collaborator

Started to work on it but I can not understand the "problem to solve" here? Is it about building the JSON (current 5.3.6) or using the JSON (something to 5.1.* or 5.2.*)?

@elarlang elarlang added 2) Awaiting response Awaiting a response from the original poster and removed 4a) Waiting for another This issue is waiting for another issue to be resolved labels Sep 18, 2024
@tghosth
Copy link
Collaborator

tghosth commented Sep 18, 2024

I think we are talking here about building JSON and we should stay to focused on that and not fall into the discussion #1195.

As such, we are just waiting for @jmanico to confirm if he is ok with this:

5.3.6 [MODIFIED] Verify that the application protects against JSON injection attacks by encoding untrusted input before including it in a JSON object. 75

@set-reminder @tghosth to PR in 5 days if no response

Copy link

octo-reminder bot commented Sep 18, 2024

Reminder
Monday, September 23, 2024 12:00 AM (GMT+02:00)

@tghosth to PR in if no response

@elarlang
Copy link
Collaborator

I can not understand the proposed requirement:

Verify that the application protects against JSON injection attacks by encoding untrusted input before including it in a JSON object.

The context - with the requirement, we are in output encoding section. There is just need to use JSON encoding for the user-input and it creates the JSON as string. If you parse the string in JavaScript, you'll get the JSON object, but it is not output encoding responsibility anymore.

So my question is still valid - what problem does it need to solve?

@jmanico
Copy link
Member Author

jmanico commented Sep 18, 2024 via email

@tghosth tghosth added the next meeting Filter for leaders label Sep 19, 2024
Copy link

octo-reminder bot commented Sep 22, 2024

🔔 @tghosth

@tghosth to PR in if no response

@tghosth tghosth added 4a) Waiting for another This issue is waiting for another issue to be resolved and removed 2) Awaiting response Awaiting a response from the original poster 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Sep 26, 2024
@tghosth
Copy link
Collaborator

tghosth commented Sep 26, 2024

Waiting for #1195

@elarlang
Copy link
Collaborator

The proposal to solve also 5.3.6 is written in #1195 (comment)

V5.3.3 [MODIFIED, SPLIT TO 50.5.*] Verify that output encoding or escaping is used when dynamically building JavaScript content (including JSON), to avoid changing the message or document structure (to avoid JavaScript and JSON injection).

V5.3.6 [DELETED, DUPLICATE of 5.3.3]

@jmanico - does it solve the risen problems here? If not, what aspect is not covered?

@elarlang elarlang added 2) Awaiting response Awaiting a response from the original poster and removed 4a) Waiting for another This issue is waiting for another issue to be resolved next meeting Filter for leaders labels Sep 26, 2024
@jmanico
Copy link
Member Author

jmanico commented Sep 26, 2024

I think this new 5.3.3 is complete enough. I like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6) PR awaiting review V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements _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.

4 participants