Not enough precision in FixedPointField

[NicsTr]

The call to normalize: https://github.com/secdev/scapy/blob/a1f999fb1721a64415af75d575cabde19625a6f5/scapy/fields.py#L3189 is rounding too aggressively and some bits are lost in the process.

See this snippet that highlights a non-expected result with Scapy 2.5.0rc1.dev20, commit a1f999fb1721a64415af75d575cabde19625a6f5:

fp = FixedPointField("foo", 0, 64, 32)
hex(fp.any2i(None, fp.i2h(None, 0x6f469178))) == '0x6f469178'

I tried to fix it by using math.ceil(math.log10(pw)) instead of int(math.log10(pw)) but it doesn't seem sufficient. When using math.ceil(math.log10(pw)) + 1 it works as expected. I'm not familiar enough with scapy nor decimal so I'm not sure to understand the rationale behind using decimal to manipulate fixed point precision from binary data (my use case is for NTP's TimeStampField) so my fix is ad hoc; in any case, I can open a PR with this patch if you want.

[gpotter2]

Hi, sorry for the delay. I'm not sure to understand your example exactly. Could you point out the expected result? Thanks

[NicsTr]

Hi. It seems that I made a mistake and that my example above is not triggering the bug I originally encountered...

This other example should better highlights what I mean:

fp = FixedPointField("foo", 0, 64, 32)
assert fp.any2i(None, fp.i2h(None, 3832717255)) == 3832717255

The expected result is for the equality to be True since we are just converting back and forth. In latest commit 6d0d230701ffd4e64e6f976e21bd7a44415a087e, this is not the case (at least on my machine). I found the above example (and a lot more) by running the following very basic fuzzing snippet:

while True:
    tmp = int.from_bytes(os.urandom(4), "big")
    if fp.any2i(None, fp.i2h(None, tmp)) != tmp:
        print(f"{tmp} have not the right behaviour")
        break # remove to be flooded with examples

I found such unexpected behaviour when dealing with NTP packets: upon reading the "Transmit timestamp" field of a captured packet and setting the "Origin Timestamp" while crafting the response, the timestamp bytes value was changed compared to the original packet. The implementation of these fields can be found here and here. Their underlying type is FixedPointField as shown here.

This is a problem because, in NTP interleaved client/server mode, the "Transmit timestamp" field of a response must be strictly equal to the "Origin timestamp" of the request in order for the response to be taken into account.

Also, please ignore the part of my previous message where I try to fix this using ugly rounding as it does not work properly every time.

I hope my description is clearer this time.

[gpotter2]

Sorry for the very, very long delay. Do you have a concrete a example of a packet that fails? The current analysis looks wrong to me.

The provided examples don't make sense: you are using i2h then any2i, so "Internal" to "Human" then "Any" to "Internal". Because you are injecting "Internal" values with a precision that is impossible for the entered fixed point, it makes sense that the results are different.

As you can try for yourself, if you do it the other way around, no issues are shown:

fp = FixedPointField("foo", 0, 64, 32)
while True:
    tmp = int.from_bytes(os.urandom(4), "big")
    if fp.i2h(None, fp.any2i(None, tmp)) != tmp:
        print(f"{tmp} have not the right behaviour")
        break

[NicsTr]

I lost track of the packets or script that lead to the mentioned behaviour.

It was when I tried to manually craft an NTP response from an NTP request. IIRC, I wanted to use for a field of the response the exact same timestamp as in another field of the request. It is not unlikely I was not using the right method to copy the TimeStampField from one packet to the other.