Problem with BouncyCastle ASN.1 design

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Problem with BouncyCastle ASN.1 design

Uri Blumenthal
Apparently, BC got the ASN.1 inheritance wrong:

...in BC, the datatype "BERApplicationSpecific" inherits from "DERApplicationSpecific". IMO that breaks a fundamental principle of ASN.1 that @velichkov mentioned above, that every DER is a valid BER, but not every BER is a valid DER.

Where does this hurt: currently the code that wants to accept/parse BER, rejects (valid) DER (which, of course, is very wrong). This issue surfaced here: https://github.com/vlm/asn1c/issues/262 

I understand that most of the ASN.1-encoded stuff BC is dealing with is DER, but still IMHO this should be corrected. 

Comments?
Reply | Threaded
Open this post in threaded view
|

Re: Problem with BouncyCastle ASN.1 design

David Hook-3

Actually, this isn't true. It was, for historical reasons, as the DER version of the class was added first and extending it was the only way to avoid breaking backwards compatibility. Post July 2015 both classes extend ASN1ApplicationSpecific. I've never seen the library reject DER data in a BER stream, although I can imagine how problems with casting could cause an application to fail.

That said, I'd say that claiming this breaks a fundamental principle of ASN.1 is a little bit strong - there are a number of ways you could approach this problem in an "OO sense", as you can see from our current model. Linking encoding to object type is a bit fraught in any case - you cannot really tell when you read something if it's DER, the only certainty is that it's definite-length encoded, after that everything becomes a guess. If there's any fundamental principle with ASN.1 encoding in general, it's probably "anything goes"....

Regards,

David

On 13/03/18 05:49, Uri Blumenthal wrote:
Apparently, BC got the ASN.1 inheritance wrong:

...in BC, the datatype "BERApplicationSpecific" inherits from "DERApplicationSpecific". IMO that breaks a fundamental principle of ASN.1 that @velichkov mentioned above, that every DER is a valid BER, but not every BER is a valid DER.

Where does this hurt: currently the code that wants to accept/parse BER, rejects (valid) DER (which, of course, is very wrong). This issue surfaced here: https://github.com/vlm/asn1c/issues/262 

I understand that most of the ASN.1-encoded stuff BC is dealing with is DER, but still IMHO this should be corrected. 

Comments?


Reply | Threaded
Open this post in threaded view
|

Re: Problem with BouncyCastle ASN.1 design

Uri Blumenthal
On Mar 12, 2018, at 18:24 , David Hook <[hidden email]> wrote:

Actually, this [that BER extends DER instead of the opposite] isn't true. It was, for historical reasons, as the DER version of the class was added first and extending it was the only way to avoid breaking backwards compatibility.

Well, since I know little about the history of BC development, and somewhat disagree about how to provide backwards compatibility - I won’t argue here.

Post July 2015 both classes extend ASN1ApplicationSpecific.

I still think that DER should extend BER (rather than both being “peer” descendants of a 3rd class), but please see below.

I've never seen the library reject DER data in a BER stream

This is exactly the point! This whole email chain was brought into existence by the problem that a user had: BC ASN.1 parser failed to decode definite-length encoded SEQUENCE but worked correctly when it was indefinite-length encoded. The pointers to the issue are below.

...although I can imagine how problems with casting could cause an application to fail.

It appears that the server in question did/does cast to BER, as I deduce from https://github.com/vlm/asn1c/issues/262#issuecomment-371235850. Regardless, there shouldn’t be a possibility when casting to BER makes DER encoding fail to parse correctly. 

Please do review this issue (https://github.com/vlm/asn1c/issues/262) and see if you still think that casting to BER should make a DER encoding not parseable. 

That said, I'd say that claiming this breaks a fundamental principle of ASN.1 is a little bit strong

Well, that was the perception of the person who was hit by this bug - and if indeed you cannot parse definite-length encoded SEQUENCE as BER, I do call it a bug. Because indeed, any valid DER is at the same time a valid BER, and therefore should be parsed OK.

- there are a number of ways you could approach this problem in an "OO sense", as you can see from our current model.

That’s true.

Linking encoding to object type is a bit fraught in any case - you cannot really tell when you read something if it's DER, the only certainty is that it's definite-length encoded, after that everything becomes a guess.


If there's any fundamental principle with ASN.1 encoding in general, it's probably "anything goes"….

Now I’d call that “a little bit strong”. I’ve worked with ASN.1 since 1992, and in general find it usable - especially nowadays when great tools to work with it became available and affordable (for example, asn1c is free ;).

Still, I’d appreciate if you could review the problem described in that issue and posted (or emailed me directly) your opinion on that specific problem.

Thanks!

On 13/03/18 05:49, Uri Blumenthal wrote:
Apparently, BC got the ASN.1 inheritance wrong:

...in BC, the datatype "BERApplicationSpecific" inherits from "DERApplicationSpecific". IMO that breaks a fundamental principle of ASN.1 that @velichkov mentioned above, that every DER is a valid BER, but not every BER is a valid DER.

Where does this hurt: currently the code that wants to accept/parse BER, rejects (valid) DER (which, of course, is very wrong). This issue surfaced here: https://github.com/vlm/asn1c/issues/262 

I understand that most of the ASN.1-encoded stuff BC is dealing with is DER, but still IMHO this should be corrected. 

Comments?




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

Re: Problem with BouncyCastle ASN.1 design

David Hook-3

The code should have been written using ASN1Sequence.getInstance(). ASN.1 streams, as objects, can contain both primitive and user defined types - casting in that situation will only cause error, errors which will often hide during development and show up as unexpected side effects resulting from apparently trivial changes. Don't cast, use the getInstance() methods, that's what they are for.

Regards,

David

On 14/03/18 02:23, Uri Blumenthal wrote:
On Mar 12, 2018, at 18:24 , David Hook <[hidden email]> wrote:

Actually, this [that BER extends DER instead of the opposite] isn't true. It was, for historical reasons, as the DER version of the class was added first and extending it was the only way to avoid breaking backwards compatibility.

Well, since I know little about the history of BC development, and somewhat disagree about how to provide backwards compatibility - I won’t argue here.

Post July 2015 both classes extend ASN1ApplicationSpecific.

I still think that DER should extend BER (rather than both being “peer” descendants of a 3rd class), but please see below.

I've never seen the library reject DER data in a BER stream

This is exactly the point! This whole email chain was brought into existence by the problem that a user had: BC ASN.1 parser failed to decode definite-length encoded SEQUENCE but worked correctly when it was indefinite-length encoded. The pointers to the issue are below.

...although I can imagine how problems with casting could cause an application to fail.

It appears that the server in question did/does cast to BER, as I deduce from https://github.com/vlm/asn1c/issues/262#issuecomment-371235850. Regardless, there shouldn’t be a possibility when casting to BER makes DER encoding fail to parse correctly. 

Please do review this issue (https://github.com/vlm/asn1c/issues/262) and see if you still think that casting to BER should make a DER encoding not parseable. 

That said, I'd say that claiming this breaks a fundamental principle of ASN.1 is a little bit strong

Well, that was the perception of the person who was hit by this bug - and if indeed you cannot parse definite-length encoded SEQUENCE as BER, I do call it a bug. Because indeed, any valid DER is at the same time a valid BER, and therefore should be parsed OK.

- there are a number of ways you could approach this problem in an "OO sense", as you can see from our current model.

That’s true.

Linking encoding to object type is a bit fraught in any case - you cannot really tell when you read something if it's DER, the only certainty is that it's definite-length encoded, after that everything becomes a guess.


If there's any fundamental principle with ASN.1 encoding in general, it's probably "anything goes"….

Now I’d call that “a little bit strong”. I’ve worked with ASN.1 since 1992, and in general find it usable - especially nowadays when great tools to work with it became available and affordable (for example, asn1c is free ;).

Still, I’d appreciate if you could review the problem described in that issue and posted (or emailed me directly) your opinion on that specific problem.

Thanks!

On 13/03/18 05:49, Uri Blumenthal wrote:
Apparently, BC got the ASN.1 inheritance wrong:

...in BC, the datatype "BERApplicationSpecific" inherits from "DERApplicationSpecific". IMO that breaks a fundamental principle of ASN.1 that @velichkov mentioned above, that every DER is a valid BER, but not every BER is a valid DER.

Where does this hurt: currently the code that wants to accept/parse BER, rejects (valid) DER (which, of course, is very wrong). This issue surfaced here: https://github.com/vlm/asn1c/issues/262 

I understand that most of the ASN.1-encoded stuff BC is dealing with is DER, but still IMHO this should be corrected. 

Comments?




Reply | Threaded
Open this post in threaded view
|

Re: Problem with BouncyCastle ASN.1 design

Uri Blumenthal
Thank you, I’ll pass your guidance along (and will remember it for when I need to use BC ASN.1 myself).

On Mar 13, 2018, at 13:04 , David Hook <[hidden email]> wrote:


The code should have been written using ASN1Sequence.getInstance(). ASN.1 streams, as objects, can contain both primitive and user defined types - casting in that situation will only cause error, errors which will often hide during development and show up as unexpected side effects resulting from apparently trivial changes. Don't cast, use the getInstance() methods, that's what they are for.

Regards,

David

On 14/03/18 02:23, Uri Blumenthal wrote:
On Mar 12, 2018, at 18:24 , David Hook <[hidden email]> wrote:

Actually, this [that BER extends DER instead of the opposite] isn't true. It was, for historical reasons, as the DER version of the class was added first and extending it was the only way to avoid breaking backwards compatibility.

Well, since I know little about the history of BC development, and somewhat disagree about how to provide backwards compatibility - I won’t argue here.

Post July 2015 both classes extend ASN1ApplicationSpecific.

I still think that DER should extend BER (rather than both being “peer” descendants of a 3rd class), but please see below.

I've never seen the library reject DER data in a BER stream

This is exactly the point! This whole email chain was brought into existence by the problem that a user had: BC ASN.1 parser failed to decode definite-length encoded SEQUENCE but worked correctly when it was indefinite-length encoded. The pointers to the issue are below.

...although I can imagine how problems with casting could cause an application to fail.

It appears that the server in question did/does cast to BER, as I deduce from https://github.com/vlm/asn1c/issues/262#issuecomment-371235850. Regardless, there shouldn’t be a possibility when casting to BER makes DER encoding fail to parse correctly. 

Please do review this issue (https://github.com/vlm/asn1c/issues/262) and see if you still think that casting to BER should make a DER encoding not parseable. 

That said, I'd say that claiming this breaks a fundamental principle of ASN.1 is a little bit strong

Well, that was the perception of the person who was hit by this bug - and if indeed you cannot parse definite-length encoded SEQUENCE as BER, I do call it a bug. Because indeed, any valid DER is at the same time a valid BER, and therefore should be parsed OK.

- there are a number of ways you could approach this problem in an "OO sense", as you can see from our current model.

That’s true.

Linking encoding to object type is a bit fraught in any case - you cannot really tell when you read something if it's DER, the only certainty is that it's definite-length encoded, after that everything becomes a guess.


If there's any fundamental principle with ASN.1 encoding in general, it's probably "anything goes"….

Now I’d call that “a little bit strong”. I’ve worked with ASN.1 since 1992, and in general find it usable - especially nowadays when great tools to work with it became available and affordable (for example, asn1c is free ;).

Still, I’d appreciate if you could review the problem described in that issue and posted (or emailed me directly) your opinion on that specific problem.

Thanks!

On 13/03/18 05:49, Uri Blumenthal wrote:
Apparently, BC got the ASN.1 inheritance wrong:

...in BC, the datatype "BERApplicationSpecific" inherits from "DERApplicationSpecific". IMO that breaks a fundamental principle of ASN.1 that @velichkov mentioned above, that every DER is a valid BER, but not every BER is a valid DER.

Where does this hurt: currently the code that wants to accept/parse BER, rejects (valid) DER (which, of course, is very wrong). This issue surfaced here: https://github.com/vlm/asn1c/issues/262 

I understand that most of the ASN.1-encoded stuff BC is dealing with is DER, but still IMHO this should be corrected. 

Comments?






smime.p7s (2K) Download Attachment