search

titusz / jcs

JCS - JSON Canonicalization for Python 3
3 stars 1 forks source link

Raise ValueError when output != input #1

Open jmgurney opened 2 years ago

jmgurney commented 2 years ago

One of the dangers is that python integers can be outside the range of floats which are used for JCS.

This causes:

>>> jcs.canonicalize(2**60)
b'1152921504606847000'
>>> 2**60
1152921504606846976

This should raise a ValueError or other exception when the output from canonicalize does not match the input. This can cause security issues w/ signatures as it allows modification of the object in undetectable and unexpected ways.

I'd be fine w/ only disabling the exception when an argument is provided, say, allow_integer_mismatch=True.

titusz commented 2 years ago

Thanks for reporting this. This is indeed seems to be an ugly issue. Because I am only packaging the code for pypi you should open an issue with the original upstream @cyberphone at https://github.com/cyberphone/json-canonicalization. I will integrate it here as soon as we get a fix from the original author.

jmgurney commented 2 years ago

Raised here: https://github.com/cyberphone/json-canonicalization/issues/20#issuecomment-1040909845

titusz commented 2 years ago

A foolproof, fast and dirty, workaround would be to make a full round-trip test like here for example: https://github.com/iscc/iscc-core/blob/main/iscc_core/utils.py#L23 I could add that with an option to deactivate the round-trip test.

cyberphone commented 2 years ago

@titusz This is possible but then you are not compatible with RFC 8785 since 333333333.33333329 would probably be flagged as an error. It all depends on how the parser deal with this. In fact, the integer test I just added to my Python reference code broke the RFC as well. I had to revert it :( :(

You are of course free to extend or deviate from the RFC, but my reference should not.

BTW, rounding errors and lost digits like you got (and in any other I-JSON compatible system as well) with the integer above is not only a JCS/RFC 8785 issue; applications that rely on true 64-bit integers are not likely to work either. Good "schema" design is the only interoperable solution, I am aware of.

titusz commented 2 years ago

@cyberphone My intention with this repository was only to package your code for pypi distribution and I am not eager do deviate from your reference implementation. So I will stick with your original code. But maybe we could/should add some "gotcha" note to the readme.