saml2 icon indicating copy to clipboard operation
saml2 copied to clipboard

Check for key mismatch in signature before verifying signature

Open relaxnow opened this issue 9 years ago • 6 comments

So often I've had to debug a signature failure to discover the wrong key had been used. It would be nice if the KeyInfo from the Signature was used to compare against the public key of the pair we're using to validate the signature.

I can't see any reason not to, can you @jaimeperez or @pmeulen ?

relaxnow avatar Jun 16 '16 06:06 relaxnow

Do you mean when SSP is signing (could be IdP or SP) ? I.e. check that the RSA private key and the public key (from the certificate) used match? Checking the keypair before signing will incur a small overhead for each message signed. Might make more sense to have a "sanity check" do this.

When verifying a signature (could be IdP or SP) SSP must obviously already check whether a trusted certificate (hash) is used during verification. It could/should log a useful message. Doesn't SSP do this already? Logging what is going wrong locally should always be OK.

pmeulen avatar Jun 16 '16 07:06 pmeulen

@pmeulen I mean primarily for verifying a HTTP-POST signature. Most implementations (like SSP) will send the public key they used to sign the message (I think that is not obligatory, but I'd have to consult the spec) in the KeyInfo. Currently the SAML2 ChainedValidator doesn't do anything with this: https://github.com/simplesamlphp/saml2/blob/master/src/SAML2/Signature/AbstractChainedValidator.php#L34 It just runs through all the keys it has and tries them. It should be pretty trivial to see if the signature has a key embedded and see if we have that key, if not at least log something.

relaxnow avatar Jun 16 '16 08:06 relaxnow

Functionally trivial, yes. If there is a certificate provided in the XML signature, see if we trust it, if not explicitly log this as an info message. I agree that this would be useful. It is common to supply the certificate used to sign in the HTTP-POST. It is not mandatory AFAIK.

I've been discussing with @thijskh how/where to implement this. The verification behaviour is rather subtle and more complex than I expected.

Validation uses the first validator in the chain that reports that it can validate the signature: https://github.com/simplesamlphp/saml2/blob/master/src/SAML2/Signature/ValidatorChain.php#L60 So either the https://github.com/simplesamlphp/saml2/blob/master/src/SAML2/Signature/FingerprintValidator.php or the https://github.com/simplesamlphp/saml2/blob/master/src/SAML2/Signature/PublicKeyValidator.php is used. Adding an INFO level log message to both validators that logs when the included certificate seems the way forward. This means that you need to (try to) fetch the certificate from the signature in the PublicKeyValidator.

pmeulen avatar Jul 26 '16 09:07 pmeulen

Hi guys!

If I understand correctly, there are two different issues here: one is the lack of proper logs telling you that the key used to sign a message does not have any correspondence in the list of certificates known by configuration. That should be almost trivial to fix, indeed.

The other issue would be an optimization of the code, inspecting the response to see if there's a certificate attached, matching that with the ones known per configuration, and use that to validate the signature directly, instead of iterating through a list of certificates. This is a bit more problematic, IMHO. Basically, if we do such optimization, we then have two different scenarios with different resource consumption in terms of time:

  • No certificate is present, which means more time to complete since we have to iterate over the list of known certificates.
  • A certificate is present and it matches a known certificate, which takes way less time to complete.

This difference could probably be noticeable and exploitable using the affected endpoint as an oracle. Such a possibility in the context of signature validation makes me trigger all alarms and at least pause for a second and try to analyze what could go wrong. The first idea that comes to my mind is using this to learn which certificates does an entity trust. Not a big issue indeed, since the certificates are published in the metadata and entities are expected to trust that. However, I'm worried about the possibility to use this as a way to guess a valid signature for a forged message (which is probably being too paranoid, but I would prefer not to overlook anything).

Can you imagine possible side effects of this, in terms of security? Or the other way around, are we certain that such optimization would be completely safe? If not, is the optimization really needed?

jaimeperez avatar Jul 26 '16 10:07 jaimeperez

On the -dev m/l I just raised the related question of the value of being able to specify the fingerprint only: https://groups.google.com/forum/#!topic/simplesamlphp-dev/MTbTGxAWniw

thijskh avatar Jul 26 '16 12:07 thijskh

Hi Jaime,

Hi guys!

If I understand correctly, there are two different issues here: one is the lack of proper logs telling you that the key used to sign a message does not have any correspondence in the list of certificates known by configuration. That should be almost trivial to fix, indeed.

Yes, that's @relaxnow 's issue. And it should be about the public key, not the certificate. For SSP the certificate is just an overly complex container for a public key.

The other issue would be an optimization of the code, inspecting the response to see if there's a certificate attached, matching that with the ones known per configuration, and use that to validate the signature directly, instead of iterating through a list of certificates. This is a bit more problematic, IMHO. Basically, if we do such optimization, we then have two different scenarios with different resource consumption in terms of time:

No certificate is present, which means more time to complete since we have to iterate over the list of known certificates.
A certificate is present and it matches a known certificate, which takes way less time to complete.

This difference could probably be noticeable and exploitable using the affected endpoint as an oracle.

Note that the SAML2_Signature_FingerprintValidator as it is currently implemented would basically reveal the same information because it only verifies the signature when the fingerprint of the signing certificate is whitelisted.

Such a possibility in the context of signature validation makes me trigger all alarms and at least pause for a second and try to analyze what could go wrong. The first idea that comes to my mind is using this to learn which certificates does an entity trust. Not a big issue indeed, since the certificates are published in the metadata and entities are expected to trust that.

Agreed

However, I'm worried about the possibility to use this as a way to guess a valid signature for a forged message (which is probably being too paranoid, but I would prefer not to overlook anything).

Good to be careful. In this case how would such an attack work? All the information that SSP uses to verify the signature is information that would already be available to a potential forger: message, signature and certificate.

Can you imagine possible side effects of this, in terms of security? Or the other way around, are we certain that such optimization would be completely safe? If not, is the optimization really needed?

I don't see anything that requires optimization for performance in the current validation code, although I did not profile. Getting rid of the fingerprint mechanism would allow reduction of complexity in code, configuration and documentation. This is more valuable IMO.

That being said, there is an opportunity for performance optimization by immediately selecting the right public key to do the verification, if it is included in the message. This can probably be used to speed up the verification a bit. Typically an entity won't have more than two keys (old & new during key rollover). If the cost of the optimalisation is low (complexity) we could do it anyway. Otherwise we should profile first to see whether it makes sense.

pmeulen avatar Jul 26 '16 15:07 pmeulen