-
Notifications
You must be signed in to change notification settings - Fork 135
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
Check for key mismatch in signature before verifying signature #66
Comments
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 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 |
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 |
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:
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? |
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 |
Hi Jaime,
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.
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.
Agreed
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.
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. |
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 ?
The text was updated successfully, but these errors were encountered: