Iv hex encoding #51
Iv hex encoding #51
Conversation
Because WriteUtil.writeHexadecimal used Integer.toHexString which
converts each byte into an integer and then writes that integer using
the minimum required number of hexadecimal digits results would vary
and negative bytes would be written using 8 hexadecimal digits with
the first 6 being ffffff and positive bytes between 0 and 15 would
only be written using 1 hexadecimal digit. This would cause the IV
in EXT-X-KEY tags to be corrupted.
Using String.format("%02x", b) instead means all bytes will be written
correctly. They won't take up more than 2 characters since no
integer-conversion is performed and if only 1 digit is required they
will be zero-padded.
| @@ -54,17 +54,20 @@ public static float parseFloat(String string, String tag) throws ParseException | |||
|
|
|||
| if (matcher.matches()) { | |||
| String valueString = matcher.group(1); | |||
| if (valueString.length() % 2 != 0) { | |||
| throw ParseException.create(ParseExceptionType.INVALID_HEXADECIMAL_STRING, tag, hexString); | |||
| } | |||
There was a problem hiding this comment.
parseHexadecimal doesn't know what it's parsing, so it shouldn't assume the length of the string is a multiple of 2. If you want to validate the IV (is the IV required to have an even length?), then that validation probably belongs in PlaylistValidation.
There was a problem hiding this comment.
Since each byte maps to 2 hexadecimal digits it has to be a multiple of 2 - unless you want to assume the last half byte is all zeroes.
There was a problem hiding this comment.
That makes sense, let's keep it like you implemented.
| for (char c : valueString.toCharArray()) { | ||
| bytes.add(hexCharToByte(c)); | ||
| for (int i = 0; i < valueString.length(); i += 2) { | ||
| bytes.add((byte)(Short.parseShort(valueString.substring(i, i+2), 16) & 0xFF)); |
There was a problem hiding this comment.
The bug description seems to be affecting the writing, not the parsing. Is there a problem with hexCharToByte?
There was a problem hiding this comment.
I see, the issue is 2 hex chars fit within 1 byte. I don't work with low level very often. :( I think I'm okay with this implementation.
There was a problem hiding this comment.
I noticed issues with parsing after I created the issue and updated the PR, I can open up another issue if you want
|
Unfortunately, not all the tests pass because of the faulty implementation. Can you please make the following changes? // from
assertEquals(
Arrays.asList((byte) 1, (byte) 2, (byte) 3, (byte) 4, (byte) 10, (byte) 11, (byte) 12, (byte) 13,
(byte) 5, (byte) 6, (byte) 7, (byte) 8, (byte) 14, (byte) 15, (byte) 9, (byte) 0),
encryptionData.getInitializationVector());
// to
assertEquals(
Arrays.asList((byte) 0x12, (byte) 0x34, (byte) 0xAB, (byte) 0xCD,
(byte) 0x56, (byte) 0x78, (byte) 0xEF, (byte) 0x90),
encryptionData.getInitializationVector());Constants.java: // from
public static final int IV_SIZE = 16;
//Against the spec but used by Adobe
public static final int IV_SIZE_ALTERNATIVE = 32;
// to
public static final int IV_SIZE = 8;
//Against the spec but used by Adobe
public static final int IV_SIZE_ALTERNATIVE = 16;The rest looks good. @sunglee413 this is good to go once the changes above are made to fix the tests. |
|
The IV sizes shouldn't be changed - an IV in AES is 128 bits, so 16 bytes is correct - so I'll just increase the length of the IV in the test. |
The IV was too short and the expected value was wrong
|
Done. |
|
Yeah, I can't maths. LGTM! |
Fix for #50