Add simple ASN.1 utils for DSA signature DER by davidmakepeace · Pull Request #9067 · openssl/openssl

davidmakepeace · 2019-06-03T05:32:10Z

This PR adds simple utility functions to allow both the default and fips providers to encode and decode DSA and ECDSA signatures to/from ASN.1 DER without requiring those providers to have a dependency on the asn1 module.

For OpenSSL 3.0, encoding and decoding of algorithm parameters to/from ASN.1 DER will be done in the framework using the asn1 module; the algorithm parameters will be passed between the framework and providers using OSSL_PARAMs.

Encoding/decoding the DSA/ECDSA signatures inside the providers using these utilities looks like being more straightforward than using OSSL_PARAMs to pass the 'r' and 's' values.

I have initially placed these utility functions in crypto/asn1_dsa.c.

Checklist

documentation is added or updated
tests are added or updated

levitte · 2019-06-03T06:45:37Z

I have initially placed these utility functions in crypto/asn1_dsa.c

That's the first thing I noticed 😄
While I understand the tentative choice, it gets confusing to have it in the same spot as the code it "competes" with. Considering this is specifically for the FIPS module, it might be that this code is better located somewhere under providers/fips.

t-j-h · 2019-06-03T06:50:58Z

I don't think this belongs in a fips-specific location - the handling of things using this sort of parser should be done in both fips and non-fips contexts IMHO.

levitte · 2019-06-03T06:51:42Z

As a general comment on the code itself, I assume we will not only encode/decode DSA, so you might as well make the primitive type encoders/decoders (enc_int and dec_int) non-static, and probably rename them to something a little more descriptive.

slontis · 2019-06-03T05:52:07Z

test/asn1_dsa_internal_test.c

Ah, yes, the good ol' copy/paste error. Thanks Shane!
As I pasted it I thought to myself "I had better remember to change that to NULL", and was then immediately distracted by something. :)

I disagree. NULL is for pointers, 0 is for scalars.

@richsalz, the comments here refer to primitives ASN.1 types, and the encoding 0x05, 0x00 is the ASN.1 "NULL". See http://luca.ntop.org/Teaching/Appunti/asn1.html for a quick reminder.

ah, so fix the comment. wasn't clear. https://www.youtube.com/watch?v=OjYoNL4g5Vg

slontis · 2019-06-03T05:53:55Z

test/asn1_dsa_internal_test.c

hmm - is it supposed to pass or fail if there is trailing data? Does pder point to the next bit of data when it returns?

It is supposed to pass if there is trailing data and pder is updated to point to the next bit of data.
I have modified the tests to check that pder is updated correctly.

slontis · 2019-06-03T06:52:24Z

crypto/asn1_dsa.c

Can this fail?

BN_bn2binpad() returns -1 if the supplied buffer is too small.
That should never happen since len is calculated to either be equal to the required size or, when BN_num_bits(n) is a multiple of 8, one greater than the required size; in the latter case the extra zero padding on the left is to ensure that the two's complement encoding of the INTEGER content always represents a positive value.

I think we should test for such failures anyway. Relying on complex "this should never happen" rules and the internal implementation of BN_bn2binpad is brittle. Code changes over time and suddenly we could find that errors might be returned.

Sure. I have added an assert.

slontis · 2019-06-03T06:54:53Z

crypto/asn1_dsa.c

What is the maximum size of cont_len? (it doesnt look like it is size_t).An assumption comment would suffice maybe?

The maximum size is 65535 bytes. That's actually overkill for current DSA groups and EC curves, where 127 is sufficient for the INTEGERs and 255 is sufficient for the SEQUENCE, but I wanted to make it a little more future-proof. The error checking is done in calc_der_len(), which is always called before enc_len().
I will add a comment.

slontis · 2019-06-03T06:59:54Z

crypto/dsa/dsa_asn1.c

allocated var not needed: sig != *psig will do the same thing.

Okay. I got rid of allocated.

slontis · 2019-06-03T07:04:21Z

crypto/include/internal/asn1_dsa.h

It returns -1?

Actually, it is i2d_TYPE() that returns -1 on error, not d2i_TYPE().
d2i_dsa_sig() did return 1 on success and 0 (not -1) on failure, but I have made some changes.
The function is now called decode_der_dsa_sig() and it now returns a size_t: the number of bytes processed on input, or zero in the case of an error. No more i2d_() or d2i_(), since these new functions didn't really conform to the i2d/d2i way of doing things anyway.

slontis · 2019-06-03T07:35:43Z

While I understand the tentative choice, it gets confusing to have it in the same spot as the code it "competes" with. Considering this is specifically for the FIPS module, it might be that this code is better located somewhere under providers/fips.

So you want them in providers/common ?

levitte · 2019-06-03T08:27:34Z

I don't think this belongs in a fips-specific location - the handling of things using this sort of parser should be done in both fips and non-fips contexts IMHO.

There's a difference between the two providers: the default provider relies on and uses libcrypto, the FIPS module doesn't. So if we make this for the default provider, it means we're starting to replace the current ASN.1 library for all, not just for providers.

t-j-h · 2019-06-03T08:51:46Z

And the objective is for the code in a FIPS context and the code in a non-FIPS context to be as close as is possible. We shouldn't unnecessarily have two sets of code for handling the rather simplistic ASN.1 required at this level. And it shouldn't be seen as an ASN.1 replacement - it should be seen as basically handling formatting items at this layer.

levitte · 2019-06-03T10:20:58Z

And it shouldn't be seen as an ASN.1 replacement - it should be seen as basically handling formatting items at this layer.

That was terribly confusing, 'cause this is effectively replacing the DER encoding / decoding, specifically for DSA, in libcrypto.

levitte · 2019-06-03T10:22:13Z

Note: I'm not opposed to this change, I just want to make it explicit what we're actually doing.

crypto/asn1_dsa.c

davidmakepeace · 2019-06-03T13:51:27Z

As a general comment on the code itself, I assume we will not only encode/decode DSA, so you might as well make the primitive type encoders/decoders (enc_int and dec_int) non-static, and probably rename them to something a little more descriptive.

I admit the names could be better, and more descriptive. I'll fix that.
I will make them non-static but note that they currently only work for positive integers since that was all that was needed for the current use case.

levitte · 2019-06-03T13:55:02Z

I will make them non-static but note that they currently only work for positive integers since that was all that was needed for the current use case.

I think that for most of the stuff we do, the numbers are positive, so until we get to a case where that's not true, this is an ok assumption.

You could possibly add an assertion that !BN_is_negative

mattcaswell

Overall this code is very reminiscent of how we used to do packet encode/decode in libssl before we introduced the PACKET/WPACKET APIs. Those APIs were introduced for a reason, i..e because the old patterns were brittle. We saw several CVEs and security issues raised because of OOB reads/writes in various corner cases. While we are writing new code from scratch we have a chance to do it right. I think should we always use PACKET/WPACKET for new packet parsing/encoding. That might require some extensions to them for DER.

Admittedly that is slightly complicated by the fact that PACKET/WPACKET is only available in libssl at the moment (although the design doc shows it moving to libcrypto in the pink nirvana/packaging view diagrams)

mattcaswell · 2019-06-03T15:51:16Z

crypto/dsa/dsa_asn1.c

If one branch of the "if" needs {} then we use {} on both branches.

mattcaswell · 2019-06-03T15:51:59Z

crypto/dsa/dsa_asn1.c

Please write this as buf != NULL

Okay, done.

mattcaswell · 2019-06-03T16:06:17Z

crypto/asn1_dsa.c

I think we should test for such failures anyway. Relying on complex "this should never happen" rules and the internal implementation of BN_bn2binpad is brittle. Code changes over time and suddenly we could find that errors might be returned.

paulidale · 2019-06-03T22:52:47Z

I just want to make it explicit what we're actually doing.

My view is that this is preventing the entirety of the ASN.1 code being pulled inside the FIPS boundary.
It isn't intended as the start of a rewrite of ASN.1.
I'm fine with this code being used only in FIPS provider or with it being used by the default provider as well.

slontis · 2019-06-03T22:59:28Z

@mattcaswell David has written many asn1 parsers and should be aware of the edge cases. I am not sure we need a full blown packet system just to address the few instances that need to reside within the fips boundary. There should not really be a problem if the code is shared by the default provider - if it has to work for the fips provider it should also be able to work for the default case.

davidmakepeace · 2019-06-04T07:25:00Z

I have just pushed the updates.

@levitte commented:

You could possibly add an assertion that !BN_is_negative

Okay, sure. I have added an assertion.

@levitte commented:

As a general comment on the code itself, I assume we will not only encode/decode DSA, so you might as well make the primitive type encoders/decoders (enc_int and dec_int) non-static, and probably rename them to something a little more descriptive.

I renamed all the functions and made them all non-static.

I haven't yet moved the implementation into providers/common since I wasn't sure if it was possible to do that before moving the dsa/ecdsa code into the providers. Suggestions and guidance welcome.

levitte

Another thought I'm having is about the encoders. The more complex the ASN.1 structure, the more hellish and separate the size calculations will have too be. An idea I've seen somewhere else is to do the DER encoding backwards, which means that at any time an element is encoded, there's just one size to keep track of (and in the case of combined elements, such as a SEQUENCE, you naturally add those sizes together on the fly).

levitte · 2019-06-04T08:18:25Z

crypto/asn1_dsa.c

Might I suggest that **ppin only gets updated on successful decode, i.e. when this function returns 0, it should be left untouched?

This goes for all decoding functions, of course

Yeah, okay. Sure.

I have changed all the decoding functions so that *ppin is only advanced on successful decode.
After adding bounds checking to the encoding functions I also did the same thing for *ppout.

paulidale · 2019-06-04T08:31:14Z

Another thought I'm having is about the encoders. The more complex the ASN.1 structure, the more hellish and separate the size calculations will have too be. An idea I've seen somewhere else is to do the DER encoding backwards, which means that at any time an element is encoded, there's just one size to keep track of (and in the case of combined elements, such as a SEQUENCE, you naturally add those sizes together on the fly).

Is this a suggestion for a recursive descent encoder?
This would be my approach for decoding as well, BTW.

levitte · 2019-06-04T08:59:26Z

Is this a suggestion for a recursive descent encoder?

In a manner of speaking, yes... although I rather see it as a series of function calls, potentially calling other functions and ending up calling generic primitive encoders. So essentially, one would have a set of functions to build up the "template" for the DER encoding of the ASN.1 structure, given a corresponding C structure.

This would be my approach for decoding as well, BTW.

Sure.

davidmakepeace · 2019-06-04T09:19:11Z

Another thought I'm having is about the encoders. The more complex the ASN.1 structure, the more hellish and separate the size calculations will have too be. An idea I've seen somewhere else is to do the DER encoding backwards, which means that at any time an element is encoded, there's just one size to keep track of (and in the case of combined elements, such as a SEQUENCE, you naturally add those sizes together on the fly).

It's a tree structure where you could have sequences and/or sets containing multiple sequences and/or sets. At some point you do need to keep track of more than one size. You might as well encode forwards rather than backwards.

If you are encoding to memory then you probably want to do a pass to calculate the total encoded size so that you can allocate a buffer of that size. If you are streaming the encode then you also need to know the sizes in advance if you want DER (i.e. definite-length encoding) and you can't do the encoding backwards. :)

Also SET and SET OF must be sorted (with a different comparator depending on whether it is a SET or a SET OF) in order to conform to DER.

Recursive descent encoding and decoding is, of course, the way to go.

levitte · 2019-06-04T09:31:46Z

Streaming is a problem for data sets, that's true. We do have to handle that in some cases, most notable CMS. But for smaller data sets (I count keys, certificates and similar there), I'm unsure how much effort we must put into streaming, rather than collecting a whole data set in a buffer and decode it when complete.

(this depends on what one means with "small", of course)

mattcaswell · 2019-06-04T09:32:18Z

@mattcaswell David has written many asn1 parsers and should be aware of the edge cases. I am not sure we need a full blown packet system just to address the few instances that need to reside within the fips boundary.

Sorry - I don't agree. I'm not doubting David's ability to write parsers, but I have fixed many bugs in this kind of code over time and the old packet parsing/encoding system was a major cause of those bugs. I do not want to make the same mistake again. It's not just the initial implementation that you have worry about. Code changes over time and this type of code is brittle. It doesn't take much to suddenly find that some edge case is causing an OOB read or write - and in a potentially exploitable way.

We already have a packet parsing and encoding system. It's not currently available to libcrypto (libssl only) - but the design document does show it as moving to libcrypto. We should be using it.

richsalz · 2019-06-04T12:53:53Z

Don't turn this into a "solve all the problems" thing. This is a small piece of code that does what's needed for the FIPS module [yes it will need more for RSA etc], and that should be enough for this release.

slontis · 2019-06-06T02:50:24Z

crypto/asn1_dsa.c

Maybe add a comment * The maximum content length supported is 0xFFFF.
The maximum returned encoding length is 3 bytes.

slontis · 2019-06-06T02:59:59Z

crypto/asn1_dsa.c

NIT: number of

slontis · 2019-06-06T03:10:54Z

crypto/asn1_dsa.c

what happens in len was passed in as 2 here? i.e- when produced is > 2 so len- produced goes negative?

In every encode function, and at each step, produced is never greater than len so len - produced can never be negative.
At the line you commented on, if len was passed in as 2 and produced is 1 (from the line above, produced = 1;) then len - produced is 2 - 1, which is 1 (which is positive).

yep ok.. so that is why the initial len < 1 check is needed. I would normally decrement the total length to do this. But your way also works.

Yeah, I could have been more verbose: initialized produced to 0, checked if 1 > len - produced before emitting ID_SEQUENCE, then incremented produced, etc., but I thought that was unnecessary.
What do you think? Would that make it clearer?

yep ok.. so that is why the initial len < 1 check is needed. I would normally decrement the total length to do this. But your way also works.

Sure, reducing the length at each step would have worked. The return value (bytes produced) could have been calculated with something like orig_len - len or by pointer difference and saving the original out pointer.

slontis · 2019-06-06T03:15:08Z

crypto/asn1_dsa.c

same here - not sure size_t comparisons with len - consumed possibly being negative is a good idea.

As for the encoders, in every decode function, and at each step, consumed is never greater than len so len - consumed can never be negative.
The value returned by a decode function is either 0 (for an error) or is less than or equal to it's len parameter. If the caller does consumed += c; then it is still true that consumed is less than or equal to len.

mattcaswell · 2019-06-06T08:40:00Z

PACKET/WPACKET looks like it would be useful if I was implementing an entire ASN.1 module, but this PR is intended to be small, simple and robust and initially only satisfy the requirements of ECDSA/DSA signatures.

PACKET/WPACKET is useful for any packet parsing. It it not a particularly large amount of code: in the PACKET case it is implemented entirely as inline functions so you only pay the cost for the code you actually use. In the WPACKET case it is a single file with less than 450 lines of code.

Using it is straight forward and should not significantly increase the complexity of your current PR.

I'm strongly of the view that you should be taking this approach.

paulidale · 2019-06-06T09:32:48Z

This isn't so much about code size, using PACKET/WPACKET inside the FIPS provider means freezing the code. The ASN.1 encoding here is two integers and every FIPS implementation I've heard of has hardcoded this...

mattcaswell · 2019-06-06T09:52:40Z

This isn't so much about code size, using PACKET/WPACKET inside the FIPS provider means freezing the code.

So are you suggesting we should do things the wrong way because of FIPS rules? Surely we should design things the right way so as to avoid introducing bugs and vulnerabilities.

My understanding is that we are not going to "freeze" any code. We just fix bugs in it as required. If that means we have to do a 3SUB then so be it. Obviously we should avoid bringing in large chunks of code unnecessarily, but that shouldn't stop us bringing in the code that we need to do the job properly.

The ASN.1 encoding here is two integers and every FIPS implementation I've heard of has hardcoded this...

And it is still hardcoded with PACKET/WPACKET...we're not bringing in all the ASN.1 code.

t-j-h · 2019-06-06T09:59:46Z

I would like to see an alternate PR using the [W]PACKET interface to compare if anyone is willing to produce it. I don't see a issue one way or the other on whether or not we included the [W]PACKET code in the FIPS module. If we hit bugs we evaluate and fix. And bugs in this area would be non-algorithm bugs and able to go through a quick lab review for non-impact.

richsalz · 2019-06-06T19:14:14Z

I shared the concerns about using PACKET/WPACKET and I just spent a bit of time trying to do this. It's an awkward fit, although some of that might be that I am not expert with the packet stuff. For example:

No ability to compute lengths without outputting the data and then throwing it away
No support for variable-length lengths (required by DER and BER)
Much support for sub-packets and passing up lengths, but that doesn't work for variable-length lengths (not surprising since all TLS presentation lengths are fixed-size). This means that it becomes an even WORSE fit if we need to support nested ASN.1 structures in the FIPS module.

So I changed my mind, threw away my half-written code, and support this PR as-is.

mattcaswell · 2019-06-06T21:20:44Z

Just to ensure this isn't merged until this is resolved:

-1

slontis · 2019-06-06T21:42:48Z

I also had a look at the interface.
You will gain very little by over-engineering this.

davidmakepeace · 2019-06-06T22:02:03Z

I just pushed some updates to code comments only.

levitte · 2019-06-07T15:35:22Z

It would be foolish to think that this will be done for DSA sigs only, so encoding/decoding of primitives (integers in this PR) should really be factored out to its own translation unit.

mattcaswell · 2019-06-07T16:45:40Z

Please take a look at #9111 for what this would look like using the PACKET API. I've not yet done the WPACKET changes.

richsalz · 2019-06-07T17:00:23Z

I prefer to wait for refactoring until you see what other use-cases and code are needed.

levitte · 2019-06-07T21:17:29Z

I prefer to wait for refactoring until you see what other use-cases and code are needed.

That's how you end up having a generic URL parser in the OCSP code and then forget it's there. Some generalism isn't always a bad idea...

Adds simple utility functions to allow both the default and fips providers to encode and decode DSA-Sig-Value and ECDSA-Sig-Value (DSA_SIG and ECDSA_SIG structures) to/from ASN.1 DER without requiring those providers to have a dependency on the asn1 module.

davidmakepeace · 2019-07-02T01:20:39Z

Rebased to resolve a conflict with master.

slontis · 2019-07-02T02:50:02Z

I prefer to wait for refactoring until you see what other use-cases and code are needed.

This case was the only ASN1 encode/decode required inside the old FOM - apart from RSA which is handled as fixed tables - so given that we already know what algorithms are inside the boundary, I dont see there being any more cases needed inside the FIPS provider. I also then dont see why it needs to use WPACKET/PACKET as it is fairly simple.
I cant see this changing any time soon. If it does then maybe refactor it then to use PACKET/WPACKET, otherwise I think its just overkill.

richsalz · 2019-07-02T11:43:45Z

I withdraw my "prefer to wait" comment; #9067 (comment) convinced me there's no new information likely to come in.

mattcaswell · 2019-07-02T13:31:41Z

To ensure it doesn't get lost in the intervening comments, my -1 on this still stands.

mattcaswell · 2019-07-02T13:42:45Z

This case was the only ASN1 encode/decode required inside the old FOM - apart from RSA which is handled as fixed tables - so given that we already know what algorithms are inside the boundary, I dont see there being any more cases needed inside the FIPS provider. I also then dont see why it needs to use WPACKET/PACKET as it is fairly simple.

I don't see that using WPACKET/PACKET adds any complexity. If anything it makes it simpler and easier to read.

Regardless of that though, IMO all new packet processing code should be using WPACKET/PACKET no matter what. No exceptions. It makes no difference whether it is apparently simple. We were constantly being bombarded with OOB read/write reports in the past in libssl in code that was very similar to this. Code that should also have been simple. It turns out it's just too easy to make a mistake. I don't ever want to go there again.

richsalz · 2019-07-02T14:11:53Z

I understand the argument for using the PACKET API's (and, to me, it's convincing; raw buffer manipulation seems a step backwards, no matter how "simple" it is). But I think it's wrong for an OMC member to be able to "perpetually" block a PR. They should either expire after some number of weeks (four?) or the blocker should start a vote.

richsalz · 2019-07-02T14:13:13Z

All other OMC policies (inactivity, voting periods) have a time-based cut-off; holds on PR's should also.

paulidale · 2019-07-15T22:33:30Z

Closed via #9111

davidmakepeace changed the title ~~Added simple ASN.1 utils for encoding/decoding DSA_SIG/ECDSA_SIG~~ Add simple ASN.1 utils for encoding/decoding DSA_SIG/ECDSA_SIG Jun 3, 2019

davidmakepeace changed the title ~~Add simple ASN.1 utils for encoding/decoding DSA_SIG/ECDSA_SIG~~ Add simple ASN.1 utils for DSA signature DER Jun 3, 2019

davidmakepeace force-pushed the add-asn1-dsa-fips branch from 0b172e5 to ab6d077 Compare June 3, 2019 05:54

slontis reviewed Jun 3, 2019

View reviewed changes

paulidale added the branch: master Applies to master branch label Jun 3, 2019

levitte reviewed Jun 3, 2019

View reviewed changes

crypto/asn1_dsa.c Outdated Show resolved Hide resolved

mattcaswell requested changes Jun 3, 2019

View reviewed changes

davidmakepeace force-pushed the add-asn1-dsa-fips branch from ab6d077 to aa973ec Compare June 4, 2019 07:11

davidmakepeace force-pushed the add-asn1-dsa-fips branch from aa973ec to 279be13 Compare June 4, 2019 07:29

levitte requested changes Jun 4, 2019

View reviewed changes

davidmakepeace force-pushed the add-asn1-dsa-fips branch from 279be13 to 2be2f07 Compare June 4, 2019 23:17

slontis reviewed Jun 6, 2019

View reviewed changes

davidmakepeace force-pushed the add-asn1-dsa-fips branch from 7ab45a6 to 5df8e86 Compare June 6, 2019 21:59

mattcaswell mentioned this pull request Jun 7, 2019

Simple ASN1 utils for DSA based on PACKET #9111

Closed

davidmakepeace mentioned this pull request Jun 12, 2019

Remove asn1 module dependency from RSASSA-PKCS1-v1_5. #9138

Closed

2 tasks

davidmakepeace force-pushed the add-asn1-dsa-fips branch from 5df8e86 to 417dece Compare July 2, 2019 01:19

paulidale closed this Jul 15, 2019

Uh oh!

Conversation

davidmakepeace commented Jun 3, 2019

Checklist

Uh oh!

levitte commented Jun 3, 2019

Uh oh!

t-j-h commented Jun 3, 2019

Uh oh!

levitte commented Jun 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmakepeace Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slontis Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmakepeace Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slontis commented Jun 3, 2019

Uh oh!

levitte commented Jun 3, 2019

Uh oh!

t-j-h commented Jun 3, 2019

Uh oh!

levitte commented Jun 3, 2019

Uh oh!

levitte commented Jun 3, 2019

Uh oh!

Uh oh!

davidmakepeace commented Jun 3, 2019

Uh oh!

levitte commented Jun 3, 2019

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulidale commented Jun 3, 2019

davidmakepeace Jun 3, 2019 •

edited

Loading

slontis Jun 3, 2019 •

edited

Loading

davidmakepeace Jun 4, 2019 •

edited

Loading

davidmakepeace commented Jun 4, 2019 •

edited by levitte

Loading

slontis Jun 6, 2019 •

edited

Loading