Improve SignatureException handling #320
Conversation
tst/com/amazon/corretto/crypto/provider/test/EvpSignatureSpecificTest.java
Outdated
Show resolved
Hide resolved
| */ | ||
| @Test | ||
| public void rsaSignatureWrongLength() throws Exception { | ||
| final Signature signer = Signature.getInstance("SHA256withRSA", NATIVE_PROVIDER); |
There was a problem hiding this comment.
should we test this over ECDSA as well?
There was a problem hiding this comment.
Somewhat covered by this, however I think that the new state probably always throws an exception for truncated signatures, so perhaps we should update it.
| * For better compatibility with the JDK, we want to throw SignatureException if the length of the | ||
| * signature is wrong. |
There was a problem hiding this comment.
this may help with JDK compatibility, but it diverges from BC's behavior of returning false rather than throwing.
on one hand, BC's decision to simply return an opaque false leaks less information about the signature and is (IMO) more likely to be more compatible with a given application (presumably, all callers will handle false verify case correctly). on the other hand, SignatureException is a checked exception, so the compiler will force callers to handle it in some way (even if that is a generic catch (Exception e) { ... } with potentially incorrect handling logic).
hoping to have further discussion here with greg and the ACCP team on tradeoffs of favoring JDK compatibility vs. BC compatibility.
There was a problem hiding this comment.
In an ideal world, it would only be false or true, but unfortunately that isn't the world we're in.
We don't need to worry about "leaking information" because every piece of data handled here is public. (Signatures are considered public and public keys are used to validate them.) The concerns are complexity and compatibility.
Complexity: Developers already (unfortunately) need to handle SignatureException and this doesn't make things any worse for them.
Compatibility: The JDK's providers (and tests) define the standard through their behaviors. ACCP cannot match it perfectly, but it can try to minimize the differences. Given a choice between mutually exclusive options of "compatible with JDK providers" and "compatible with BC," I believe that ACCP should usually choose to be compatible with the JDK providers. (Obviously, there may be questions of security and correctness, but neither apply here.)
Finally, I believe it is a reasonably generalization to say "Teams use the JDK providers by default and might choose to use BC." This means that use of BC is more likely to be an intentional decision rather than using the JDK providers. So, if ACCP is being added, we shouldn't change how things work for teams which are just grabbing the default and not paying attention to the details. If a team has chosen to use BC then:
- They are overriding ACCP (so these changes don't matter anyway), OR
- They are making intentional decisions around their cryptography and so can be expected to make adjustments to their code if needed.
Truthfully, many teams use BC without thinking about it (either because of a dependency or just not understanding cryptographic code). But it remains true that using BC is more of a conscious choice than just sticking with the default.
There was a problem hiding this comment.
ACCP-FIPS compatibility with BCFIPS is highly desirable. Customers that have FIPS requirements and want to switch to ACCP-FIPS for performance and still satisfy the FIPS requirements would have simpler transition.
P.S. we have just released ACCP-FIPS for experimental purposes. AWS-LC-FIPS is still waiting for it certificate.
There was a problem hiding this comment.
I still believe that my above positions hold the vast majority of the time.
Careful code needs to handle SignatureException (partially because it is a checked exception).
There is an interesting question as to whether there should be some way to tune ACCP's compatibility towards JDK or BC, but I think the answer there should remain "no."
There was a problem hiding this comment.
I agree with @SalusaSecondus's points. The incompatibility introduced here is trivially resolved by the customers and returning more contextual information in our SignatureExceptions is more helpful than a boolean failure.
There was a problem hiding this comment.
@SalusaSecondus @geedo0 Sounds good to me.
There was a problem hiding this comment.
Regarding the tension between prioritizing JDK vs. BC compatibility, in my view it depends on:
- FIPS mode
- the nature/severity of the incompatibility
I agree that in non-FIPS mode, ACCP is more likely to replace default JDK providers. However, since BCFIPS is the only FIPS-approved JCA provider (as far as I know), I believe ACCP-FIPS is more likely to replace BCFIPS in migration use-cases. In other instances of incompatibility (not this PR, see below), this may warrant differentiating runtime behavior based on FIPS mode to honor the respective compatibility priorities.
For this case specifically, i think SignatureException being a checked exception should give us reasonable confidence that callers already handle it, and is thus unlikely to cause compatibility issues. If this were an unchecked exception, I don't think we could hold the same degree of confidence.
All this is to say, I'm also good with the new behavior.
There was a problem hiding this comment.
Will's guideline is a great mechanism for deciding how incompatibilities should be handled.
| if (key_ instanceof RSAKey) { | ||
| final RSAKey rsaKey = (RSAKey) key_; | ||
| if (length != (rsaKey.getModulus().bitLength() + 7) / 8) { | ||
| throw new SignatureException("RSA Signature of invalid length."); |
There was a problem hiding this comment.
Would it be helpful to report the expected and received lengths?
There was a problem hiding this comment.
Whatever you prefer. I'm happy either way.
There was a problem hiding this comment.
I wouldn't block the merge over it, I'd only encourage it if you're pushing another rev anyway.
WillChilds-Klein
force-pushed
the
sig_ex
branch
from
fc4a8e8
to
094ec72
Compare
Description of changes:
The JCA does not fully specify when SignatureException should be thrown but the ResetAfterException test from OpenJDK indicates that SignatureException should be thrown when validating a too-short signature. This PR modifies ACCP to throw an exception in this case and to always properly reset when exceptions are thrown.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.