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

Edit algorithms to permit more general proof configuration options #49

Merged
merged 4 commits into from
Feb 25, 2024

Conversation

Wind4Greg
Copy link
Collaborator

@Wind4Greg Wind4Greg commented Jan 4, 2024

This PR addresses issue #48. It edits the document in three sections:

  • Proof Configuration (ecdsa-rdfc-2019),
  • Proof Configuration (ecdsa-jcs-2019),
  • Base Proof Configuration (ecdsa-sd-2023)

The changes are all similar. Instead of starting with an empty object the procedure starts with a clone of the proof options, this way any custom proof options are passed through for hashing and signing.


Preview | Diff

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small editorial tweaks for clarity

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Wind4Greg and others added 2 commits January 8, 2024 14:33
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@Wind4Greg Wind4Greg requested a review from TallTed January 8, 2024 22:41
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Copy link

@KDean-GS1 KDean-GS1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no opinion on this PR.

@@ -691,33 +691,17 @@ <h4>Proof Configuration (ecdsa-rdfc-2019)</h4>

<ol class="algorithm">
<li>
Let <var>proofConfig</var> be an empty object.
Let <var>proofConfig</var> be a clone of the <var>options</var> object.
Copy link
Member

@msporny msporny Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I avoid cloning the options object because it could contain information that has nothing to do with this algorithm. At worst, it could get the implementer to implement something that allows an injection attack (from the network) of some kind. Imagine that an implementer just takes options off of the wire in a VC API call and passes it right into this algorithm. If that object is then passed to a 3rd party library that takes other options, it would allow an attacker to change runtime call parameters for that 3rd party library. To avoid this class of attack, all of the algorithms are written such that that particular type of injection attack is avoided.

Copy link
Contributor

@dlongley dlongley Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cloning is what enables extensibility and makes this PR solve the issue -- without it, the issue would not be solved. I think "preventing injection attacks from the network" here would be done at the wrong layer and there is already a presumption that untrusted inputs will always be appropriately sanitized (or just rejected) before being passed to a signing library. Otherwise, an attacker could ask for anything to be signed; it doesn't matter whether the proof options are "built-ins" or extensible ones.

So, -1 to changing this; I think what is here is already the right approach for solving the issue.

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rewrite allows a class of injection attack that should be of concern. Let's try to mitigate that class of injection attack if possible.

@Wind4Greg is this a normative change? It looks like one. We have to track those (by labeling the PR) because ANY normative change triggers another CR round (minimum wait time 30 days). I believe we have to do one anyway for all the DI specs, just noting that normative changes are typically bad things during CR... so if we have one normative change happening, we might as well do all the ones we know about.

I'm marking this one as normative until you tell me otherwise.

@msporny msporny added the normative This item is a normative change. label Feb 4, 2024
Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion w/ @Wind4Greg and @dlongley, I'm removing my change requests for the following reasons:

  • Protecting against injection attacks should be done at a higher layer than these algorithms (it's a best practice, we don't need to repeat it here).
  • The algorithm is simplified by cloning the options.

@msporny msporny merged commit 0a2c902 into w3c:main Feb 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative This item is a normative change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants