OCSP_BASICRESP_verify() in 1.1.0

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

OCSP_BASICRESP_verify() in 1.1.0

Dave Coombs
Hello,

I was fiddling around with OpenSSL 1.1.0 this past weekend, because One Day We'll Need To Upgrade (tm), and ran into the following.

We have some code that uses OCSP_BASICRESP_verify() with 1.0.1 / 1.0.2 to confirm that the signature on an ocsp response is correct.  This is a macro in ocsp.h, which directly accesses the signature, signatureAlgorithm, and tbsResponseData members of the OCSP_BASICRESP structure.  In 1.1.0, this structure is now opaque, but the macros are still present in the public ocsp.h, so any external code that uses this macro can't compile.

I can get around this by copying the struct definitions from ocsp_lcl.h into the external code, but that both defeats the purpose of opaque structures and will cause me problems if the structure contents ever change.

Is the correct solution to use OCSP_basic_verify(), which feels like overkill for my needs (the code in question is *part of* our own path-validation routine), or might there be some other way?

Either way, I hereby report you've got a few macros in a public header that can't possibly work as things stand. :-)

Thanks,
  -Dave

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Matt Caswell-2


On 31/10/17 13:06, Dave Coombs wrote:

> Hello,
>
> I was fiddling around with OpenSSL 1.1.0 this past weekend, because
> One Day We'll Need To Upgrade (tm), and ran into the following.
>
> We have some code that uses OCSP_BASICRESP_verify() with 1.0.1 /
> 1.0.2 to confirm that the signature on an ocsp response is correct.
> This is a macro in ocsp.h, which directly accesses the signature,
> signatureAlgorithm, and tbsResponseData members of the OCSP_BASICRESP
> structure.  In 1.1.0, this structure is now opaque, but the macros
> are still present in the public ocsp.h, so any external code that
> uses this macro can't compile.
>
> I can get around this by copying the struct definitions from
> ocsp_lcl.h into the external code, but that both defeats the purpose
> of opaque structures and will cause me problems if the structure
> contents ever change.
>
> Is the correct solution to use OCSP_basic_verify(), which feels like
> overkill for my needs (the code in question is *part of* our own
> path-validation routine), or might there be some other way?

Can you use OCSP_basic_verify() passing in OCSP_NOVERIFY in the final
"flags" argument? This basically finds the signer certificate and
verifies the signature using OCSP_BASICRESP_verify(), but skips all the
chain validation bit.

> Either way, I hereby report you've got a few macros in a public
> header that can't possibly work as things stand. :-)

Yes - a bug. I'm tempted just to remove them.

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

Re: OCSP_BASICRESP_verify() in 1.1.0

OpenSSL - User mailing list
On 10/31/2017 10:36 AM, Matt Caswell wrote:
>
> On 31/10/17 13:06, Dave Coombs wrote:
>
>> Either way, I hereby report you've got a few macros in a public
>> header that can't possibly work as things stand. :-)
> Yes - a bug. I'm tempted just to remove them.
>

That seems like the best course of action.  It's not like we're actually
removing them from the public API, since they never worked at all (on
this branch).  They were actually "removed"/desupported in the 1.1.0
release, we just didn't realize it at the time.

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Wouter Verhelst
In reply to this post by Matt Caswell-2
Hi Matt,

On 31-10-17 16:36, Matt Caswell wrote:
> Can you use OCSP_basic_verify() passing in OCSP_NOVERIFY in the final
> "flags" argument? This basically finds the signer certificate and
> verifies the signature using OCSP_BASICRESP_verify(), but skips all the
> chain validation bit.
Just wanted to point out that that is, actually, a confusing name for
that flag.

"NOVERIFY" seems to imply that there is no verification being done, at
all. Intuitively one senses that's not right, and that at least some
verification will be done (in casu the signature will still be checked);
but figuring out which part of the verification is being dropped and
which part isn't requires one to read either the library source or the
documentation, both of which are annoying if they can be avoided and do
not help for the readability of code that uses the flag in question.

Might I suggest that this flag be renamed somehow, to something that
makes it more clear what exactly it does?

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Matt Caswell-2


On 31/10/17 16:02, Wouter Verhelst wrote:

> Hi Matt,
>
> On 31-10-17 16:36, Matt Caswell wrote:
>> Can you use OCSP_basic_verify() passing in OCSP_NOVERIFY in the final
>> "flags" argument? This basically finds the signer certificate and
>> verifies the signature using OCSP_BASICRESP_verify(), but skips all the
>> chain validation bit.
> Just wanted to point out that that is, actually, a confusing name for
> that flag.
>
> "NOVERIFY" seems to imply that there is no verification being done, at
> all. Intuitively one senses that's not right, and that at least some
> verification will be done (in casu the signature will still be checked);
> but figuring out which part of the verification is being dropped and
> which part isn't requires one to read either the library source or the
> documentation, both of which are annoying if they can be avoided and do
> not help for the readability of code that uses the flag in question.
>
> Might I suggest that this flag be renamed somehow, to something that
> makes it more clear what exactly it does?
>

I agree its not a great name for it. Unfortunately we are stuck with it
for compatibility reasons. If we renamed it we would break any code that
is currently using it. We could introduce a new flag with a different
name which does the same thing - but I'm not sure that does anything to
make things less confusing.

The best way forward is to document it. It isn't documented at all at
the moment along with a number of other OCSP related functions and
features. PRs welcome for that.

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Jakob Bohm-7
On 31/10/2017 17:26, Matt Caswell wrote:

>
> On 31/10/17 16:02, Wouter Verhelst wrote:
>> Hi Matt,
>>
>> On 31-10-17 16:36, Matt Caswell wrote:
>>> Can you use OCSP_basic_verify() passing in OCSP_NOVERIFY in the final
>>> "flags" argument? This basically finds the signer certificate and
>>> verifies the signature using OCSP_BASICRESP_verify(), but skips all the
>>> chain validation bit.
>> Just wanted to point out that that is, actually, a confusing name for
>> that flag.
>>
>> "NOVERIFY" seems to imply that there is no verification being done, at
>> all. Intuitively one senses that's not right, and that at least some
>> verification will be done (in casu the signature will still be checked);
>> but figuring out which part of the verification is being dropped and
>> which part isn't requires one to read either the library source or the
>> documentation, both of which are annoying if they can be avoided and do
>> not help for the readability of code that uses the flag in question.
>>
>> Might I suggest that this flag be renamed somehow, to something that
>> makes it more clear what exactly it does?
>>
> I agree its not a great name for it. Unfortunately we are stuck with it
> for compatibility reasons. If we renamed it we would break any code that
> is currently using it. We could introduce a new flag with a different
> name which does the same thing - but I'm not sure that does anything to
> make things less confusing.
>
> The best way forward is to document it. It isn't documented at all at
> the moment along with a number of other OCSP related functions and
> features. PRs welcome for that.
>
> Matt
You could introduce the new name, but define the old name to it, and
document that the flag is alsoavailable under the other name for
backwards compatibility.  Then code that doesn't need compatibility with
1.1.0 or older can just use the new name.

As for the macro that doesn't work, wouldn't it be better to make it
a function (or a wrapper around the call with the badly named flag).
One could just as easily argue that the API was accidentally broken,
not accidentally kept.  After all, the references to internal structures
is internal to the inline implementation, not part of the interface.


Enjoy

Jakob
--
Jakob Bohm, CIO, Partner, WiseMo A/S.  https://www.wisemo.com
Transformervej 29, 2860 Søborg, Denmark.  Direct +45 31 13 16 10
This public discussion message is non-binding and may contain errors.
WiseMo - Remote Service Management for PCs, Phones and Embedded

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Wouter Verhelst
In reply to this post by Matt Caswell-2
On 31-10-17 17:26, Matt Caswell wrote:
> I agree its not a great name for it. Unfortunately we are stuck with it
> for compatibility reasons. If we renamed it we would break any code that
> is currently using it. We could introduce a new flag with a different
> name which does the same thing - but I'm not sure that does anything to
> make things less confusing.

You could always keep the old name around and mark it deprecated. GCC
even has a pragma for that -- __attribute__((deprecated)) -- although I
doubt it works on macros (haven't tested that).

I suppose it might be too much of an effort for too little gain, though.

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Matt Caswell-2


On 31/10/17 16:42, Wouter Verhelst wrote:

> On 31-10-17 17:26, Matt Caswell wrote:
>> I agree its not a great name for it. Unfortunately we are stuck with it
>> for compatibility reasons. If we renamed it we would break any code that
>> is currently using it. We could introduce a new flag with a different
>> name which does the same thing - but I'm not sure that does anything to
>> make things less confusing.
>
> You could always keep the old name around and mark it deprecated. GCC
> even has a pragma for that -- __attribute__((deprecated)) -- although I
> doubt it works on macros (haven't tested that).
>
> I suppose it might be too much of an effort for too little gain, though.
>

As a matter of policy we won't deprecate anything more until we do a
1.2.0 release.

If someone wants to create a PR for a new name for this (defining the
old one to point at the new one), then I'd review it. But if we're going
to go to that effort then we should write the documentation as part of
the PR (there seems little sense to me in replacing an undocumented name
which you have to read the source to understand with another
undocumented name that you also have to read the source to understand).

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Dave Coombs
In reply to this post by Matt Caswell-2
Hi Matt, thanks for your response.

>> Is the correct solution to use OCSP_basic_verify(), which feels like
>> overkill for my needs (the code in question is *part of* our own
>> path-validation routine), or might there be some other way?
>
> Can you use OCSP_basic_verify() passing in OCSP_NOVERIFY in the final
> "flags" argument? This basically finds the signer certificate and
> verifies the signature using OCSP_BASICRESP_verify(), but skips all the
> chain validation bit.

If I pass in a STACK_OF(X509) *certs with only the signer's cert in it, and NULL for X509_STORE *st since it won't be used, then I think I should get the desired result, yes, at the cost of ocsp_find_signer(single-entry certs) and the internal creation/destruction of an unused X509_STORE_CTX.  I'd have a small performance hit but it probably wouldn't be too bad.

The alternative would be to change the OCSP_BASICRESP_verify() macro into an externally available function, and then both it and OCSP_basic_verify() could call the former macro, suitably renamed and internally scoped.  Clearly I'd be happy with that, though I understand if you don't want to go that route.

Cheers,
  -Dave

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Matt Caswell-2


On 31/10/17 17:30, Dave Coombs wrote:

> Hi Matt, thanks for your response.
>
>>> Is the correct solution to use OCSP_basic_verify(), which feels like
>>> overkill for my needs (the code in question is *part of* our own
>>> path-validation routine), or might there be some other way?
>>
>> Can you use OCSP_basic_verify() passing in OCSP_NOVERIFY in the final
>> "flags" argument? This basically finds the signer certificate and
>> verifies the signature using OCSP_BASICRESP_verify(), but skips all the
>> chain validation bit.
>
> If I pass in a STACK_OF(X509) *certs with only the signer's cert in it, and NULL for X509_STORE *st since it won't be used, then I think I should get the desired result, yes, at the cost of ocsp_find_signer(single-entry certs) and the internal creation/destruction of an unused X509_STORE_CTX.  I'd have a small performance hit but it probably wouldn't be too bad.

Probably the construction of that ctx is in the wrong place. It should
be later in the function. I can't imagine the ocsp_find_signer() hit is
too great.

>
> The alternative would be to change the OCSP_BASICRESP_verify() macro into an externally available function, and then both it and OCSP_basic_verify() could call the former macro, suitably renamed and internally scoped.  Clearly I'd be happy with that, though I understand if you don't want to go that route.

I did consider it, but I'm not especially keen. I think the intended
application interface here is to use OCSP_basic_verify().
OCSP_BASICRESP_verify() should probably have never been exposed in the
first place.

Matt

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Dave Coombs
>> If I pass in a STACK_OF(X509) *certs with only the signer's cert in it, and NULL for X509_STORE *st since it won't be used, then I think I should get the desired result, yes, at the cost of ocsp_find_signer(single-entry certs) and the internal creation/destruction of an unused X509_STORE_CTX.  I'd have a small performance hit but it probably wouldn't be too bad.
>
> Probably the construction of that ctx is in the wrong place. It should
> be later in the function. I can't imagine the ocsp_find_signer() hit is
> too great.

Having tried this, I now see that my copying the structs from ocsp_lcl.h into the external code masked the fact that the external code is getting the signer's cert beforehand by directly accessing OCSP_BASICRESP->certs (and ->tbsResponseData) anyway, effectively doing what ocsp_find_signer() does.  So it is clear that I will need to be rework this, potentially centred around OCSP_basic_verify(), while remaining ignorant of the signer cert.

It would be nice, though, if the API provided a way to get the signer's certificate.  There is OCSP_resp_get0_signature(), but that only returns the bit string.  Comparable functions in other modules (eg: X509_get0_signature(), X509_REQ_get0_signature(), X509_CRL_get0_signature(), CMS_SignerInfo_get0_algs()) provide a way to get any combination of bit string, algorithm, and signer cert.

Cheers,
  -Dave

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

Re: OCSP_BASICRESP_verify() in 1.1.0

OpenSSL - User mailing list
On 10/31/2017 01:05 PM, Dave Coombs wrote:
>>> If I pass in a STACK_OF(X509) *certs with only the signer's cert in it, and NULL for X509_STORE *st since it won't be used, then I think I should get the desired result, yes, at the cost of ocsp_find_signer(single-entry certs) and the internal creation/destruction of an unused X509_STORE_CTX.  I'd have a small performance hit but it probably wouldn't be too bad.
>> Probably the construction of that ctx is in the wrong place. It should
>> be later in the function. I can't imagine the ocsp_find_signer() hit is
>> too great.
> Having tried this, I now see that my copying the structs from ocsp_lcl.h into the external code masked the fact that the external code is getting the signer's cert beforehand by directly accessing OCSP_BASICRESP->certs (and ->tbsResponseData) anyway, effectively doing what ocsp_find_signer() does.  So it is clear that I will need to be rework this, potentially centred around OCSP_basic_verify(), while remaining ignorant of the signer cert.
>
> It would be nice, though, if the API provided a way to get the signer's certificate.  There is OCSP_resp_get0_signature(), but that only returns the bit string.  Comparable functions in other modules (eg: X509_get0_signature(), X509_REQ_get0_signature(), X509_CRL_get0_signature(), CMS_SignerInfo_get0_algs()) provide a way to get any combination of bit string, algorithm, and signer cert.
>

Kind of like https://github.com/openssl/openssl/pull/4573 ?

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Dave Coombs
>> It would be nice, though, if the API provided a way to get the signer's certificate.  There is OCSP_resp_get0_signature(), but that only returns the bit string.  Comparable functions in other modules (eg: X509_get0_signature(), X509_REQ_get0_signature(), X509_CRL_get0_signature(), CMS_SignerInfo_get0_algs()) provide a way to get any combination of bit string, algorithm, and signer cert.
>
> Kind of like https://github.com/openssl/openssl/pull/4573 ?

Quite a lot like that, yes.  Neat.  Is there any chance this might be included in the 1.1.0 series?

Thanks,
  -Dave

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

Re: OCSP_BASICRESP_verify() in 1.1.0

OpenSSL - User mailing list
On 11/01/2017 09:52 AM, Dave Coombs wrote:
>>> It would be nice, though, if the API provided a way to get the signer's certificate.  There is OCSP_resp_get0_signature(), but that only returns the bit string.  Comparable functions in other modules (eg: X509_get0_signature(), X509_REQ_get0_signature(), X509_CRL_get0_signature(), CMS_SignerInfo_get0_algs()) provide a way to get any combination of bit string, algorithm, and signer cert.
>> Kind of like https://github.com/openssl/openssl/pull/4573 ?
> Quite a lot like that, yes.  Neat.  Is there any chance this might be included in the 1.1.0 series?
>

Since there have been no reviews yet, it's easy enough for me to add the
"1.1.0" label and see if a reviewer is persuaded that it is relevant there.

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

Re: OCSP_BASICRESP_verify() in 1.1.0

Wouter Verhelst
In reply to this post by Matt Caswell-2
On 31-10-17 17:47, Matt Caswell wrote:

>
>
> On 31/10/17 16:42, Wouter Verhelst wrote:
>> On 31-10-17 17:26, Matt Caswell wrote:
>>> I agree its not a great name for it. Unfortunately we are stuck with it
>>> for compatibility reasons. If we renamed it we would break any code that
>>> is currently using it. We could introduce a new flag with a different
>>> name which does the same thing - but I'm not sure that does anything to
>>> make things less confusing.
>>
>> You could always keep the old name around and mark it deprecated. GCC
>> even has a pragma for that -- __attribute__((deprecated)) -- although I
>> doubt it works on macros (haven't tested that).
>>
>> I suppose it might be too much of an effort for too little gain, though.
>>
>
> As a matter of policy we won't deprecate anything more until we do a
> 1.2.0 release.

That's a sensible policy, thanks.

> If someone wants to create a PR for a new name for this (defining the
> old one to point at the new one), then I'd review it. But if we're going
> to go to that effort then we should write the documentation as part of
> the PR (there seems little sense to me in replacing an undocumented name
> which you have to read the source to understand with another
> undocumented name that you also have to read the source to understand).

As I ran into this when reviewing how to do OCSP, but ended up not
needing it (I need normal path validation within a limited set of root
certificates), I might look into doing that if/when I find the time for
it some time soon.

Thanks,

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