Unclear docs -- request clarification on X509_STORE_add_cert

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Unclear docs -- request clarification on X509_STORE_add_cert

Karl Denninger

Assume the following code snippet:

const unsigned char a_cert = {....... }; (A DER certificate we wish to load into the context's chain storage)
int size_a_cert = sizeof(a_cert);

const unsigned char *cp;

X509 *cc_cert;

X509_STORE *cc = SSL_CTX_get_cert_store(a_context);
if (cc == NULL) {
    panic ("Cannot get chain; fail");
}
cp = a_cert;
cc_cert = d2i_X509(NULL, &cp, size_a_cert);
if (cc_cert == NULL) {
      panic("Cert not valid");
}
if (!X509_STORE_add_cert(cc, cc_cert)) {        /* Push the cert into the chain store */
     panic ("Cannot add required chain certificate");
}

/*  X509_free(cc_cert); */

The question is the last line and whether it should be there (uncommented) -- does the X509_STORE_add_cert call load the *reference* or does it load the *data* (allocating whatever it needs internally to do so)?  In other words do I need to keep that X509 structure around that got allocated by the d2i_X509 call or do I free it after I've pushed it into the store?

The docs are silent on this as far as I can tell but some example code I've seen floating around doesn't free it.

--
Karl Denninger
[hidden email]
The Market Ticker
[S/MIME encrypted email preferred]

--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Unclear docs -- request clarification on X509_STORE_add_cert

Dave Coombs
Hello,

Looking at the code in x509_lu.c, X509_STORE_add_cert() takes ownership of your X509 *cc_cert -- you don't need to (and probably shouldn't) free it.

Cheers,
  -Dave


On Jan 2, 2018, at 19:38, Karl Denninger <[hidden email]> wrote:

Assume the following code snippet:

const unsigned char a_cert = {....... }; (A DER certificate we wish to load into the context's chain storage)
int size_a_cert = sizeof(a_cert);

const unsigned char *cp;

X509 *cc_cert;

X509_STORE *cc = SSL_CTX_get_cert_store(a_context);
if (cc == NULL) {
    panic ("Cannot get chain; fail");
}
cp = a_cert;
cc_cert = d2i_X509(NULL, &cp, size_a_cert);
if (cc_cert == NULL) {
      panic("Cert not valid");
}
if (!X509_STORE_add_cert(cc, cc_cert)) {        /* Push the cert into the chain store */
     panic ("Cannot add required chain certificate");
}

/*  X509_free(cc_cert); */

The question is the last line and whether it should be there (uncommented) -- does the X509_STORE_add_cert call load the *reference* or does it load the *data* (allocating whatever it needs internally to do so)?  In other words do I need to keep that X509 structure around that got allocated by the d2i_X509 call or do I free it after I've pushed it into the store?

The docs are silent on this as far as I can tell but some example code I've seen floating around doesn't free it.

--
Karl Denninger
[hidden email]
The Market Ticker
[S/MIME encrypted email preferred]
--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users


--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
Reply | Threaded
Open this post in threaded view
|

Re: Unclear docs -- request clarification on X509_STORE_add_cert

Viktor Dukhovni
In reply to this post by Karl Denninger


> On Jan 2, 2018, at 7:38 PM, Karl Denninger <[hidden email]> wrote:
>
> The question is the last line and whether it should be there (uncommented) -- does the X509_STORE_add_cert call load the *reference* or does it load the *data* (allocating whatever it needs internally to do so)?  In other words do I need to keep that X509 structure around that got allocated by the d2i_X509 call or do I free it after I've pushed it into the store?
>
> The docs are silent on this as far as I can tell but some example code I've seen floating around doesn't free it.

The store takes ownership of the object (bumps its reference count
when it is added to the store) and so the caller should free it if
no longer needed outside the store.

At first glance I thought that commit:

  c0452248ea1a59a41023a4765ef7d9825e80a62b

changed this in master, but a more careful reading of the
code reveals that the behaviour remains the same (corect).
The behaviour should of course be documented.  Feel free
to open an issue on github.

I should note that taking ownership of the object when added
to the store is the "natural" or "expected" behaviour, and
while this does not "excuse" not documenting it, that should
be the best guess of how the function behaves.

--
        Viktor.

--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
Reply | Threaded
Open this post in threaded view
|

Re: Unclear docs -- request clarification on X509_STORE_add_cert

Viktor Dukhovni
In reply to this post by Dave Coombs


> On Jan 2, 2018, at 8:10 PM, Dave Coombs <[hidden email]> wrote:
>
> Looking at the code in x509_lu.c, X509_STORE_add_cert() takes ownership of your X509 *cc_cert -- you don't need to (and probably shouldn't) free it.

The observation is correct, but the conclusion is wrong.
The object is reference counted, and X509_free() is needed
to avoid a leak (when the store is freed along with the
context).

--
        Viktor.

--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
Reply | Threaded
Open this post in threaded view
|

Re: Unclear docs -- request clarification on X509_STORE_add_cert

Dave Coombs
> The observation is correct, but the conclusion is wrong.
> The object is reference counted, and X509_free() is needed
> to avoid a leak (when the store is freed along with the
> context).

My apologies -- I assumed based on its name that X509_OBJECT_up_ref_count was upping the refcount on the internal X509_OBJECT, which had taken over the X509*, which led to my conclusion that freeing the X509_STORE frees the X509 too.  However, you're right, it ups the refcount on the underlying X509, and so the caller *should* free the underlying object when finished with it.

I've now confirmed with a quick test program and valgrind.

Oops,
  -Dave

--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users
Reply | Threaded
Open this post in threaded view
|

Re: Unclear docs -- request clarification on X509_STORE_add_cert

Karl Denninger
On 1/2/2018 19:36, Dave Coombs wrote:
The observation is correct, but the conclusion is wrong.
The object is reference counted, and X509_free() is needed
to avoid a leak (when the store is freed along with the
context).
My apologies -- I assumed based on its name that X509_OBJECT_up_ref_count was upping the refcount on the internal X509_OBJECT, which had taken over the X509*, which led to my conclusion that freeing the X509_STORE frees the X509 too.  However, you're right, it ups the refcount on the underlying X509, and so the caller *should* free the underlying object when finished with it.

I've now confirmed with a quick test program and valgrind.

Oops,
  -Dave
Thanks.

--
Karl Denninger
[hidden email]
The Market Ticker
[S/MIME encrypted email preferred]

--
openssl-users mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-users

smime.p7s (6K) Download Attachment