[openssl-dev] [openssl.org #4602] Missing accessors

[Kurt Roeckx via RT]

Hi,

I received the following bug in debian:
https://bugs.debian.org/829272


I got a lot of bugs filed about packages FTBFS with openssl 1.1.0.
I started to look at some of them, and many of them are due too
structures having been made opaque. In many cases accessors already
exists, but definitely not for all.

Here is a list of accessors I so far have identified as missing. The
filenames given in the "Add to ..." comments below are suggestions
based on where similar functions are defined and implemented.


/* Add to include/openssl/x509_vfy.h : */

typedef int (*X509_STORE_CTX_get_issuer)(X509 **issuer, X509_STORE_CTX *ctx, X509 *x);
typedef int (*X509_STORE_CTX_check_issued)(X509_STORE_CTX *ctx, X509 *x, X509 *issuer);

void X509_STORE_CTX_set_get_issuer(X509_STORE_CTX *ctx,
X509_STORE_CTX_get_issuer get_issuer);
X509_STORE_CTX_get_issuer X509_STORE_CTX_get_get_issuer(X509_STORE_CTX *ctx);
void X509_STORE_CTX_set_check_issued(X509_STORE_CTX *ctx,
X509_STORE_CTX_check_issued check_issued);
X509_STORE_CTX_check_issued X509_STORE_CTX_get_check_issued(X509_STORE_CTX *ctx);


/* Add to crypto/x509/x509_vfy.c : */

void X509_STORE_CTX_set_get_issuer(X509_STORE_CTX *ctx,
X509_STORE_CTX_get_issuer get_issuer)
{
ctx->get_issuer = get_issuer;
}

X509_STORE_CTX_get_issuer X509_STORE_CTX_get_get_issuer(X509_STORE_CTX *ctx)
{
return ctx->get_issuer;
}

void X509_STORE_CTX_set_check_issued(X509_STORE_CTX *ctx,
X509_STORE_CTX_check_issued check_issued)
{
ctx->check_issued = check_issued;
}

X509_STORE_CTX_check_issued X509_STORE_CTX_get_check_issued(X509_STORE_CTX *ctx)
{
return ctx->check_issued;
}


/* Add to include/openssl/x509v3.h */

void X509_set_extension_flags(X509 *x, uint32_t ex_flags);
void X509_clear_extension_flags(X509 *x, uint32_t ex_flags);


/* Add to crypto/x509v3/v3_purp.c */

void X509_set_extension_flags(X509 *x, uint32_t ex_flags)
{
x->ex_flags |= ex_flags;
}

void X509_clear_extension_flags(X509 *x, uint32_t ex_flags)
{
x->ex_flags &= ~ex_flags;
}


Regarding the new locking. Do I understand it correctly that e.g.

CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE);

should be replaced by something like

CRYPTO_THREAD_write_lock(X509_STORE_get_lock(ctx));

But then the accessors to get hold of the lock objects in the opaque
structs are missing. E.g.

/* Add to some header file */

CRYPTO_RWLOCK *X509_STORE_get_lock(X509_STORE *ctx);

/* Add to some implementation file */

/* Add to crypto/x509/x509_lu.c */

CRYPTO_RWLOCK *X509_STORE_get_lock(X509_STORE *v)
{
return v->lock;
}

Repeat for other relevant classes with locks.

Mattias

[Richard Levitte via RT]

Post by Kurt Roeckx via RT
Hi,
https://bugs.debian.org/829272
I got a lot of bugs filed about packages FTBFS with openssl 1.1.0.
I started to look at some of them, and many of them are due too
structures having been made opaque. In many cases accessors already
exists, but definitely not for all.
Here is a list of accessors I so far have identified as missing. The
filenames given in the "Add to ..." comments below are suggestions
based on where similar functions are defined and implemented.
/* Add to include/openssl/x509_vfy.h : */
typedef int (*X509_STORE_CTX_get_issuer)(X509 **issuer, X509_STORE_CTX
*ctx, X509 *x);
typedef int (*X509_STORE_CTX_check_issued)(X509_STORE_CTX *ctx, X509
*x, X509 *issuer);
void X509_STORE_CTX_set_get_issuer(X509_STORE_CTX *ctx,
X509_STORE_CTX_get_issuer
get_issuer);
X509_STORE_CTX_get_issuer X509_STORE_CTX_get_get_issuer(X509_STORE_CTX
*ctx);
void X509_STORE_CTX_set_check_issued(X509_STORE_CTX *ctx,
X509_STORE_CTX_check_issued
check_issued);
X509_STORE_CTX_check_issued
X509_STORE_CTX_get_check_issued(X509_STORE_CTX *ctx);

And I suppose that was only those that particular submitter needed. Looking at
crypto/include/internal/x509_int.h, I can see a whole lot more function
pointers that are unreachable.

Post by Kurt Roeckx via RT
Regarding the new locking. Do I understand it correctly that e.g.
CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE);
should be replaced by something like
CRYPTO_THREAD_write_lock(X509_STORE_get_lock(ctx));
But then the accessors to get hold of the lock objects in the opaque
structs are missing. E.g.
/* Add to some header file */
CRYPTO_RWLOCK *X509_STORE_get_lock(X509_STORE *ctx);
/* Add to some implementation file */
/* Add to crypto/x509/x509_lu.c */
CRYPTO_RWLOCK *X509_STORE_get_lock(X509_STORE *v)
{
return v->lock;
}
Repeat for other relevant classes with locks.

I'll look into all of this.

Cheers,
Richard

--
Richard Levitte
***@openssl.org

[Richard Levitte via RT]

Post by Kurt Roeckx via RT
/* Add to include/openssl/x509_vfy.h : */
typedef int (*X509_STORE_CTX_get_issuer)(X509 **issuer, X509_STORE_CTX
*ctx, X509 *x);
typedef int (*X509_STORE_CTX_check_issued)(X509_STORE_CTX *ctx, X509
*x, X509 *issuer);
void X509_STORE_CTX_set_get_issuer(X509_STORE_CTX *ctx,
X509_STORE_CTX_get_issuer
get_issuer);
X509_STORE_CTX_get_issuer X509_STORE_CTX_get_get_issuer(X509_STORE_CTX
*ctx);
void X509_STORE_CTX_set_check_issued(X509_STORE_CTX *ctx,
X509_STORE_CTX_check_issued
check_issued);
X509_STORE_CTX_check_issued
X509_STORE_CTX_get_check_issued(X509_STORE_CTX *ctx);

For this part, https://github.com/openssl/openssl/pull/1294

--
Richard Levitte
***@openssl.org

[Richard Levitte via RT]

Post by Kurt Roeckx via RT
/* Add to include/openssl/x509v3.h */
void X509_set_extension_flags(X509 *x, uint32_t ex_flags);
void X509_clear_extension_flags(X509 *x, uint32_t ex_flags);
/* Add to crypto/x509v3/v3_purp.c */
void X509_set_extension_flags(X509 *x, uint32_t ex_flags)
{
x->ex_flags |= ex_flags;
}
void X509_clear_extension_flags(X509 *x, uint32_t ex_flags)
{
x->ex_flags &= ~ex_flags;
}

This gives me the heebie jeebies. ex_flags is used a lot internally, and I
can't begin to imagine the consequences of letting external code manipulate
this. I understand that in some cases, it seems easy and quick, but...

So, if someone else wants to have a go at this and can make something sensible,
please be my guest. Me, I'm backing off from this particular idea.

Cheers,
Richard

--
Richard Levitte
***@openssl.org

[Salz, Rich via RT]

I think we should ask kurt to ask the original reporter what they need to do.

[Richard Levitte via RT]

Post by Kurt Roeckx via RT
/* Add to some header file */
CRYPTO_RWLOCK *X509_STORE_get_lock(X509_STORE *ctx);
/* Add to some implementation file */
/* Add to crypto/x509/x509_lu.c */
CRYPTO_RWLOCK *X509_STORE_get_lock(X509_STORE *v)
{
return v->lock;
}

https://github.com/openssl/openssl/pull/1295

Post by Kurt Roeckx via RT
Repeat for other relevant classes with locks.

This has to be considered carefully. For most opaque types, it makes zero sense
to give out the lock. However, for opaque types that contain function pointers
that leads to user code, I can see scenarios where it does make sense.

For this case, I've only considered X509_STORE.

--
Richard Levitte
***@openssl.org

[Kurt Roeckx]

Post by Richard Levitte via RT

Post by Kurt Roeckx via RT
/* Add to include/openssl/x509v3.h */
void X509_set_extension_flags(X509 *x, uint32_t ex_flags);
void X509_clear_extension_flags(X509 *x, uint32_t ex_flags);
/* Add to crypto/x509v3/v3_purp.c */
void X509_set_extension_flags(X509 *x, uint32_t ex_flags)
{
x->ex_flags |= ex_flags;
}
void X509_clear_extension_flags(X509 *x, uint32_t ex_flags)
{
x->ex_flags &= ~ex_flags;
}

This gives me the heebie jeebies. ex_flags is used a lot internally, and I
can't begin to imagine the consequences of letting external code manipulate
this. I understand that in some cases, it seems easy and quick, but...
So, if someone else wants to have a go at this and can make something sensible,
please be my guest. Me, I'm backing off from this particular idea.

Mattias,

Can you explain why this is needed, what the code is trying to do?


Kurt

[Kurt Roeckx via RT]

Post by Richard Levitte via RT

Post by Kurt Roeckx via RT
/* Add to include/openssl/x509v3.h */
void X509_set_extension_flags(X509 *x, uint32_t ex_flags);
void X509_clear_extension_flags(X509 *x, uint32_t ex_flags);
/* Add to crypto/x509v3/v3_purp.c */
void X509_set_extension_flags(X509 *x, uint32_t ex_flags)
{
x->ex_flags |= ex_flags;
}
void X509_clear_extension_flags(X509 *x, uint32_t ex_flags)
{
x->ex_flags &= ~ex_flags;
}

This gives me the heebie jeebies. ex_flags is used a lot internally, and I
can't begin to imagine the consequences of letting external code manipulate
this. I understand that in some cases, it seems easy and quick, but...
So, if someone else wants to have a go at this and can make something sensible,
please be my guest. Me, I'm backing off from this particular idea.

Mattias,

Can you explain why this is needed, what the code is trying to do?


Kurt

[Richard Levitte via RT]

In addition to github PR 1294, there's now also PR 1339 which adds the function to set the EXFLAG_PROXY flag on a given certificate.

Also, PR 1295 has been updated. Instead of a function that returns a lock, there is now a lock and an unlock function.

To me, it seems that that covers what's being asked for. Perhaps not exactly (the setters are for X509_STORE only), but should be workable.

(writing this from my mobile, sorry for the lack of github links)
--
Richard Levitte
***@openssl.org