Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from
Closed
Changes from 1 commit
Commits
File filter
Filter file types
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -222,7 +222,7 @@ public boolean isConstructed(byte constructedTag) {
*/
DerValue(byte tag, byte[] buffer, int start, int end, boolean allowBER) {
if ((tag & 0x1f) == 0x1f) {
throw new IllegalArgumentException("Tag number cannot be 31");
throw new IllegalArgumentException("Tag number 31 is not supported");

This comment has been minimized.

@XueleiFan

XueleiFan Apr 8, 2021
Member

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

This comment has been minimized.

@wangweij

wangweij Apr 8, 2021
Author Contributor

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.

This comment has been minimized.

@XueleiFan

XueleiFan Apr 8, 2021
Member

It makes sense. Your words is good to me.

}
this.tag = tag;
this.buffer = buffer;
@@ -319,7 +319,7 @@ public DerValue(byte[] encoding) throws IOException {
int pos = offset;
tag = buf[pos++];
if ((tag & 0x1f) == 0x1f) {
throw new IOException("Tag number cannot exceed 30");
throw new IOException("Tag number over 30 is not supported");

This comment has been minimized.

@jnimeh

jnimeh Apr 8, 2021
Member

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.

This comment has been minimized.

@wangweij

wangweij Apr 8, 2021
Author Contributor

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.

This comment has been minimized.

@jnimeh

jnimeh Apr 8, 2021
Member

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.

This comment has been minimized.

@wangweij

wangweij Apr 8, 2021
Author Contributor

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.

}
int lenByte = buf[pos++];

@@ -395,7 +395,7 @@ public DerValue(byte[] encoding) throws IOException {
DerValue(InputStream in, boolean allowBER) throws IOException {
this.tag = (byte)in.read();
if ((tag & 0x1f) == 0x1f) {
throw new IOException("Tag number cannot exceed 30");
throw new IOException("Tag number over 30 is not supported");
}
int length = DerInputStream.getLength(in);
if (length == -1) { // indefinite length encoding found
@@ -1150,7 +1150,7 @@ public static boolean isPrintableStringChar(char ch) {
*/
public static byte createTag(byte tagClass, boolean form, byte val) {
if (val < 0 || val > 30) {
throw new IllegalArgumentException("Tag number cannot exceed 30");
throw new IllegalArgumentException("Tag number over 30 is not supported");
}
byte tag = (byte)(tagClass | val);
if (form) {
ProTip! Use n and p to navigate between commits in a pull request.