ultravideo / uvgRTP

An open-source library for RTP/SRTP media delivery
BSD 2-Clause "Simplified" License
318 stars 90 forks source link

What is the correct length field of a zrtp message? #40

Closed jrsnen closed 3 years ago

jrsnen commented 3 years ago

I was refactoring zrtp, but I couldn't figure out what is the correct length of a zrtp message.

Most of zrtp messages calculate their length like this:

https://github.com/ultravideo/uvgRTP/blob/00706d63712590b89770919ad54d7a252420b3df/src/zrtp/dh_kxchng.cc#L46

https://github.com/ultravideo/uvgRTP/blob/00706d63712590b89770919ad54d7a252420b3df/src/zrtp/confirm.cc#L38

But then hello calculates it like this: https://github.com/ultravideo/uvgRTP/blob/00706d63712590b89770919ad54d7a252420b3df/src/zrtp/hello.cc#L48

The difference is one byte I believe. In general this size should be in 32 bit words which means its incorrect regardless, but this difference is throwing me off even more.

Section 5.2 of RFC

All ZRTP messages begin with the preamble value 0x505a, then a 16-bit
length in 32-bit words.  This length includes only the ZRTP message
(including the preamble and the length) but not the ZRTP packet
header or CRC.  The 8-octet Message Type follows the length field.

It would suggest that the length is calculated in the same way for all. Some of the sizes are already give in the document.

altonen commented 3 years ago

To be honest, I'm very unhappy with the ZRTP code in general and after getting an MVP ready came the paper, holidays and the second paper and somehow refactoring the code into something more manageable just got postponed and postponed.

Getting that code to more maintainable is not a huge task but the code is pretty ugly, not going to lie. Based on the snippet from the RFC you pasted, the first two lengths seem more valid to me.

I think the 1-byte mismatch is because the ZRTP header in [1] has the payload field is set to 1 (it should be zero but MSVC complains) but I forgot the subtract 1 byte from the length calculation. The first two calculations do not use the uvgrtp::frame::zrtp_frame for length calculation so they do not have that extra byte. So I would change how the hello message's length is calculated

[1] https://github.com/ultravideo/uvgRTP/blob/master/include/frame.hh#L157

jrsnen commented 3 years ago

This was actually pointed out to me by @fador. If you look carefully, the spec says the length should be in 32-bit words and sizeof returns the size in bytes or 8-bit chunks. This means the size should be divided by 4.

Can you explain what are the relations between uvgrtp::frame::zrtp_frame, zrtp_header, zrtp_msg and individual zrtp messages, such as hello or commit? This confuses me and I'm wondering if some of those could be combined for simplicity?

I didn't test the difference, I just saw it when trying to understand the code. I'm doing some refactoring for zrtp, where all the messages will use same function for constructing the header that is why I created this issue so I'll understand how to do the refactoring correctly.

One of the files also said in TODO that these should be in network byte order. Is this correct?

jrsnen commented 3 years ago

I think the size is now calculated correctly with 5a80f616f40b35974bc44ccb41730becfd834458

This is one aspect that would need careful checking that the sizes are correct in general, but at least the size calculation should be according to specification.