Re: [FFmpeg-devel] [PATCH] avutil/twofish: Fixed decryption

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Sebastian Kirmayer:
> The previous implementation swapped the two halves of the plaintext. The
> existing tests only decrypted data with a plaintext of all zeroes, which is
> not affected by swapping the halves. Tests which detect the old buggy behavior
> have been added.
> 
> Signed-off-by: Sebastian Kirmayer <ffmpeg@kirmayer.eu>
> ---
>  libavutil/tests/twofish.c | 15 ++++++++++++---
>  libavutil/twofish.c       |  8 ++++----
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/libavutil/tests/twofish.c b/libavutil/tests/twofish.c
> index 74e0926e..7e8b1292 100644
> --- a/libavutil/tests/twofish.c
> +++ b/libavutil/tests/twofish.c
> @@ -39,7 +39,7 @@ int main(int argc, char *argv[])
>      };
>      uint8_t temp[32], iv[16], rpt[32] = {0};
>      const int kbits[3] = {128, 192, 256};
> -    int i, j, err = 0;
> +    int i, j, k, err = 0;
>      struct AVTWOFISH *cs;
>      cs = av_twofish_alloc();
>      if (!cs)
> @@ -70,10 +70,19 @@ int main(int argc, char *argv[])
>              memcpy(Key+16,Key,(kbits[j]-128) >> 3);
>              memcpy(Key,rpt,16);
>              memcpy(rpt,temp,16);
> +            av_twofish_crypt(cs, temp, temp, 1, NULL, 1);
> +            for (k = 0; k < 16; k++) {
> +                // Need to compare to Key here, because the plaintext comes
> +                // from rpt but was moved over to Key.
> +                if (Key[k] != temp[k]) {
> +                    av_log(NULL, AV_LOG_ERROR, "%d %02x %02x\n", k, Key[k], temp[k]);
> +                    err = 1;
> +                }
> +            }
>          }
>          for (i = 0; i < 16; i++) {
> -            if (rct[3 + j][i] != temp[i]) {
> -                av_log(NULL, AV_LOG_ERROR, "%d %02x %02x\n", i, rct[3 + j][i], temp[i]);
> +            if (rct[3 + j][i] != rpt[i]) {
> +                av_log(NULL, AV_LOG_ERROR, "%d %02x %02x\n", i, rct[3 + j][i], rpt[i]);
>                  err = 1;
>              }
>          }
> diff --git a/libavutil/twofish.c b/libavutil/twofish.c
> index d84fa4f3..649b4bc4 100644
> --- a/libavutil/twofish.c
> +++ b/libavutil/twofish.c
> @@ -260,10 +260,10 @@ static void twofish_decrypt(AVTWOFISH *cs, uint8_t *dst, const uint8_t *src, uin
>          P[3] ^= AV_RL32(iv + 12);
>          memcpy(iv, src, 16);
>      }
> -    AV_WL32(dst, P[2]);
> -    AV_WL32(dst + 4, P[3]);
> -    AV_WL32(dst + 8, P[0]);
> -    AV_WL32(dst + 12, P[1]);
> +    AV_WL32(dst, P[0]);
> +    AV_WL32(dst + 4, P[1]);
> +    AV_WL32(dst + 8, P[2]);
> +    AV_WL32(dst + 12, P[3]);
>  }
>  
>  av_cold int av_twofish_init(AVTWOFISH *cs, const uint8_t *key, int key_bits)
> 

Confirmed the bug through testing with more interesting data than the
current test does. Will apply this on Sunday unless there are objections.
Thanks.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".