Security Checks on the DCQL Query

[simoneonofri]

During the security analysis of the Digital Credentials API, we identified an issue with using a query language in protocols.

The use of a query language such as DCQL generates a potential vulnerability in terms of injection, as defined by CWE-943:

The product generates a query intended to access or manipulate data in a data store such as a database, but it does not neutralize or incorrectly neutralizes special elements that can modify the intended logic of the query

It is therefore important to indicate in the Security Considerations section or warn the issue to the implementers.

Thanks,

Simone

[jogu]

Thanks Simone. I'm not sure this is actually the most actionable advice though - if the verifier is allowing the user to modify the DCQL query without proper escaping, then there will almost certainly be ways the attacker can modify the query such that is it still valid.

The advice around preventing SQL injection (e.g. https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html ) is all around the component preparing the SQL query doing it correctly, so I think the primary thing we need to say is that verifiers need to ensure proper escaping if they are including user-sourced input into the DCQL query.

I think this should really only be problematic in the case where value matching is being used, I'm not sure there's anywhere else in the DCQL query where there would be a reason to include user provided content.

[simoneonofri]

@jogu you're welcome.

Using the example of SQL injection, one of the most famous cases, the best solution is to use prepared statements (i.e., a parametric query). I'm not sure how applicable this is to the specific case.

The point is that in SQL injection, we typically have: User (malicious) > modifies the input > application server that performs the query > database that receives it.

Here we have: Verifier (malicious) > User > the query > Wallet.

The threat source is from those who make a query, but the impact is felt first and foremost by those who receive and process it.

Current security considerations rightly advise that you should not trust what the Verifier receives, and the concept remains largely the same. So it makes sense to tell the recipient of the query not to trust it and to verify it.

Therefore, checking the query structure at the format level (JSON) could be useful (to prevent someone from changing the semantics of that query), and this can be done in several ways. Validating the JSON and checking for characters that have semantic value.

Concerning the use case, I was conducting some experiments via digitalcredentials.dev (and was preparing a Proof of Concept, but I was advised to write first due to time constraints).

[jogu]

So I'm not sure SQL injection is the right parallel for the problem you're trying to describe, as in the SQL injection case the attacker is trying to manipulate the site under attack to produce SQL that doesn't do what the site originally intended. That kind of manipulation is potentially also applicable to OID4VP verifiers, but only if they include user input into the query and don't generate the JSON correctly.

Verifier (malicious) > User > the query > Wallet.

I don't think the "user" is really a player at that point in the flow (unless they're acting as an attacker, and if they are the right way to mitigate that threat is signed requests).

Therefore, checking the query structure at the format level (JSON) could be useful (to prevent someone from changing the semantics of that query)

I mean perhaps, but really there is a single component that is consuming the query, so the semantics are what they are.

, and this can be done in several ways. Validating the JSON

Calling out explicitly that invalid JSON needs to be rejected seems fine, I'd imagine most wallets are already doing that

checking for characters that have semantic value.

I don't really understand what this means a wallet needs to do. Maybe there's something that could be done but it's heuristic perhaps - there's not really any characters that have semantic value after you've parsed the json, and at parsing point the json is either invalid or valid (and obviously you should reject invalid json).

[jogu]

Discussed on yesterday's WG call. We think it would be good to include something about verifiers being careful to form json correctly (particularly in the case where they are, e.g., doing value matching based on potentially untrusted input values), and also to warn wallets to reject invalid json (e.g. enabling 'strict' mode in any json parser if it has such) and possibly to ensure that they only have one component that parses DCQL to avoid inconsistencies if slightly invalid DCQL was parsed differently in different components - so (as noted in some of the above comments) we think we need updates to the text before we can merge this.

[simoneonofri]

Discussed on yesterday's WG call. We think it would be good to include something about verifiers being careful to form json correctly (particularly in the case where they are, e.g., doing value matching based on potentially untrusted input values), and also to warn wallets to reject invalid json (e.g. enabling 'strict' mode in any json parser if it has such) and possibly to ensure that they only have one component that parses DCQL to avoid inconsistencies if slightly invalid DCQL was parsed differently in different components - so (as noted in some of the above comments) we think we need updates to the text before we can merge this.

Thank you @jogu! Do you want me to work on the PR text?

[jogu]

Thank you @jogu! Do you want me to work on the PR text?

@simoneonofri sure, if you have some ideas on how to address the feedback please feel free to update the PR!

[Sakurann]

@simoneonofri could you please sign IPR for DCP WG (which is a pre-requisite to merging PRs by a contributor)? I don't think we currently have it on file neither for W3C or you as an individual.