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

Security best practices for verifying AnonCreds W3C VPs #322

Open
TimoGlastra opened this issue Feb 7, 2024 · 2 comments
Open

Security best practices for verifying AnonCreds W3C VPs #322

TimoGlastra opened this issue Feb 7, 2024 · 2 comments

Comments

@TimoGlastra
Copy link
Member

It might be good to set up some best practices for verifying AnonCreds W3C VPs, especially in relation to usage with for example PEX.

Here's some security things I've ran into that maybe allow for some unepxected behaviour:

  • AnonCreds W3C VCs and VPs only allow the credentialSubject to be integrity integrity protected. If you for example add a field hello on the top-level of the w3c credential, AnonCreds will just strip it (as it only looks at the credential Subject). This is different from other W3C Signature Suite implementation I've seen, which always throw an error if there's a field that's not integrity protected. If you now take the JSON of a W3C cred, pass it to AnonCreds RS to verify and it's OK 👍 , you think you can trust the W3C JSON, but rather you should then get the W3C credential from AnonCreds RS as that will have the non-integrity-protected values stripped. I think AnonCreds RS should default to throwing an error if a value is not integrity protected. This means that after creating the presentation I can include some properties top-level that may be needed for the PEX submission. In our case the PEX library checks if the presentation / credential has the right attributes and values, but it's up to the credential/presentation verification logic (in this case AnonCresd) to make sure the values are actually integrity protected.
  • I was able to change the issuer on a VC in a VP and the verification went succesfull. I think a check may need to be added to make sure the issuer claim in the W3C cred matches with the issuer_id in the credential definition id
  • I was able to change the verificationMethod on a VC in VP and the verification went successfull. Also here a check would be useful as you cannot trust this value at all.

I think some concrete things we can do to make bypassing validation harder is:

  • only allow integrity protected fields to be present on AnonCreds VCs and VPs. If the AnonCreds RS notices any field that is not integrity protected it should throw an error (like a normal JWT verification would also fail if you add a field to the JWT payload)
  • Verify some parameters not native in AnonCreds VC, but present in W3c AnonCreds VCs (like issuer and proof.verificationMethod)

I think especially the verificationMethod on the proof is what we currently used to fetch the credential definitions before passing it to the verify method, however it could be that the actual credential is using different credential definitions. (a smart constructed request could exploit this)

There's probably more things to consider but these are few I encountered when tinkering with AnonCreds RS

@TimoGlastra
Copy link
Member Author

I think we should address these in the AnonCreds RS lib. We could also address these in Credo / ACA-Py but it'd mean duplication of work, and also that other frameworks need to be very careful in adopting AnonCreds W3C

@swcurran
Copy link
Member

swcurran commented Feb 8, 2024

@andrewwhitehead is working on this now. Let’s hold off on 0.2.0 for this. Reasonable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants