[crypto] HaiCrypt_Clone(): sets up receiver clone properly

[funman]

This fixes key rotation when kk changes
Remote TX sends one even key on initial handshake, and later sends two (even and odd) keys in a KMREQ USER packet

Here is srt-live-transmit showing initial handshake, and key update:

---

15:45:37.523916/SRT:RcvQ:w1.N:SRT.cn: processSrtMsg_KMRSP: cmd=4(KMRSP) len=224 KmState: SND=SECURED RCV=SECURED 15:45:37.524445/SRT:RcvQ:w1.N:SRT.cn: @234271448: Connection established to: 10.129.73.178:5000 SRT source connected

15:45:42.285301/SRT:RcvQ:w1!W:SRT.cn: KMREQ/rcv: (snd) Rx process failure - BADSECRET 15:45:42.285372/SRT:RcvQ:w1.N:SRT.cn: processSrtMsg_KMREQ: cmd=3(KMREQ) len=288 KmState: SND=BADSECRET RCV=BADSECRET 15:45:42.285410/SRT:RcvQ:w1!W:SRT.cn: @234271448: KMREQ FAILURE: BADSECRET - rejecting per enforced encryption --------------------

[kierank]

Is this a potential security issue as it was using uninitalised data before?

[maxsharabayko]

From what I can see, the issue is on the caller (initiator) to become a payload receiver. It creates the TX key and clones it into the RX crypto instance. The m_hRcvCrypto->ctx then points to the m_hSndCrypto->ctx_pair[0] instead of its own. The proposed fix resolved this by setting the m_hRcvCrypto->ctx = m_hRcvCrypto->ctx_pair[0].
I assume the status should be set to HCRYPT_CTX_S_ACTIVE then. @jeandube please confirm.

The m_hRcvCrypto->ctx is however not used to decrypt the payload. The appropriate ctx_pair[ ] is used instead. Each time a payload is decrypted the m_hRcvCrypto->ctx is set to an appropriate ctx_pair (see the HaiCrypt_Rx_Data). Thus "resolving" (or masking) the issue.

@funman How to reproduce the BADSECRET issue? Should the key refresh be sent before even sending any data packet?

[jeandube]

@maxsharabayko going through this thread

[jeandube]

@maxsharabayko Any ref to the issue being fixed by this PR?

[ethouris]

Just such that the original reporter claims that the change fixes the problem. The matter is only that we need to make sure that there was any chance for this problem to happen and under what circumstances so that we can confirm the fix.

[kierank]

This occured with an independent SRT implementation with the key rotation behaviour described in the first message. Kieran

…

On Tue, 2 Apr 2024, 13:21 Sektor van Skijlen, ***@***.***> wrote: Just such that the original reporter claims that the change fixes the problem. The matter is only that we need to make sure that there was any chance for this problem to happen and under what circumstances so that we can confirm the fix. — Reply to this email directly, view it on GitHub <#2905 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABDEEGBQZRQNGJ4N3UZY2LY3KIDLAVCNFSM6AAAAABELE4OJSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZRG42DMNRQGQ> . You are receiving this because you commented.Message ID: ***@***.***>

[kierank]

The author is in my team

…

On Tue, 2 Apr 2024, 17:01 Sektor van Skijlen, ***@***.***> wrote: Hard to believe that you accidentally made the exactly same kind of bug, frankly speaking. — Reply to this email directly, view it on GitHub <#2905 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABDEEHQ45A6KKWYFCJD3G3Y3LB23AVCNFSM6AAAAABELE4OJSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZSGI4TIMJUHE> . You are receiving this because you commented.Message ID: ***@***.***>

[funman]

Le mar. 2 avr. 2024, 12:57, Maxim Sharabayko ***@***.***> a écrit :

funman <https://github.com/funman> How to reproduce the BADSECRET issue? Should the key refresh be sent before even sending any data packet?

No, the key refresh can be sent after a couple data packets. I will give you a pcap if it makes things clearer, it will have to wait a couple days though. If you wanna reproduce before, send both keys (odd and even) in the key refresh packet rather than only one.

[ethouris]

@funman The pcap would be nice, no matter the time to wait.

What is most interesting for us is that: is there any special socket options (for SRTO_KMREFRESHRATE or SRTO_KMPREANNOUNCE) or it should work also with defaults, whether the transmission was in only one on both directions, if one, which side (caller or listener) was sending, if both, which achieved the key switch period earlier, and more-less what was the transmission bitrate.

[funman]

badsecret.pcapng.zip

% ./srt-live-transmit 'srt://10.129.73.178:5000?latency=500&passphrase=ristbetter' udp://127.0.0.1:2000  -ll info
Media path: 'srt://10.129.73.178:5000?latency=500&passphrase=ristbetter' --> 'udp://127.0.0.1:2000'
10:44:30.814802/SRT:RcvQ:w1.N:SRT.cn: processSrtMsg_KMRSP: cmd=4(KMRSP) len=224 KmState: SND=SECURED RCV=SECURED
10:44:30.816397/SRT:RcvQ:w1.N:SRT.cn: @449459863: Connection established to: 10.129.73.178:5000
SRT source connected
10:44:34.103475/SRT:RcvQ:w1!W:SRT.cn: KMREQ/rcv: (snd) Rx process failure - BADSECRET
10:44:34.103525/SRT:RcvQ:w1.N:SRT.cn: processSrtMsg_KMREQ: cmd=3(KMREQ) len=288 KmState: SND=BADSECRET RCV=BADSECRET
10:44:34.103542/SRT:RcvQ:w1!W:SRT.cn: @449459863: KMREQ FAILURE: BADSECRET - rejecting per enforced encryption
10:44:35.104698/SRT:RcvQ:w1!W:SRT.cn: KMREQ/rcv: (snd) Rx process failure - BADSECRET
10:44:35.104765/SRT:RcvQ:w1.N:SRT.cn: processSrtMsg_KMREQ: cmd=3(KMREQ) len=288 KmState: SND=BADSECRET RCV=BADSECRET
10:44:35.104787/SRT:RcvQ:w1!W:SRT.cn: @449459863: KMREQ FAILURE: BADSECRET - rejecting per enforced encryption
10:44:36.100720/SRT:RcvQ:w1!W:SRT.cn: KMREQ/rcv: (snd) Rx process failure - BADSECRET
10:44:36.100746/SRT:RcvQ:w1.N:SRT.cn: processSrtMsg_KMREQ: cmd=3(KMREQ) len=288 KmState: SND=BADSECRET RCV=BADSECRET
10:44:36.100753/SRT:RcvQ:w1!W:SRT.cn: @449459863: KMREQ FAILURE: BADSECRET - rejecting per enforced encryption
^C

Stream is generated with:

ffmpeg -re -f lavfi -i "smptehdbars=rate=60:size=1280x720" -f lavfi -i "sine=frequency=400:sample_rate=48000" -vf drawtext="text='FOO BAR:timecode=01\:00\:00\:00':rate=25:x=(w-tw)/2:y=(h-lh)/2:fontsize=48:fontcolor=white:box=1:boxcolor=black" -c:v h264 -profile:v baseline -pix_fmt yuv420p -preset ultrafast -tune zerolatency  -b:v 500k -minrate 700k -maxrate 700k -bufsize 700k -c:a aac -f mpegts -muxrate 700k udp://127.0.0.1:1212\?pkt_size=1316

[maxsharabayko]

@funman Thank you. Now everything is clear. Kudos for the fix! 👏

[kierank]

Is this a security issue?

[maxsharabayko]

I would not classify it as a security issue.
A connection with an alternative SRT implementation gets rejected (if it would instead use a wrong encryption key, that would be an issue). Also, there is no uninitialized memory access in this case. Just the RX context points to the TX context (both are the same, as RX was cloned from TX), a bug fixed by this PR.

[funman]

That's my understanding too, also these objects should be freed at the same time so no use after free either