Re: [Opensc-devel] [engine_pkcs11] Ajf fixes 201305 (#3)

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

Michael --

Sorry to hear that the patch set is giving you issues.

As a counter-example, I have my patchset running on an embedded system
that grabs then releases the key once a minute, and it can run for
days without memory leaks; before, it was leaking about 2MB/day.

I'll admit that I retrieve my certificates directly from the token
(via libp11), so I never tested the engine interface.

Having said that, I don't recall making any change to the
functionality there, just refactoring / moving code around.  It's been
a while, however.

The main thrust of my patchset was to allow me to use private keys on
the token without leaking massive amounts of memory.  If you can find
a way to integrate the key memory management with Petr's patchset,
that would hopefully be the best of both worlds.

It's not clear to me whether this email thread is echoed to the opensc
devel list; if not, it should be, as there is at least one person
there (Douglas Engert) who is looking at integrating some of these
fixes as well as pushing through other features like EC keys.

I'm busy this afternoon, but I can try to take a look at it this
evening (North America time).

Sorry for the inconvenience.  :(

Good luck,
Tony

p.s. Apologies for top-posting, I'm a little pressed for time right now.

On Mon, Sep 16, 2013 at 7:13 AM, Michael Gebetsroither
<not...@gi...> wrote:
> hi @tkil! i just wanted to try your patches because we needed loading of
> certificates by label, but we had severe problems.
>
> Quick overview:
> 1) Broken loading of certificates, only ever giving back the first
> certificate
> 2) Loading certificates by Label is completely broken
> 3) Corrupted cert structures in function scan_certs parameters, possible
> stack corruption
>
> The whole problem showed signs of stack-corruption and segfaults without
> backtrace longer than 2-3 entries. Please take this with a grain of salt as
> we haven't exactly pinned down the culprint, except that
> https://github.com/petrpisaratlascz/engine_pkcs11 worked for days in our
> case.
>
> We used engine-pkcs11 for curl with client certificates with additional
> intermediate ca needed for it, everything loaded from the token in a long
> running process.
>
> What we have found:
>
> ad 1) Even with matching labels this patchset only ever returns the first
> certificate, not the first matching one. Code giving back a wrong
> certificate without notice.
> scan_certs:
>
> @@ static PKCS11_CERT *scan_certs(PKCS11_CERT *certs,
>                unsigned int cert_count,
>                char *id,
>                unsigned int id_len)
> /* If we couldn't find a match, just use the first one. */
> if (!rv) {
>     rv = certs;
> }
>
> ad 2) and again scan_certs called with what we suspect wrong parameters and
> wrong implementation:
> // the check for id_len not being 0 is pointless as for both call sites the
> size given is from a static buffer from stack
> // id_len is always MAX_VALUE_LEN / 2 (100 here) and the length of a buffer
> on stack in function pkcs11_load_cert/pkcs11_load_key
> // comparing c->id_len against id_len is pointless as both are static here
> and do NOT reflect the containing labels
> // c->id_len is always 20 in our case, and id_len is from a statically
> allocated buffer
> // which brings us to memcmp: this also does not work here because c->id is
> already corrupted
>
> @@ static PKCS11_CERT *scan_certs(PKCS11_CERT *certs,
>                unsigned int cert_count,
>                char *id,
>                unsigned int id_len)
>     if (id_len && c->id_len == id_len &&
>         memcmp(c->id, id, id_len) == 0) {
>         rv = c;
>         cert_num = n;
>     }
>
> The problem it seems is that all those bugs are hidden by point 1, that
> simply the first cert is returned without warning/log entry and not even a
> debug output.
>
> ad 3) Wrong function parameters usage of what the code expects, or at least
> strange things / possible corrupted stack.
> scan_certs:
> PKCS11_CERT *certs all have corrupted labels.
>
> Call stack with own debug output:
>
> pkcs11_load_cert: slot_id="label_bar"
> parse_slot_id_string_aux: slot_id="label_bar" slot_nr=-1 id="" id_len=100
> label="(null)" use="certificate"
> parse_slot_id_string: slot_id="label_bar" slot=-1 id="" id_len=100
> label="(null)"
> Looking in slot 1109423260 for certificate: label: 'bar'
> pkcs11_load_cert: cert_label="bar"
> scan_slots: Num slots: 2
>
> [18446744073709551615] Virtual hotplug slot       no token
> [1] Feitian ePass2003 00 00    login             (mytoken (User PIN))
> scan_slots: Found slot 'Feitian ePass2003 00 00', token 'mytoken (User PIN)'
> scan_certs: cert_count=2 id="" id_len=100
> scan_certs: Found 2 certificates:
> scan_certs: id_len=100 c->id_len=20 id="" c->id=")���Z2�7�K-ȹ� ��"
>    1    foofoofo (/C=XX/CN=xxxxxxxx)
> scan_certs: id_len=100 c->id_len=20 id="" c->id="����ze�� D���ܟ�"
>    2    bar (/C=XX/ST=XX/CN=XXXXXX)
> Did not find a matching certificate, using the first one
> Selecting certificate 0 ("foofoofo")
>
> —
> Reply to this email directly or view it on GitHub.