Inconsistent handling of multibyte tags in TLVReader

[dmercer-google]

The TLV reader class seems to handle multibyte TLVs correctly when seeking the length field but not when getting a tag.

static short getLength(byte[] data, short offset) handles multibyte tags correctly when skipping over them to het the length of the TLV.

The problem is that there are only getTag and getTagShort methods. There should also be a getTagByteArray which returns either the full multibyte array or only the actual value with high byte stripped and continuation bits cleared.

The getTag and getTagShort methods just assume that you know what you are doing and know the length of the tag and will happily truncate or read beyond the actual tag if requested. IMO, its never a good idea to assume that engineers know what they are doing.

In addition: getTag and getTagShort should throw if the tag is not of the appropriate type length. For example:

If the tag is 5FC107. I would expect:

• getTag to throw,
• getTagShort to return 0x4107. Note that the high continuation bit was stripped giving the raw short. Up to you if you choose to strip it or not as long as the behaviour is documented.
• and getTagByteArray to return new byte[] {0x41, 0x07}. Again note that continuation but is stripped.

Lastly: the various getTagShort and getTagByteArray methods should document whether the value returned is with or without the multibyte tag indicator byte and if the continuation bit is set ot not.

[makinako]

This is correct, however there is a rationale for it (good or not). I've deliberately made the TLVReader class as bare-bones as possible and this includes parsing of the tag format.
When traversing through tags, it is important that the format is observed in order to know where the start of the length bytes and data bytes are. This is why getLength() and getDataOffset() correctly handle the tag multi-byte format.

However when it comes to matching/finding the tag values, the structure is ignored and a byte-wise comparison is done instead, which means the const values used for the match must include the Tag class, constructed bit, multi-byte bits and tag identifier values.
This seemed acceptable for an implementation that doesn't really care about dynamic tag values, but it may cause invalid TLV to be accepted and even match incorrectly. It probably could use a re-write especially since we are no longer supporting older (slower) smartcards.
I'll leave this open for any discussion about whether this specifically introduces security problems, but otherwise I'll add this to the enhancement wishlist which means it may not make the FIPS 140 build.