What is the REALLY proper way to use an ENGINE?

[Andrey Kulikov]

If we take a look at any ENGINE_load_XXX function, we find that they all has
similar structure:

ENGINE *toadd = engine_XXX();
if(!toadd) return;
ENGINE_add(toadd);
ENGINE_free(toadd);
ERR_clear_error();

My question is: why we need call ENGINE_free(toadd) ??
Somewhere inside it calls EVP_PKEY_asn1_free(), which besides everything
else desroy all AMETH structures, created during engine initialization via
EVP_PKEY_asn1_set_* .

So, let's consider following example:

CRYPTO_malloc_init();
ERR_load_crypto_strings();
ENGINE_load_builtin_engines();

e = ENGINE_by_id("XXX");
ENGINE_set_default(e, ENGINE_METHOD_ALL);
OpenSSL_add_all_algorithms();
OpenSSL_add_ssl_algorithms();

EVP_PKEY *signing_key = ENGINE_load_private_key(e, NULL, NULL, NULL);

// bla-bla-bla...

And, if in engine load priv func we write something like
EVP_PKEY *pkey = EVP_PKEY_new();
pkey->ameth = ENGINE_get_pkey_asn1_meth(eng, NID_id_SOME_NID);

it's not gonna work, because all ASN1 structures, carefully created by
engine via calls to EVP_PKEY_asn1_set_* are invalid and possibly corrupted
at this moment.

Is it a design issue, or provided example is invalid (but it taken from
openssl sources, hm...)?

[Andrey Kulikov]

Update: adding
ENGINE_init(e)
after
e = ENGINE_by_id("XXX");

doesn't make any difference, as in my case functional reference count is
8(???) at the moment of ENGINE_init(e) call, so engine is not
re-initialised. :(

Post by Andrey Kulikov
If we take a look at any ENGINE_load_XXX function, we find that they all
ENGINE *toadd = engine_XXX();
if(!toadd) return;
ENGINE_add(toadd);
ENGINE_free(toadd);
ERR_clear_error();
My question is: why we need call ENGINE_free(toadd) ??
Somewhere inside it calls EVP_PKEY_asn1_free(), which besides everything
else desroy all AMETH structures, created during engine initialization via
EVP_PKEY_asn1_set_* .
CRYPTO_malloc_init();
ERR_load_crypto_strings();
ENGINE_load_builtin_engines();
e = ENGINE_by_id("XXX");
ENGINE_set_default(e, ENGINE_METHOD_ALL);
OpenSSL_add_all_algorithms();
OpenSSL_add_ssl_algorithms();
EVP_PKEY *signing_key = ENGINE_load_private_key(e, NULL, NULL, NULL);
// bla-bla-bla...
And, if in engine load priv func we write something like
EVP_PKEY *pkey = EVP_PKEY_new();
pkey->ameth = ENGINE_get_pkey_asn1_meth(eng, NID_id_SOME_NID);
it's not gonna work, because all ASN1 structures, carefully created by
engine via calls to EVP_PKEY_asn1_set_* are invalid and possibly corrupted
at this moment.
Is it a design issue, or provided example is invalid (but it taken from
openssl sources, hm...)?

[Dr. Stephen Henson]

Post by Andrey Kulikov
If we take a look at any ENGINE_load_XXX function, we find that they all has
ENGINE *toadd = engine_XXX();
if(!toadd) return;
ENGINE_add(toadd);
ENGINE_free(toadd);
ERR_clear_error();
My question is: why we need call ENGINE_free(toadd) ??

To avoid a memory leak.

When you call engine_XXX() you get a reference to the ENGINE. When it is added
to the ENGINE list the reference count is incremented. When you call
ENGINE_free() the count is decremented and the ENGINE is freed only if the
reference count is zero.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Andrey Kulikov]

Thanks for a explanations.

Let's consider following main, using ccgost engine:

main(){

OPENSSL_config(NULL);
ENGINE *e = ENGINE_by_id("gost");
ENGINE_init(e);
ENGINE_free(e);
ENGINE_set_default(e, ENGINE_METHOD_ALL);
OpenSSL_add_all_algorithms();

// emulating ENGINE_load_private_key()
EVP_PKEY *pkey = EVP_PKEY_new();
pkey->ameth = ENGINE_get_pkey_asn1_meth(e, NID_id_GostR3410_2001);
bla-bla-bla...
//end emulating


EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, e);
}

Result ctx is always NULL.
It happens because ENGINE_get_pkey_asn1_meth() returns pointer to corrupted
internall ccgost structure "ameth_GostR3410_2001".
It IS initialized, then being freed. And contains garbage. Specifically
pkey->ameth->pkey_id contains some random value instead of 811
(NID_id_GostR3410_2001).

Is this code contain some error or invalid engine API usage?

Post by Dr. Stephen Henson

Post by Andrey Kulikov
If we take a look at any ENGINE_load_XXX function, we find that they all

has

Post by Andrey Kulikov
ENGINE *toadd = engine_XXX();
if(!toadd) return;
ENGINE_add(toadd);
ENGINE_free(toadd);
ERR_clear_error();
My question is: why we need call ENGINE_free(toadd) ??

To avoid a memory leak.
When you call engine_XXX() you get a reference to the ENGINE. When it is added
to the ENGINE list the reference count is incremented. When you call
ENGINE_free() the count is decremented and the ENGINE is freed only if the
reference count is zero.
Steve.

[Dr. Stephen Henson]

Post by Andrey Kulikov
Thanks for a explanations.
main(){
OPENSSL_config(NULL);
ENGINE *e = ENGINE_by_id("gost");
ENGINE_init(e);
ENGINE_free(e);
ENGINE_set_default(e, ENGINE_METHOD_ALL);
OpenSSL_add_all_algorithms();
// emulating ENGINE_load_private_key()
EVP_PKEY *pkey = EVP_PKEY_new();
pkey->ameth = ENGINE_get_pkey_asn1_meth(e, NID_id_GostR3410_2001);
bla-bla-bla...
//end emulating
EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, e);
}
Result ctx is always NULL.
It happens because ENGINE_get_pkey_asn1_meth() returns pointer to corrupted
internall ccgost structure "ameth_GostR3410_2001".
It IS initialized, then being freed. And contains garbage. Specifically
pkey->ameth->pkey_id contains some random value instead of 811
(NID_id_GostR3410_2001).
Is this code contain some error or invalid engine API usage?

I can't see how that would happen:

ENGINE_by_id() should up the structural reference.
ENGINE_init() should up the functional and structural reference.
ENGINE_free() should decrement the structural reference.

ENGINE_set_default() should increment both structural and functional
references by an amount which depends on the number of default algorithms
added (which might be zero for some ENGINEs).

ENGINE_load_private_key() should be passed a valid functional reference.

A correct implementation of an ENGINE private key function should call
EVP_PKEY_assign which will also up functional and structural reference counts.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Andrey Kulikov]

I find a way to reproduce it: just call ENGINE_load_builtin_engines() twice.
Or call ENGINE_load_builtin_engines() after OPENSSL_config() which calls it
too.

It happens because internal ccgost AMETH structures are static (which,
obviously, does matter).
First call of ENGINE_load_builtin_engines() creates new ENGINE structure and
store it in some list, etc.

Second call to ENGINE_load_builtin_engines() creates new engine structure
AGAIN, but AMETH part of it reffers to the same static variable
(ameth_GostR3410_2001).
But ENGINE_add() in second call fails, because engine with the same ID
already in the list.
So, reference count not increased, and just created engine released in the
very next line.
AMETH structures being destoryed, and it makes them invalid for originally
created engine too (because there is only one such structure).


Ways to avoid it:
1. Make ccgost engine allocates AMETH structures dynamically.
As there is no other engines, used this functionality (calls to
ENGINE_set_pkey_asn1_meths() ), I can't compare.
But from my point of view this is a bad idea, because it introduces
unnecessary complexity.
And, there is no clear reason to create them more that once.
Of cource we can imagine some engine which provides different NID_id's
depend on configuration. But ccgost is not the one.

2. Make ENGINE_load_builtin_engines() single-callable.
Just like OPENSSL_config().
I can't see any reason why it have to be called more that once.


Would be really appreciated if guru comment on that.

Andrey.