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

Misleading exception when validating malformed signature #234

Open
JohnnyJayJay opened this issue Sep 23, 2022 · 1 comment
Open

Misleading exception when validating malformed signature #234

JohnnyJayJay opened this issue Sep 23, 2022 · 1 comment

Comments

@JohnnyJayJay
Copy link
Contributor

The Crypto.verifySignature method used to validate assertion signatures currently throws a very general exception if any IllegalArgumentException or GeneralSecurityException occurs in the process. It seems to me that this is not intended to be caught in user code because it is sort of a non-recoverable thing ("either reconfigure your JVM because e.g. you're lacking some cryptographic provider or report this as a bug in the library" is what the exception message suggests).

However there is a case that is recoverable and should be equivalent to an invalid signature (i.e. signature.verify(bytes) returning false). That case is a malformed signature. I.e., the signature bytes are present but cannot be parsed according to the provided algorithm. in that case, signature.verify(bytes) will throw a SignatureException. IMO this exception should be caught separately and ultimately result in a AssertionFailedException on the API end rather than a RuntimeException. The goal of this would be to handle (possibly malicious) erroneous requests better and prevent them from throwing uncaught RuntimeExceptions that the library or your app isn't at fault for.

One open question is of course how that information should be conveyed to the caller of verifySignature. If verifySignature just returned false in case of a SignatureException on the verify call, you would certainly lose some information that could be useful in development/debugging.

@emlun
Copy link
Member

emlun commented Sep 23, 2022

Thanks for bringing this up! This seems like a good point, but we'll have to do some testing before we can say for certain if we want to make a change. My gut feeling is that this seems like a good idea, but I can't promise anything yet.

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

No branches or pull requests

2 participants