Reuse of AES initialization vector in AESCipher / UsernamePasswordMako / Server

[obi1kenobi]

The Server class randomly generates a fixed 16-byte initialization vector (IV) for the purpose of encrypting data. Then, via the UsernamePasswordMako class, that fixed IV makes its way to the AESCipher class, where it is consistently reused for encrypting data.

Initialization vector reuse like this is a security concern, since it leaks information about the encrypted data to attackers, regardless of the encryption mode used. For example, if the IV is reused with the same key in AES-CTR mode, the attacker will very likely be able to entirely decrypt the encrypted data: https://crypto.stackexchange.com/questions/2991/why-must-iv-key-pairs-not-be-reused-in-ctr-mode

Instead of relying on a fixed, randomly-generated IV, it would be better to randomly-generate a new IV for every encryption operation. Here are a couple of links that have more information on why that is the preferred approach:

• https://defuse.ca/cbcmodeiv.htm
• https://crypto.stackexchange.com/questions/39615/best-way-to-generate-a-iv-for-aes-cbc-when-encrypting-files

[spaceone]

CC

[obi1kenobi]

This issue has been assigned CVE-2017-1000246.

[jkakavas]

Hi there, thanks for the interesting report and congrats on getting your CVE.

Although I agree with you in principle and we are going to fix this shortly, I'm trying to look into the applicability of an attack based on your report and I'd appreciate your feedback.

. For example, if the IV is reused with the same key in AES-CTR mode,

We do not use or support CTR mode
https://github.com/rohe/pysaml2/blob/4375361939e942c4dd666d3ca4e1159858404bc4/src/saml2/aes.py#L11-L15

Even if the attacked could decrypt the data, the only thing we encrypt is the username of the authenticated user before we store that to a cookie. This username is not secret ( in fact it would probably be included in a possibly non encrypted SAML Assertion a few steps down the line ) and I would be more concerned with the attacks against the integrity(authenticity) of the value rather than its confidentiality. **

For CBC mode, since the username vocabulary is limited, I would argue that it would be difficult to mount a chosen plaintext attack ( plaintext = username) and thus, even though the IV reuse is bad practice, I don't see how anyone using this code would be vulnerable to any attacks.

What do you think @obi1kenobi ?

** We don't do a pretty good job with that too admittedly, but I will be opening a separate issue for that.

[obi1kenobi]

Hi @jkakavas, thanks for the reply.

The issue exists inside the class that handles AES encryption generically for the entire library, and not just for encrypting usernames. I'd argue that even if usernames are indeed the only thing being encrypted right now, it isn't safe to assume that will continue to be the case in the future.

I agree that attacks on the message authenticity are concerning, as you point out.

I'd suggest three things:

• Remove support for ECB mode, since it essentially can't be used safely (also note that it currently points to the CFB mode's constant).
• Remove the IV argument to the AESCipher class, and instead randomly generate the IV for each encryption.
• Switch to AES-GCM mode as the default, thereby ensuring authenticity as well as confidentiality.

The IV reuse problem must be fixed before switching to AES-GCM though, since AES-GCM has a similar construction to AES-CTR and fails in the same way when the IV is reused. Also, you might consider AES-GCM-SIV, which is resistant to reused IVs but is slightly more expensive in terms of computation time.

[jkakavas]

The issue exists inside the class that handles AES encryption generically for the entire library, and not just for encrypting usernames. I'd argue that even if usernames are indeed the only thing being encrypted right now, it isn't safe to assume that will continue to be the case in the future.

Yes, we only use it to encyrpt the username in the cookie, so it is not currently something that can e exploited. I agree though that it should be changed so we will not unknowingly inject any vulnerabilities in future versions.

I will make a pull request to address this. Thanks again for reporting it.

[obi1kenobi]

Sounds great -- thank you for looking into fixing this.

[kylegibson]

@jkakavas Is there a PR addressing this?

[jkakavas]

@kylegibson No, not yet. I can work on it soon-ish but if you have something you can contribute, please do by all means

[dcripplinger]

Was this issue ever resolved? My team is working on resolving all the CVEs, and we're not sure if simply upgrading the version does it, or what commit fixes it if any, or if we have a solid reason for ignoring it.

[obi1kenobi]

@dcripplinger as far as I can tell, the issue appears to not be resolved, at least not on the master branch. I believe it is still a problem that should be addressed, and it should be addressable by simply generating a new IV for every encryption operation.

[laserlemon]

@jkakavas 👋 Hello! I'm on the GitHub team responsible for sending security alerts for vulnerable versions of Python libraries. It appears that the issue described here (assigned CVE-2017-1000246) affects all versions as of its writing (<= 4.4.0). Since its writing, version 4.5.0 was released. Can you share whether version 4.5.0 fixes this issue?

As it stands now, we plan to alert pysaml2 users today on vulnerable versions <= 4.5.0 (all currently released versions), providing no remediation steps as no fixed version has yet been identified. Can you please confirm that this information is accurate? Thank you! 💛

[c00kiemon5ter]

I am not a cryptographer so I cannot easily decide whether using AES-GCM or AES-GCM-SIV (as proposed by @obi1kenobi ) is the right thing.

Instead of relying on a fixed, randomly-generated IV, it would be better to randomly-generate a new IV for every encryption operation

However, to generate a new random IV for every encryption operation, one only needs not provide an IV in the first place. I will work on @obi1kenobi proposals immediately.

---

we plan to alert pysaml2 users today

@laserlemon sending security alerts is really helpful and mindful to do. It is great to have a team like that and thanks for doing that 👍 A security alert though, usually panics people, and as such it should be of actual value; it should be an actual issue. Even though there is a CVE assigned, there is nothing of real interest here as @jkakavas already explained earlier. I wouldn't like to wake up with a security alert that ends up not being an actual issue.

Ofcourse, this is no excuse by the pysaml2-team not having acted upon this for so long 😟. I am here to fix this.

---

[offtopic] 🐰

I would be really interested to that list. I would like to be able to contact users of pysaml2 to understand their use cases and alert for breaking changes. Is this something the pysaml2-team can have access to? Can we generate that list somehow?

---

Again, thanks for doing this
I will prepare a PR soon.