Re: [Opensc-devel] ECDSA, engine_pkcs11, libp11 and OpenSSL

[Anthony F. <ant...@gm...>]

Douglas --

I think a few parts of this are aimed at me, but I can't speak to the
whole issue.

On Tue, Sep 24, 2013 at 10:25 AM, Douglas E. Engert <dee...@an...> wrote:
>
> On 9/21/2013 4:45 PM, Markus Kötter wrote:
>> This patch:
>> https://github.com/tkil/engine_pkcs11/commit/91133b719c71d0c92c233a0c29b0d5b94c4ee102
>> reduces the leak by caching the results, not fixing the real problem but
>> masking it.

Heh.  It fixed *my* problem.  :-)

> I have been looking at the PKCS11_* structures too, including the _private parts.

There are other infelicities in that code as well.  I ran into a
situation where the login state was weird (in my projects, users can
enter PIN at almost any time, which might trigger a logout etc).  I
ended up hitting the private section directly; ugly, but it works:

namespace // anonymous
{

struct pkcs11_slot_private {
    PKCS11_CTX *parent;
    unsigned char haveSession, loggedIn;
    /* CK_SLOT_ID */        unsigned long id;
    /* CK_SESSION_HANDLE */ unsigned long session;
};

#if DEBUG_LEVEL >= DEBUG_LEVEL_FINE

void
debugSlotPrivate( const PKCS11_SLOT * slot )
{
    const pkcs11_slot_private * priv =
      static_cast< pkcs11_slot_private * >( slot->_private );

    FINE( "dsp: " << std::boolalpha <<
          "hs=" << !!priv->haveSession << ", "
          "li=" << !!priv->loggedIn << ", "
          "sess=" << priv->session );
}

#else

void
debugSlotPrivate( const PKCS11_SLOT * )
{
}

#endif

} // end namespace [anonymous]

...

    {
        // uhg, this is a huge hack.  the problem seems to be that
        // releasing the key frees all the slots, which in turn
        // terminates all active sessions.  since the session is gone,
        // we have to make sure that libp11 doesn't try to end the
        // session again.

        pkcs11_slot_private * priv =
          static_cast< pkcs11_slot_private * >( m_pUsedSlot->_private );

        if ( priv->haveSession )
        {
            priv->loggedIn = 0;
            priv->haveSession = 0;
        }
    }

> The engine_finish could free everything it has. To avoid
> freeing the a cert or key that may still be in use by the
> application, maybe reference counts could be used.

Don't forget to latch into the global engine cleanup at program exit.

The OpenSSL object lifetime rules are HORRIBLE.

> The RSA_METHOD has a finish routine but the ECDSA_METHOD
> does not which could help in the clean up. (This is Alan's complaint.)
> maybe we can live without it...

Or we could propose its addition.  The more similar all the key
objects are (especially for issues like this that are truly
independent of key type), the better off everyone is.

As my own experience is a prefect example, however, much of OpenSSL is
formed by someone hacking at it enough to get their use case working,
then moving on to the next thing.

I can't see any other reason why the lifetime / auxilary data
management of EC vs. RSA keys should differ at all.

> So it looks like your patch above tries to address many of these
> issues, but adds: "One must instead call a new control function:"

As above: it worked for me.  :-/

> You asked:
> "If anyone knows how to properly "subclass" EVP_PKEYs from outside the
> openssl code base, I'd love to learn how."
>
> Is this what you mean...
> The EVP_KEY type has EVP_PKEY_RSA , EVP_PKEY_EC and others...

More or less.  What I was really trying to describe (poorly,
apparently) is true subclassing with virtual functions (specifically,
virtual destructors) as available in C++.

To phrase it in C++, I would love to see something like:

struct EVP_KEY { ... virtual ~EVP_KEY(); ... }
struct EVP_PKEY_RSA : public EVP_KEY { ... virtual ~EVP_PKEY_RSA(); .... }
struct EVP_PKEY_EC: public EVP_KEY { ... virtual ~EVP_PKEY_EC(); ... }

Then, when any application does [the equivalent of]:

    EVP_KEY * key = get_my_fancy_key(...);
    ... // use key for whatever
    delete key; // trigger *virtual* destructor.

I'm not advocating use of C++ in this instance, I'm just trying to use
the well-defined C++ destructor semantics to explain my POV.

For a C-based example, there's the VFS and driver ops in Linux, where
the vtable is explicitly maintained, and subclassing is done by
assigning different pointers -- but we have access to the previous
value, so we can chain calls up to the superclass if necessary.

I did find that at least RSA has a finish method, and it can be
retrieved -- but when I looked, I'm pretty sure there was no way to
set it.  (Or, maybe it was the other way around -- there were public
methods for setting the 'finish' method, but no way to get the
*current* finish method, so it couldn't be chained, and therefore it
wasn't clear just how useful such a method could be.).

> The OpenSSL ECDSA_METHOD does not have a finish() so we may have to live
> without it.

If you're successful in persuading the OpenSSL devs to make some of
these changes (expose structures for use by external code), then
perhaps you can start a discussion for how to help that external code
properly handle lifecycle issues like the ones we're discussing here.

> Your mods combined with the engine_finish could handle this.

As I've said a few times recently, I don't expect to have time to
pursue this work much further (if at all).  The project is of course
welcome to integrate as much of my code in the pull request as they
see fit -- that's why I submitted it, after all.  :)

Best regards (and good luck!),
Anthony "tkil" Foiani