RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding

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

RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding

Weijun Wang-2
This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31.

-------------

Commit messages:
 - 8264864: Multiple byte tag not supported by ASN.1 encoding

Changes: https://git.openjdk.java.net/jdk/pull/3391/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3391&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264864
  Stats: 78 lines in 2 files changed: 77 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3391.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v2]

Weijun Wang-2
> This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31.

Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:

  make sure test fails before code change

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3391/files
  - new: https://git.openjdk.java.net/jdk/pull/3391/files/d25d3fb2..46b3700b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3391&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3391&range=00-01

  Stats: 11 lines in 1 file changed: 6 ins; 1 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3391.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v2]

Xue-Lei Andrew Fan
On Thu, 8 Apr 2021 01:33:56 GMT, Weijun Wang <[hidden email]> wrote:

>> This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>
>   make sure test fails before code change

src/java.base/share/classes/sun/security/util/DerValue.java line 322:

> 320:         tag = buf[pos++];
> 321:         if ((tag & 0x1f) == 0x1f) {
> 322:             throw new IOException("Tag number cannot exceed 30");

It may be safe if not support multiple bytes tag in the current implementation of JDK, especially the ASN.1 implementation is private.  However, multiple bytes tag is a legal form of ASN.1 encoding, I think.  It would be nice to have a comment to state that this form is not support yet, and we may consider it in the future if needed.  It may be helpful for future code maintenance.

The exception message, "Tag number cannot exceed 30", may be not accuracy.  I think tag number can exceed 30 per the specification, but JDK does not support it yet because we did not run into such tags in practice.  I may use some words like: "Tag number exceed 30 is not supported".

-------------

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]

Weijun Wang-2
In reply to this post by Weijun Wang-2
> This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31.

Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:

  update exception wordings

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3391/files
  - new: https://git.openjdk.java.net/jdk/pull/3391/files/46b3700b..9b0f9db9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3391&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3391&range=01-02

  Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3391.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v2]

Weijun Wang-2
In reply to this post by Xue-Lei Andrew Fan
On Thu, 8 Apr 2021 03:46:07 GMT, Xue-Lei Andrew Fan <[hidden email]> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   make sure test fails before code change
>
> src/java.base/share/classes/sun/security/util/DerValue.java line 322:
>
>> 320:         tag = buf[pos++];
>> 321:         if ((tag & 0x1f) == 0x1f) {
>> 322:             throw new IOException("Tag number cannot exceed 30");
>
> It may be safe if not support multiple bytes tag in the current implementation of JDK, especially the ASN.1 implementation is private.  However, multiple bytes tag is a legal form of ASN.1 encoding, I think.  It would be nice to have a comment to state that this form is not support yet, and we may consider it in the future if needed.  It may be helpful for future code maintenance.
>
> The exception message, "Tag number cannot exceed 30", may be not accuracy.  I think tag number can exceed 30 per the specification, but JDK does not support it yet because we did not run into such tags in practice.  I may use some words like: "Tag number exceed 30 is not supported".

Messages updated. "exceed" is a verb and I'm not sure whether to choose "exceeding" or "that exceeds" so finally use "over". Thanks!

-------------

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]

Xue-Lei Andrew Fan
In reply to this post by Weijun Wang-2
On Thu, 8 Apr 2021 13:57:37 GMT, Weijun Wang <[hidden email]> wrote:

>> This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>
>   update exception wordings

Looks good to me, except a minor comment.

src/java.base/share/classes/sun/security/util/DerValue.java line 225:

> 223:     DerValue(byte tag, byte[] buffer, int start, int end, boolean allowBER) {
> 224:         if ((tag & 0x1f) == 0x1f) {
> 225:             throw new IllegalArgumentException("Tag number 31 is not supported");

As number 31 just means the tag is bigger than 31, Is it more accuracy by using "Tag number over 30 is not supported"?

-------------

Marked as reviewed by xuelei (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v4]

Weijun Wang-2
In reply to this post by Weijun Wang-2
> This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31.

Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:

  change exception message

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3391/files
  - new: https://git.openjdk.java.net/jdk/pull/3391/files/9b0f9db9..004b35e8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3391&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3391&range=02-03

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3391.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]

Weijun Wang-2
In reply to this post by Xue-Lei Andrew Fan
On Thu, 8 Apr 2021 15:53:10 GMT, Xue-Lei Andrew Fan <[hidden email]> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   update exception wordings
>
> src/java.base/share/classes/sun/security/util/DerValue.java line 225:
>
>> 223:     DerValue(byte tag, byte[] buffer, int start, int end, boolean allowBER) {
>> 224:         if ((tag & 0x1f) == 0x1f) {
>> 225:             throw new IllegalArgumentException("Tag number 31 is not supported");
>
> As number 31 just means the tag is bigger than 31, Is it more accuracy by using "Tag number over 30 is not supported"?

Well, it's a little delicate here. Even if we support multi-byte tag one day, this constructor will still only be used to create a single-byte tag `DerValue`, and it's illegal for a single byte tag to end with 0x1f. So the words above is to remind people that they cannot create a tag number 31 `DerValue` just because it seems it still fits into the 5 bits. Precisely, the words should be "this constructor only supports tag number between 0 and 30", but... I'll choose your words.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]

Jamil Nimeh-2
In reply to this post by Weijun Wang-2
On Thu, 8 Apr 2021 13:57:37 GMT, Weijun Wang <[hidden email]> wrote:

>> This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>
>   update exception wordings

src/java.base/share/classes/sun/security/util/DerValue.java line 322:

> 320:         tag = buf[pos++];
> 321:         if ((tag & 0x1f) == 0x1f) {
> 322:             throw new IOException("Tag number over 30 is not supported");

Would it be useful for these types of exception messages to either display the offending tag value or perhaps the tag offset?  Just thinking it might be a nice thing for the recipient to know where in the DER encoding the issue is.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]

Weijun Wang-2
On Thu, 8 Apr 2021 16:58:24 GMT, Jamil Nimeh <[hidden email]> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   update exception wordings
>
> src/java.base/share/classes/sun/security/util/DerValue.java line 322:
>
>> 320:         tag = buf[pos++];
>> 321:         if ((tag & 0x1f) == 0x1f) {
>> 322:             throw new IOException("Tag number over 30 is not supported");
>
> Would it be useful for these types of exception messages to either display the offending tag value or perhaps the tag offset?  Just thinking it might be a nice thing for the recipient to know where in the DER encoding the issue is.

I don't want to go on reading the following bytes to find out what the intended tag number is, because that somehow shows I do understand the encoding _a lot_ but still don't want to support it (well, actually I only understand _a little_). There are only 2 kinds of tags: one <= 30 and one >= 31. IMHO, the message has already expressed the meaning that we only support the 1st one.

An alternative message I can think of is "Unsupported tag byte: 0xBF", but it looks too cryptic.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]

Jamil Nimeh-2
On Thu, 8 Apr 2021 17:10:13 GMT, Weijun Wang <[hidden email]> wrote:

>> src/java.base/share/classes/sun/security/util/DerValue.java line 322:
>>
>>> 320:         tag = buf[pos++];
>>> 321:         if ((tag & 0x1f) == 0x1f) {
>>> 322:             throw new IOException("Tag number over 30 is not supported");
>>
>> Would it be useful for these types of exception messages to either display the offending tag value or perhaps the tag offset?  Just thinking it might be a nice thing for the recipient to know where in the DER encoding the issue is.
>
> I don't want to go on reading the following bytes to find out what the intended tag number is, because that somehow shows I do understand the encoding _a lot_ but still don't want to support it (well, actually I only understand _a little_). There are only 2 kinds of tags: one <= 30 and one >= 31. IMHO, the message has already expressed the meaning that we only support the 1st one.
>
> An alternative message I can think of is "Unsupported tag byte: 0xBF", but it looks too cryptic.

I think that is fair.  If you don't want to read ahead like that, what about using the "offset" or "pos" field to give a message like "Tag number over 30 at offset NN is not supported" (something like that, at least)  Maybe don't worry about the tag value itself, but at least the position in the data stream.  Just a suggestion only, no strong feelings about this either way.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]

Xue-Lei Andrew Fan
In reply to this post by Weijun Wang-2
On Thu, 8 Apr 2021 16:59:54 GMT, Weijun Wang <[hidden email]> wrote:

>> src/java.base/share/classes/sun/security/util/DerValue.java line 225:
>>
>>> 223:     DerValue(byte tag, byte[] buffer, int start, int end, boolean allowBER) {
>>> 224:         if ((tag & 0x1f) == 0x1f) {
>>> 225:             throw new IllegalArgumentException("Tag number 31 is not supported");
>>
>> As number 31 just means the tag is bigger than 31, Is it more accuracy by using "Tag number over 30 is not supported"?
>
> Well, it's a little delicate here. Even if we support multi-byte tag one day, this constructor will still only be used to create a single-byte tag `DerValue`, and it's illegal for a single byte tag to end with 0x1f. So the words above is to remind people that they cannot create a tag number 31 `DerValue` just because it seems it still fits into the 5 bits. Precisely, the words should be "this constructor only supports tag number between 0 and 30", but... I'll choose your words.

It makes sense.  Your words is good to me.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]

Weijun Wang-2
In reply to this post by Jamil Nimeh-2
On Thu, 8 Apr 2021 17:18:50 GMT, Jamil Nimeh <[hidden email]> wrote:

>> I don't want to go on reading the following bytes to find out what the intended tag number is, because that somehow shows I do understand the encoding _a lot_ but still don't want to support it (well, actually I only understand _a little_). There are only 2 kinds of tags: one <= 30 and one >= 31. IMHO, the message has already expressed the meaning that we only support the 1st one.
>>
>> An alternative message I can think of is "Unsupported tag byte: 0xBF", but it looks too cryptic.
>
> I think that is fair.  If you don't want to read ahead like that, what about using the "offset" or "pos" field to give a message like "Tag number over 30 at offset NN is not supported" (something like that, at least)  Maybe don't worry about the tag value itself, but at least the position in the data stream.  Just a suggestion only, no strong feelings about this either way.

There _is_ an `offset` value here but I have really no idea if the user knows where to count from. If we say "offset" then we probably need to tell what data block we are talking about. What if the DerValue is just a portion of a bigger data block?

That said, if you really like it, I can add an offset like "tag byte at offset nnnn". I just hope the user can find it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v5]

Weijun Wang-2
In reply to this post by Weijun Wang-2
> This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31.

Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:

  add offset info in exception message

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3391/files
  - new: https://git.openjdk.java.net/jdk/pull/3391/files/004b35e8..9cb908bf

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3391&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3391&range=03-04

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3391.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391

PR: https://git.openjdk.java.net/jdk/pull/3391
Reply | Threaded
Open this post in threaded view
|

Integrated: 8264864: Multiple byte tag not supported by ASN.1 encoding

Weijun Wang-2
In reply to this post by Weijun Wang-2
On Thu, 8 Apr 2021 01:06:47 GMT, Weijun Wang <[hidden email]> wrote:

> This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31.

This pull request has now been integrated.

Changeset: 3d2b4cc5
Author:    Weijun Wang <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/3d2b4cc5
Stats:     83 lines in 2 files changed: 82 ins; 0 del; 1 mod

8264864: Multiple byte tag not supported by ASN.1 encoding

Reviewed-by: xuelei

-------------

PR: https://git.openjdk.java.net/jdk/pull/3391