Fix found: significant aes cbc decryption bug in rpiplay

[fduncanh]

Edit: The bug is in crypto.c. (EVP_DecriptFinal is usually called after EVP_DecriptUpdate to finalize the unencrypted remainder ( < 16 bytes) of the AES-CBC encrypted packet. But this is is already being done in raop_buffer.c: so it is not done in this code.

A simple workaround that seems to stop the last 16 encrypted bytes from being lost is the replacement where

line 178 lib/raop_buffer.c: aes_cbc_decrypt(aes_ctx_audio, &data[12], output, encryptedlen);

is modified to

aes_cbc_decrypt(aes_ctx_audio, &data[12], output, encryptedlen + 15);

Without this, EVP_DecriptUpdate seems to leave the last part of the encrypted packet for decryption by a call to EVP_DecriptFinal, but this is not done in crypto.c. Adding 15 to the packet length reported to EVP_DecriptUpdate fools it into decrypting all the encrypted bytes: the unencrypted <16 byte remainder is then handled "manually" in raop_buffer.c . The "correct" solution would be to rework crypto.c so it follows the OpenSSL EVP guidelines, but the simple fix seems to do job without that. (sidecomment: the author of the OpenSSL EVP utilities had a spelling issue with "cript" as opposed to "crypt"!).

Since encryptedlen %16 = 0, there is no danger of a buffer overrun by aes_cbc_decrypt, which eventually calls EVP_DecriptUpdate from OpenSSL.

I would post a pull request but they are being ignored. It's a simple fix. I'll add it to PR #266, but this is being ignored. This fix will be used to add ALAC support to the fork UxPlay 1.3x https://github.com/FDH2/UxPlay (which only uses GStreamer rendering)

Edit 2:

Fixing this bug has made it easy to add support for ALAC streams, it was this bug that prevented adding ALAC support before. Fixing it also seems to improve aac (MPEG4) audio (although the lost 16 bytes didnt seem to have much effect on AAC streams).

The missing bytes were fatal for ALAC, as the start of the Airplay stream seems to involve an initial send of a 32 byte header, and the bug meant that the second (last) set of 16 bytes were lost.

------------------------------------------------end of edit --------------------------------------------------

I've been trying to get ALAC streams to play in rpiplay (and uxplay), with gstreamer. Comparing the audio stream from shairplay (parent of raop in rpiplay) with that from rpiplay, I found that the last 16 entrypted bytes are each set to 00 (before any residual less than 16 unencrypted bytes are appended to the packet) The same is true for the AAC-ELD mpeg v4 stream that does play in current rpiplay.

This is very surprising: is this a feature of an AAC-ELD packet? (if this was a corruption of the AAC packet I wouldn't expect it to play, but perhaps AAC is resilent against losing those 16 bytes).

example of a decrypted (not yet decoded) AAC packet

8d867f5ffff056c625098e2241815f9d
773a132798b7d50262652a6160b24843
83e0d9ec841c7fff27f01308a20a1f36
8a0759aaafb8771158d8fd04e2fd23f1
bf32b51c35a6fe5bf3badaf074ff6d4b
4f2ceafd3ce3639784759ce322a24637
252042b2cea79099c46a29c3445963f0
4a48f4074773176c10a3d23ad6dd4607
ae1ee145438880af13edf5f7e652b8ce
13db43265d183c49c19dae12a24c1994
5c2d18fdfe03d7de2b96adc1b553ba33
fc73831ba7c49acb6450720ea33e60e5
a41ad6926b4ceea8c41ecf60c4dfb74a
cde510cf15134409699db64d23f9f7ab
00000000000000000000000000000000          <--------   unexpected!  (but audio still  plays)
927bf5aa5a5cab2d7f585aa800

example of a decrypted (not yet decoded) ALAC packet :

2000000400130805ddfa0c0531fc4413
080369fd240363fdefff00067a3cd609
39bb74db4b2608120698035792da85e4
439510ae2ad18e336a2684921a01d3da
bc2249996b132db975a647230ab2aca9
4e260189225693b455011a54534e3df2
901193394d3a5d0a92cb2a391a55ade6
ab70b7516ea95e3b8c22a9298cdd21c4
98b01d8e7496ed2be914f12c68545415
7aace55197ba862241892a94c2931d5c
25a6836e9ba50a40bfe000ed66698c54
145087695086914dcb23aa4e2b70424d
a6e48bbd516c3536ac453da4ef0534f1
535b6d0c8eee9b6f39a0d42b520b74e2
1e6356921b7109d4a14057126535b34e
aa0cd31e5ba447b288442599549da451
8e9f71b1dd12d276da54a875954aa2c2
34aa32293114dc594d2c653ca834ba80
00000000000000000000000000000000      <----------------- definitely an error (by comparison with shairplay)
65d48c5b6a9d2bac4d1c

The rest of the ALAC packet is good. This is not an inserted extra 16 bytes of 00, it is a lost 16 bytes of the decrypted packet. The last 16 encrypted bytes are NOT 00 before the AES CTR decryption is applied.

The relevant code from raop_buffer.c :+1:

    encryptedlen = payload_size / 16*16;
    memset(output, 0, payload_size);
    // Need to be initialized internally                                                                                                                                          
    aes_ctx_t *aes_ctx_audio = aes_cbc_init(raop_buffer->aeskey, raop_buffer->aesiv, AES_DECRYPT);
    aes_cbc_decrypt(aes_ctx_audio, &data[12], output, encryptedlen);
    aes_cbc_destroy(aes_ctx_audio);

    memcpy(output + encryptedlen, &data[12 + encryptedlen], payload_size - encryptedlen);
    *outputlen = payload_size;

    for (int i = 0; i < payload_size ; i ++) {
      printf("%2.2x", *(output + i));                              My added code to print the packet
      if(i%16 ==  15) printf("\n");
    }
    printf("\n");

[FD-]

This is an excellent find! Thanks for your efforts! I'm sorry I'm so slow with my responses at the moment, I'm just very busy with other stuff. If you submit a PR with just this fix (or a rework to make our crypto code follow the OpenSSL EVP guidelines), I'll gladly accept it :)

[pallas]

I have a potential fix for this that involves using Final. I believe the issue is a padding setting, but I haven't tested with OpenSSL 3.0 yet & I'm a little afraid to install it on Raspberry Pi OS.

[pallas]

Please check out the commit I just pushed, b9e096354fce474d5c9ba1e4a04a85bb9f8cdb03

[pallas]

It sounds like that commit fixes this issue. I want to refactor the lib/crypto stuff into the actual callers & replace the aes_init/aes_destroy calls with calls to EVP_DecryptInit_ex once and EVP_CIPHER_CTX_reset to prevent a much of malloc/free in this tight loop.