-
Notifications
You must be signed in to change notification settings - Fork 35
Security Checks on the DCQL Query #647
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
Open
simoneonofri
wants to merge
1
commit into
openid:main
Choose a base branch
from
simoneonofri:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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).
I mean perhaps, but really there is a single component that is consuming the query, so the semantics are what they are.
Calling out explicitly that invalid JSON needs to be rejected seems fine, I'd imagine most wallets are already doing that
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).