hs-asn1
hs-asn1 copied to clipboard
vincenthz
Fix + Test for #17
I think the change you just made fixes the problem, but this is still an improvement. Specifically checking for lengths long enough to overflow makes the constraint clearer and gives a better error message. And tests are always good :)
As to your question of why ensure doesn't actually ensure the input is long enough: it's because the overflow makes the argument negative in this case. B.length s0 >= n is always true when n is negative, so it always acts as if there is enough input.
I think your code can be slightly simplified to:
let intByteSize = finiteBitSize (undefined :: Int) `quot` 8
lw <- getBytes len
if B.length lw > intByteSize - 1
then fail "invalid length - would overflow"
else return (LenLong len $ uintbs lw)
(Or maybe using bitSize instead of finiteBitSize if we need better backwards-compatibility)
But both of our approaches seem to be slightly too pessimistic, i.e. on a 32-bit machine we reject inputs longer than 3 bytes, even though 4 bytes may work if the number is positive. I don't really know anything about ASN.1 so I can't say if this might be a problem in practice.
@enolan: I agree that further hardening of the code and definitely adding tests is the right thing to do here
I think the right thing to do here is moving to Integer instead of Int so that we don't overflow, and put a reasonable limit on the number of bytes that can be pulled for safety reason.
further data point, the same bug has been present in cereal until 0.3.5.2
Do you mean making the second field of LenLong an Integer instead of an Int? There's no way to write a getBytes :: Integer -> ByteString - an array longer than the machine word size is impossible.
Alternative: accumulate an Integer in uintbs and check if it's bigger than maxBound :: Int before converting it to Int and putting it in LenLong. If it's too big, throw an error. This solves the problem @hreinhardt pointed out - the largest possible length is 2^31-1 bytes or 2^63-1 bytes (~2 GiB or 8 EiB) instead of 2^24-1 bytes or 2^58-1 bytes (~16 MiB or 256 PiB).
If that sounds good I can go make the change.
This might also be a possible solution:
let intByteSize = finiteBitSize (undefined :: Int) `quot` 8
lw <- getBytes len
if (B.length lw < intByteSize) ||
(B.length lw == intByteSize && B.head lw < 128)
then return (LenLong len $ uintbs lw)
else fail "invalid length - would overflow"
It would allow 32/64-bit lengths if they represent positive numbers.
What's the status of this? Id really like to be able to close out my original problem.