Heat does home-grown symmetric crypto (AES-CFB) for no apparent reason

[Laurens Van Houtven]

In the following commit:

https://github.com/openstack/heat/commit/58cd52624b50476ed5ed1c5c0ba7cb1b4d7ba66d

... a decision was introduced to encrypt authentication information using unauthenticated AES-CFB.

There's a few things I don't like about that commit, but suffice to say that heat/engine/auth.py should probably not be a place where symmetric crypto decisions are made.

I've been told that there's a new public API for symmetric encryption, SymmetricCrypto that lives in openstack/common/crypto/utils.py: https://github.com/openstack/oslo-incubator/blob/master/openstack/common/crypto/utils.py#L99

I think that also gets a few things wrong, but at the very least Heat should use a centralized thing for encrypting stuff.

(I'd love to complain about and work on SymmetricCrypto too, but that's not this ticket :)

[Thierry Carrez (ttx)]

I think that affects heat rather than heat-templates

[Thierry Carrez (ttx)]

Sounds like you object about a bad security practice and design, not really an exploitable vulnerability. Is that correct ? If yes, we'll probably make this bug public.

[Laurens Van Houtven (lvh)]

Yes, I think so. I certainly haven't attempted to exploit it. I don't know enough about the code around it to decide if it's exploitable :)

[Thierry Carrez (ttx)]

Will wait until Heat team chimes in before opening it up

[Steve Baker (steve-stevebaker)]

As there is no known exploit I have no objection to this being public.

[Bryan D. Payne (bdpayne)]

I completely agree with the OP that this is a bad security practice. I would also be interested in a discussion about his concerns about the implementation in Oslo.

[Laurens Van Houtven (lvh)]

Should my opinion on what has to happen in Oslo go here?

I think the resolution for *this* ticket is that Heat should use https://github.com/openstack/oslo-incubator/blob/master/openstack/common/crypto/utils.py#L99. Then, we make *that* good.

So, my opinions on that, which I think should also be in a different ticket:

Crypto issues:

- The crypto primitive should probably be NaCl/libsodium's crypto_secretbox(_open). That's Salsa20 with a Poly1305-AES authenticator, but you shouldn't care too much about that :)
- os.urandom should be used exclusively as a CSPRNG. User-level CPRNGs can certainly be done right, but there's plenty of ways to get them wrong. (Say, PyCrypto2.6.0, sometimes, when forking. And I don't even want to remember what I have to do to use RAND_bytes(3) properly in all cases). Since you eventually always rely on /dev/(u)random to get your seed anyway, I don't see the point. FWIW, cperciva disagrees with me, and he's smarter than me; but tptacek agrees with me, and he's smarter than me too. cperciva's arguments *might* be valid in Heat, since he's suggesting same-host entropy use leaks. I'm not convinced those are real attacks. I am convinced that your CSPRNG being broken leads to real attacks ;)
- The padding routine in Oslo isn't constant time. This leaks information about the ciphertext length mod block size. In a system emitting a limited number of messages with predictable sizes, that's pretty bad.
- The unpadding routine in Oslo blindly unpads the ordinal value of the last byte worth of bytes from the plaintext. That's a lot of options for generating valid (parseable) truncated messages.

This is not an exhaustive list. All of the above issues would be fixed by leaving crypto up to NaCl/libsodium, except for key generation (which possibly isn't broken in the first place).

API issues:

- For a symmetric key API, there should not be separate "authenticate" and "encrypt" methods. There should be *only* encrypt, and that should /mean/ encrypt-and-then-MAC-the-ciphertext (EtM). Alternatively, you can also call it "encrypt_and_authenticate" :) The existence of *just* "authenticate" for authenticated plaintext messages is fine. There is no reason to expose the unauthenticated encryption foot-gun. Also, having them be separate methods means that people can get it wrong, for example, they could MtE (first MAC, append MAC to plaintext, then encrypt both), or they could E&M (encrypt plaintext, MAC plaintext, concatenate both). All of these are footguns. The public API should not let you get it wrong.
- The verb "sign" should be reserved for public key signatures.

These two groups of issues are orthogonal.

cheers
lvh

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/59685

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/62295

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/59684
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=37919c6b955e9f9b87d4dc76056efce858c63b1d
Submitter: Jenkins
Branch: master

commit 37919c6b955e9f9b87d4dc76056efce858c63b1d
Author: Angus Salkeld <email address hidden>
Date: Tue Dec 3 21:24:36 2013 +1100

    oslo: add the crypto module

    This is to be used instead of the hand rolled heat/common/crypt.py.

    Partial-bug: #1251647
    Change-Id: I622b9d0c942075f99fdbaff470906123c631504a

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/62295
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=f67cd2182823e4f4173aabd9c67e5ba3b0a20d92
Submitter: Jenkins
Branch: master

commit f67cd2182823e4f4173aabd9c67e5ba3b0a20d92
Author: Angus Salkeld <email address hidden>
Date: Mon Dec 16 19:59:18 2013 +1100

    Add support for multiple encryption methods

    We use only one encrypt() method but it returns the method to decrypt
    the data. This way we can change the encryption mechanism but
    always have a way to know how to decrypt whatever is stored.

    Change-Id: I2315a33105a8766f69d02f0617af39a9dae19ddf
    Partial-bug: #1251647

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/59685
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=e313efce5160ec48b0d7b292dbfea4b2311ebcd3
Submitter: Jenkins
Branch: master

commit e313efce5160ec48b0d7b292dbfea4b2311ebcd3
Author: Angus Salkeld <email address hidden>
Date: Wed Jan 15 12:31:58 2014 +1000

    Use oslo crypto

    Use olso crypto as the new encryption solution.

    Change-Id: I80b76dc5acef5362c49c437bdefdf88f08983fc4
    Closes-bug: #1251647