Please reenable proper encryption support

[baszoetekouw]

In #120, encryption support for EngineBlock was severely limited. Specifically, Shib IdPs with a default setup can no longer be used to authenticate to Engineblock. For SURFconext is is not really a big issue at the moment, but for OpenConext, it is a major problem.

Recap: to work around a chosen-ciphertext attack in the AES-CBC cipher (#116) used for assertion encryption, @relaxnow and @pmeulen implemented a fix: IdPs that encrypt the assertion are now required to sign the entire SAML Response. This properly fixes the problem, as confirmed by @jurajsomorovsky, who was involved in the ROS security assessment on SURFconext.

However, this fix breaks saml2int compliance (as @joostd remarked), and makes Engineblock incompatible with recent Shib IdP versions (which refuse to send unencrypted assertions unless explicitly told to).

Therefore, please reenable support for unsigned, encrypted assertions. In the original thread (#120), @pmeulen suggested the following fix:

My prefered way of fixing this would be to switch to encryption algorithms that are not vulnerable to this kind of attack (adaptive chosen ciphertext attack) like RSA-OAEP and AES-GCM and disable support for the vulnerable PKCS#1 v1.5 encryption and CBC-* mode algorithms. This way we solve the problem at the origin.

However, @jurajsomorovsky in #116 remarked:

This algorithm is required and you are supporting it, otherwise I would not be able to test it.

which suggest that removing support for AES-CBC altogether might also break standards-compliance.

Another way to solve this might be to address the original issue (#116):

Because the error output for a failure to decrypt differs from a failure to verify the signature the output may be used as an oracle along with a chosen plaintext to guess the cipher.

which should be easy to fix.

[baszoetekouw]

@pmeulen , @thijskh, @quartje and me discussed this over lunch. Our conclusion was that the current behaviour (i.e., requiring signed SAML if the attribute payload is encrypted) is sane, and going back to the old behaviour is not a sustainable solution. There are two possible way going forward:

1. Introduce the current EB behaviour in saml2int, and convince Shib upstream to support this; and/or 2.Check to see if there is a usable cipher that is supported by popular IdPs (ie., Shib, ADFS, SSP) that is not vulnerable to padding attacks.

[baszoetekouw]

Ongoing progress on how to fix this bug is kept in https://wiki.surfnet.nl/display/OpenConext/Ciphers+en+encryption

[baszoetekouw]

I've concluded my investigation, and afraid the news is not good. Of the three main IdP product (ADFS, SSP, Shib), only Shibboleth support aes-gcm ciphers. Neither ADFS or SSP support anything else than aes-cbc. Therefore, the easy way to fix this bug (namely: require IdPs to use aes-gcm instead of aes-cbc ciphers) isn't going to work.

[thijskh]

Do you agree that this is fixed since EB 5.1?

[baszoetekouw]

I don't know. Has anyone tested encrypted IdPs recently? @surfnet-niels?

[thijskh]

I've concluded my investigation, and afraid the news is not good. Of the three main IdP product (ADFS, SSP, Shib), only Shibboleth support aes-gcm ciphers. Neither ADFS or SSP support anything else than aes-cbc. Therefore, the easy way to fix this bug (namely: require IdPs to use aes-gcm instead of aes-cbc ciphers) isn't going to work.

This is now no longer the case I think as AES-GCM is now relatively mainstream.

[thijskh]

Also signing of responses is now mainstreamly available.

I propose to close this now and 'fix' encryption (if any needed) when someone has a concrete need that can also directly confirm or deny which approach works for them.

[tvdijen]

Demand for encryption is growing with each cloud-service we connect... Along with Artifact-binding..

[thijskh]

Encryption support is there.. so by all means try to use it so we know what to change to fix it (if any)