I spent some time debugging this, as it sounded similar to bug reported for our
project.  I don't think the two are related, but here is what I learned.

This bug is happening when an exception is thrown in
XMLSignature#checkSignatureValue(Key), between the time that
SignatureAlgorithm#initVerify(Key) is called and
SignatureAlgorithm#verify(byte[]) is called, obviously resulting in the latter
never being called.

Because the verify operation is not performed, the (thread local) cached
java.security.Signature instance held in the SignatureAlgorithm instancesVerify
Map is left in a state (in this particular case) with updated data in the
buffer, that is never cleared/reset by a verify operation.

Ordinarily this wouldn't be a problem, because on the next verify op in the same
thread, the java.security.Signature instance would be reinitialized ultimately
with java.security.Signature#initVerify(PublicKey), via a call to
SignatureAlgorithmSpi.  However, due to the optimization that's being done in
SignatureAlgorithm#initVerify(Key), if the key used for the next verify op for
that algorithm in that thread is the *same* key as the previous (failed)
operation, it doesn't call the underlying
SignatureAlgorithmSpi#engineInitVerify(Key).

So the bug is that this optimization implicitly assumes that the previous op
with that key always ran to completion, which is not true in this case.

In the subsequent verification, the data over which the signature is calculated
is *added* to whatever data is in the buffer from the previous failed op,
resulting in a failed verification on an otherwise valid signature.

In this particular test case, the exception is being thrown here:

         //retrieve the byte[] from the stored signature
         byte sigBytes[] = this.getSignatureValue(); 

because the way the invalidly signed test case control document is being
invalidated is by modifying the ds:SignatureValue data, but with what is
syntactically invalid Base64-encoded data - it's not a multiple of 4 bytes and
also has an illegal character.  I suppose it could be argued that this is pretty
unlikely to happen in the real world, but IMHO it seems the library probably
ought to handle it more gracefully. Especially because I think you could get a
similar bug case if for example the code below that writes the signed data into
the SignatureAlgorithm were to throw an exception after having written some data
to the java.security.Signature buffer.


         // Get the canonicalized (normalized) SignedInfo
         SignerOutputStream so=new SignerOutputStream(sa);
         OutputStream bos=new UnsyncBufferedOutputStream(so);
         si.signInOctectStream(bos);
         try {
			bos.close();
		} catch (IOException e) {
			//Imposible
		}



I don't see a clean fix.  Perhaps ideally the SignatureAlgorithm class would
expose a method that could be called from the exception catch block and which
caused the cached algorithm -> key mapping in the keysVerify Map to be removed
or the key reference set to null.  This would ensure that
SignatureAlgorithmSpi#engineVerifyInit would be called in initVerify(Key) on the
next verification, regardless of what the most recently used key was.

An ugly workaround is to ensure that SignatureAlgorithm#verify(byte[]) is always
called.  For example, this "fixes" the problem in this test case by verifying
some dummy bytes before rethrowing the exception:

         //retrieve the byte[] from the stored signature
         byte sigBytes[] = null;
         try {
        	 sigBytes = this.getSignatureValue();
         } catch (XMLSignatureException e) {
        	 sa.verify("dummy-data".getBytes());
        	 throw new XMLSignatureException("empty", e);
         }


Also, I haven't looked at it in detail, but I would think that the signing side
of things might have a similar bug, since it essentially uses the same
optimizations in SignatureAlgorithm as to caching java.security.Signature
instances and conditional re-initialization of those based on the most recently
used Key.

thanks for looking into this ... I agree with your assessment that it is
unlikely to happen in production, and also that the library should handle it
gracefully.

Would it not be as simple as putting a try/catch around:

           byte sigBytes[] = this.getSignatureValue(); 

And then resetting the appropriate values if necessary?

Well, yes, that's basically what I illustrated in my "ugly workaround".  But the
only way to currently reset the appropriate values is to call verify (on some
bytes, it doesn't matter what).  That completes the init/update/verify cycle and
leaves the underlying SignatureAlgorithmSpi (really the wrapped
java.security.Signature/Mac instance) in a known good state.

Re-invoking initVerify(Key) there wouldn't work because it's the source of the
problem in the first place.  You'd have to pass in the current verification key,
b/c that's all you have - and that would be a noop, b/c of the optimization.

Those are the only 2 methods that I can see which cause the underlying engine's
state to be reset.

So other than calling verify on dummy data (less than ideal...), one obvious
option is to expose a new method on SignatureAlgorithm, which causes the
underlying engine Spi instance to be effectively reset (or recreated?) to a
known good state, either prior to or at the time of, the next verification.

Another option, which just occurred to me, is for the engine Spi instance to
keep track of what state it is in with respect to the init/update/verify cycle
(or init/update/sign).  The SignatureAlgorithm could then check this state and
do the right thing if the engine isn't in the "ready" state.  This might be more
desirable, because it alleviates the caller (XMLSignature) from having to be
sure to reset the engine for all cases where exceptions might encountered in the
sign/verify cycles.  This would require augmenting the API of the
SignatureAlgorithmSpi, however.

Hey,

I just got this bug.

I think there may be a quicker fix:

just move the two lines:

         //retrieve the byte[] from the stored signature
         byte sigBytes[] = this.getSignatureValue();

before the try/catch block:

      try {
         SignedInfo si=this.getSignedInfo();


This way, if the Base64 decoding fails, the SignatureAlgorithm hasn't yet been updated and does not need to be reset.
 
This avoids having to call #verify() just to reset the SignatureAlgorithm.



The checkSignatureValue method would look like this:

    public boolean checkSignatureValue(Key pk) throws XMLSignatureException {

        if (pk == null) {
            Object exArgs[] = { "Didn't get a key" };

            throw new XMLSignatureException("empty", exArgs);
        }

        //retrieve the byte[] from the stored signature
        // Do this before calling the SignatureAlgorithm
        // that way if something goes bad, the sa isn't corrupted
        byte sigBytes[] = this.getSignatureValue();

        try {
            SignedInfo si=this.getSignedInfo();
            //create a SignatureAlgorithms from the SignatureMethod inside
            //SignedInfo. This is used to validate the signature.
            SignatureAlgorithm sa =si.getSignatureAlgorithm();
            if (log.isDebugEnabled()) {
                log.debug("SignatureMethodURI = " + sa.getAlgorithmURI());
                log.debug("jceSigAlgorithm    = " + sa.getJCEAlgorithmString());
                log.debug("jceSigProvider     = " + sa.getJCEProviderName());
                log.debug("PublicKey = " + pk);
            }
            sa.initVerify(pk);

            // Get the canonicalized (normalized) SignedInfo
            SignerOutputStream so=new SignerOutputStream(sa);
            OutputStream bos=new UnsyncBufferedOutputStream(so);
            si.signInOctectStream(bos);
            try {
                bos.close();
            } catch (IOException e) {
                //Imposible
            }

            if (!sa.verify(sigBytes)) {
                log.warn("Signature verification failed.");
                return false;
            }

            return si.verify(this._followManifestsDuringValidation);
        } catch (XMLSecurityException ex) {
            throw new XMLSignatureException("empty", ex);
        }
    }

Created attachment 23779 [details]
A patch for this issue


See attached for a patch for this issue.

I added in two new methods to SignatureAction, one to clear the signature caches, and one to clear the verification caches.

The initializing code in XMLSignature for signing and verification is now wrapped in a try/catch that calls the corresponding cache clearing method in SignatureAction on an error, before throwing the exception. So if there's an error here, the Signature "state" is reset before the next attempt and there won't be a problem with a subsequent verification as per the bug.

I've tested it with the submitted test-case and it works fine.

Colm.

Patch applied.

Colm.