randombit / botan

Cryptography Toolkit
https://botan.randombit.net
BSD 2-Clause "Simplified" License
2.59k stars 567 forks source link

ECIES_Encryptor::encrypt() throws #2150

Closed DisableAsync closed 5 years ago

DisableAsync commented 5 years ago

Just trying out ECIES:

#include <botan/auto_rng.h>
#include <botan/ecies.h>
#include <botan/ecdh.h>
#include <iostream>

int main()
{
    try
    {
        Botan::AutoSeeded_RNG rng;
        Botan::ECDH_PrivateKey private_key(rng, Botan::EC_Group("secp521r1"));
        Botan::ECDH_PrivateKey private_key2(rng, Botan::EC_Group("secp521r1"));
        Botan::ECIES_System_Params params(private_key.domain(), "KDF1-18033(SHA-512)",
                                          "AES-256/CBC", 32, "HMAC(SHA-512)", 20,
                                          Botan::PointGFp::Compression_Type::COMPRESSED, Botan::ECIES_Flags::CHECK_MODE);
        Botan::ECIES_Encryptor enc(private_key, params, rng);
        enc.set_other_key(private_key2.public_point());
        std::string plain("some random data");
        std::vector<uint8_t> data(plain.data(), plain.data() + plain.size());
        auto ret = enc.encrypt(data, rng);
    }
    catch (const Botan::Invalid_State &e)
    {
        std::cout << e.what() << std::endl;
    }
    catch (const std::exception &e)
    {
        std::cout << e.what() << std::endl;
    }
}

output: Invalid state: state().empty() == false was false in finish:src/lib/modes/cbc/cbc.cpp

tried with 2.11.0 and 2.12.0 releases, results are the same. not sure if i did something wrong? i dont find ECIES example in the docs, so i tried to grab code from test_ecies.cpp

it still throws even if i change dem_algo_spec to other unauthenticated cipher modes, so i think its an issue in the ECIES_Encryptor implementation?

randombit commented 5 years ago

Your code looks fine at first glance. This is probably a bug. I will see if I can get a fix developed in time for 2.12.1. Thanks for reporting this.

mouse07410 commented 5 years ago

I haven’t touched ECIES for ages, and never in Botan - but why is ECIES encryptor initialized/constructed with a private key?

Or am I missing something?

DisableAsync commented 5 years ago

im not sure if this is useful: i just tried all cipher modes, AES-256/SIV and AES-256/CCM work perfectly, without needing to add extra code and others throw all kinds of exceptions :v

randombit commented 5 years ago

@CingKong thanks that's useful information. Interestingly we have many tests with ECIES using CBC mode... but I can reproduce the problem you are seeing on my machine also. So what is different? It makes me wonder if these ECIES tests are working correctly.

@mouse07410 In Botan the private keys inherit the public key so it is implicitly convertible. But yes in a practical usage of course you would only have the public key of the recipient, at least in most cases [*]

[*] You might be encrypting to "yourself", eg for storage, in which case it's possible the key is the private key if it was loaded that way.

randombit commented 5 years ago

Ok found it.

ECIES can use an initialization vector. It does not strictly require one since each key is only used once (due to the key being derived using a key exchange) so for example using a fixed IV every time is actually fine. But if set_initialization_vector is not called, then ECIES calls the Cipher_Mode interface in a way which is not valid for most modes, and in the case of CBC causes the exception you are seeing.

This worked for SIV because SIV explicitly works without a nonce. And it works for CCM because of a bug in CCM, since CCM does require a nonce ;)

I'm going to change ECIES so that it always sets the nonce, even if it was not set. Then if the mode accepts an empty nonce (as CBC and CTR do) then it works. Or otherwise you get an error along the lines of "GCM requires a xx byte nonce" which at least gives a hint at what the problem is.

tl;dr call enc.set_initialization_vector(std::vector<uin8_t>(16)) to use an all-zero CBC IV

mouse07410 commented 5 years ago

Shouldn’t ECIES generate/handle IV automatically, without user intervention?

randombit commented 5 years ago

Probably so. I'm not sure why ECIES was written like this. It may be related to how the ISO ECIES format works. @neusdan @weberph any hints why ECIES was written this way instead of randomly generating a nonce and including it with the ECIES ciphertext?

mouse07410 commented 5 years ago

Regardless of why it was written the way it was - perhaps we should provide the “correct” interface, either instead of or in addition to what’s currently provided?

Update

Here are some references:

https://www.nominet.uk/how-elliptic-curve-cryptography-encryption-works/

http://digital.csic.es/bitstream/10261/32671/1/V2-I2-P7-13.pdf

https://www.shoup.net/papers/iso-2_1.pdf

https://www.secg.org/sec1-v2.pdf

randombit commented 5 years ago

I just noticed that DLIES does the same thing, so maybe when ECIES was written it just took that same interface. So this may actually be my fault. The DLIES code is over 15 years old so I certainly have no memory of why I would have done it like that ... I'd have to track down IEEE 1363

But it also appears that BouncyCastle does the same thing, wrt nonce handling in ECIES.

DisableAsync commented 5 years ago

Had a peek into the implementation, seems the symmetric key is actually derive from ECDH public key. does it mean one can decrypt the cipher text if they holds the public key? 🤔

mouse07410 commented 5 years ago

...symmetric key is actually derive from ECDH public key...

No! Symmetric key is derived from the sender's/encryptor's ephemeral private key and the recipient's/decryptor's long-term/static public key.

Encryptor's ephemeral public key is sent along with the encrypted data.

The decryptor derives/computes the symmetric key from the sender's ephemeral public key, and his static private key.

Take a look at the first reference I posted here.

randombit commented 5 years ago

@CingKong no because ECDH agreement requires two keys, in the case of ECIES one of those is the public key that is acting as the receiver. The other key is randomly generated during encryption, ECDH agreement is performed, and then the private key is destroyed. The ephemeral public key is included in the ciphertext, which allows the receiver to use their private key to perform the same agreement, derive the shared secret and decrypt the message.

weberph commented 5 years ago

Concerning setting the IV: we tried to implement the interface similar to the DLIES implementation. ECIES does not provide a mechanism to transmit the IV. Therefore it may be good to force users to set an IV that is known (instead of generating the IV implicitly)?

The constructor of ECIES_Encryptor accepts a private key, because ECIES may be used with static keys. In most cases, this private key should be an ephemeral key (see comments in header file). Using the other constructor will generate this ephemeral key implicitly.

mouse07410 commented 5 years ago

... it may be good to force users to set an IV that is known...

No! Both the symmetric key, and the IV for the symmetric encryption should be random-generated. See the specs.

The constructor of ECIES_Encryptor accepts a private key, because ECIES may be used with static keys...

No. The constructor of ECIES_Encryptor accepts a private key simply because in this implementation it can be implicitly converted to the correct public key. In general (usually, in real world), you send encrypted data to somebody else, and by definition do not have his/her private key.

In most cases, this private key should be an ephemeral key (see comments in header file)

No. In most cases this should be a static public key of the recipient. Sender's key pair is ephemeral, and is supposed to be under complete control of the ECIES mechanism. I.e., it stays under the hood, invisible to the user.

weberph commented 5 years ago

Both the symmetric key, and the IV for the symmetric encryption should be random-generated.

Of course, I agree that the key and IV must be random-generated. To clarify: if the IV is required for decryption, it must be either included in the message or transmitted by other means.

No. The constructor of ECIES_Encryptor accepts a private key simply because in this implementation it can be implicitly converted to the correct public key.

ECIES_Encryptor uses the private key to initialize ECIES_KA_Operation (m_ka) where it is used in create_key_agreement to create PK_Key_Agreement ("PK Secret Value Derivation Key"). PK_Key_Agreement is then used in ECIES_KA_Operation::derive_secret to derive the secret using the public key of the other party (other_public_key_bin).

No. In most cases this should be a static public key of the recipient.

See above.

Sender's key pair is ephemeral, and is supposed to be under complete control of the ECIES mechanism. I.e., it stays under the hood, invisible to the user.

This is exactly what happens if the other constructor of ECIES_Encryptor is used. See comment in ecies.h: Creates an ephemeral private key which is used for the key agreement.

mouse07410 commented 5 years ago

I agree that the key and IV must be random-generated...

Great. That answers the first part of the argument - IV is provided by the ECIES (rather than taken from the user).

if the IV is required for decryption, it must be either included in the message or transmitted by other means

Certainly. And all the implementations I'm aware of, package into one blob: IV, ephemeral public key, ciphertext, and MAC. While it's technically possible to pass all these four components via separate channels - in 99.999% of the use cases such a separation makes no sense.

No. In most cases this should be a static public key of the recipient.

See above.

I do not understand. Are we discussing how this particular implementation is written, or how ECIES should behave? I'm talking about the correct implementation of the ECIES algorithm/protocol. My purpose is to convince the authors that if the current implementation behaves differently, it should change.

Sender's key pair is ephemeral, and is supposed to be under complete control of the ECIES mechanism. I.e., it stays under the hood, invisible to the user.

This is exactly what happens if the other constructor of ECIES_Encryptor is used...

So, there's a "conformant" API, and a "low-level" access to allow users create their own modifications?

DisableAsync commented 5 years ago

@mouse07410 i have seens many statements like this (in first reference you posted here)

To give an idea of the difference, a 256-bit ECC key can be considered equivalent to a 3072-bit RSA key.

i assume these are about signature algorithm security? then how about the encryption aspect, can we say ECIES with 256 bit key (and AES-256/SIV", HMAC(SHA-512)) is (way) less secure than RSA encryption with 3072 bit key? cuz former is essentially symmetric key encryption?

weberph commented 5 years ago

all the implementations I'm aware of, package into one blob

I think compatibility is very important; if the majority of other implementations do it this way, it might be good to change the current behavior. ISO-18033 does not specify the encoding of the final message. We implemented the encoding similar to the DLIES implementation (dlies.h: output = (ephemeral) public key + ciphertext + tag). This encoding should be compatible to sec1-v2.pdf and iso-2_1.pdf (C = (R, EM, D)).

So, there's a "conformant" API, and a "low-level" access to allow users create their own modifications?

This is the same interface as provided by the DLIES implementation.

I'm talking about the correct implementation of the ECIES algorithm/protocol.

These decisions were made for consistency between DLIES and ECIES. Note that the private key is not used "because in this implementation it can be implicitly converted to the correct public key". This constructor allows using an existing private key object which is used during unit tests for example. The public key has to be set using set_other_key. Having a constructor which accepts the public key of the recipient would be more intuitive.

I am not arguing for or against changing the interface or encoding - just trying to give some hints why it has been implemented this way.

mouse07410 commented 5 years ago

These are about asymmetric crypto algorithms security. The given numbers aren’t Gospel, but a reasonable guideline to consider.

IMHO, your question is mis-phrased. ECIES does not employ digital signature. But it does perform ECDH (key agreement). That’s where the strength of the asymmetric crypto comes in. The rest of the operations indeed use symmetric crypto (where, again, you don’t need to climb up all the way to SHA512).

Attempting to compare cryptographic strength of dissimilar algorithms, people consider AES-128 roughly equivalent to RSA-2048 and ECC-256. AES-256 is “stronger” than RSA-3072 (or it’s equivalent ECC-384).

It is likely that ECC-256 is weaker than RSA-3072, but there are many other aspects to consider - such as what kinds of attacks constitute your threat model.

I’d probably go with ECC-384 and AES-256/GCM-SIV. If not, HMAC(SHA384)) would be sufficient.

Since I personally prefer key agreement to key encapsulation (for many reasons), I probably would not go with encrypting the key with RSA-3072 (though that’s exactly what’s happening to all of my S/MIME emails).

DisableAsync commented 5 years ago

Um, sorry for not being clear, by signature algorithm I mean ECDSA.

Just as claimed in another issue, I literally don't have any cryptography background. I'm trying out Botan with SHA-512 all because randombit in this reply only mentioned SHA-512, and most tests of test_ecies.cpp also use SHA-512.

I'm trying to understand more about cryptography, but there are just too many to be learned. And I haven't really look into SHAs. :v

mouse07410 commented 5 years ago

This issue is about ECIES, which has nothing to do with ECDSA.

Also, comparing ECDSA signature with RSA key encapsulation doesn’t make sense.

If you want to know whether SHA512 is ok - the answer is yes. It would be an overkill, but “very few operations failed because they used excessive force”.

mouse07410 commented 5 years ago

@weberph, speaking of API consistency, everything I said about ECIES API applies to DLIES as well.

I understand that this design makes testing easier. But prioritizing unit-test in API design against the actual usage is a bad idea.

I’m less interested in how this API came to look the way it is now, but in whether it matches what the standard expects (my conjecture is No), and whether it matches the API that other crypto packages provide (provably it doesn’t - look at Crypto++ for C++ and BouncyCastle for Java).

As I already said, I hope to convince the authors to change this API to what the standard intended, and what the other packages already provide - if the authors agree.

As a side-note, in Crypto++ we had no problem writing tests for ECIES, even though it didn’t provide an Encryptor constructor with a private key as an argument.

DisableAsync commented 5 years ago

@mouse07410 , let's not talking about the fact of these two encryption systems being dissimilar for now. In your opinion, is choosing ECIES over RSA encryption generally a bad idea, in terms of security? ECIES looks promising to me because it doesn't employ padding(?) which RSA does that makes the cipher texts crazily long even from tiny pieces of plain texts.

Let's say ECIES uses secp256r1, KDF1-18033(SHA-256), AES-256/SIV, HMAC(SHA-256), and randomly appends some extra nonsense date to the plain text before encrypting (just to make the cipher text different even encrypting same exact data); and RSA are just with 3072bit key.

(critiques expected :P)

mouse07410 commented 5 years ago

@CingKong ECIES is a concrete standard that defines what primitives to use.

Re. RSA encryption - you'd need to invent the whole framework to go around it. ECIES is an EC-based Integrated Encryption System that encrypts data (possibly quite large) to a public key of the recipient. With RSA-3072 you'd be able to encrypt ballpark 3000 bits of data. That doesn't sound like a lot.

ECIES will increase the size of the cipher text too, because in addition to the encrypted plaintext itself, it needs to pass along: ephemeral public key used to generate symmetric keys, IV used to encrypt the data, and MAC to ensure the data was not tampered with.

My personal advice - don't try to re-invent the wheel, as the likelihood of coming up with something more secure than what's standardized is very low, while the likelihood of shooting yourself in the foot is quite high. ECIES with secp384r1, and KDF based on SHA384 should serve you well enough. If you can convince ECIES to use AES-256/SIV for symmetric data encryption - perfect.

DisableAsync commented 5 years ago

That seems fine for me... though if I use secp384r1 for ECDSA, the signature size is only about 96 bytes. Is that normal?

(Cause I wanna use same key for ECDSA and ECIES.)