Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trouble with CFB and OFB with the C implementation #65

Closed
yqxk opened this issue Feb 21, 2024 · 10 comments
Closed

Trouble with CFB and OFB with the C implementation #65

yqxk opened this issue Feb 21, 2024 · 10 comments

Comments

@yqxk
Copy link

yqxk commented Feb 21, 2024

Hello, I can get the python CFB and OFB tests working with the aes_ni implementation, but somehow it fails with the C and amd64.asm implementation.

I have a Dockerfile reproducing this (my CPU is 12th Gen Intel(R) Core(TM) i9-12900H, but I've reproduced this on a few other machines, and with gcc 9.3)

FROM gcc:13.2.0
RUN apt-get --allow-unauthenticated update && apt-get --allow-unauthenticated install -y python3-dev git yasm python3-psutil
RUN git clone https://github.com/BrianGladman/aes
WORKDIR /aes/python_binding
RUN git checkout 646c5d4

# This uses the aes_ni implementation
RUN gcc -fPIC -shared -I.. -O2 -Wall -g -I/usr/include/python3.11 -D__PROFILE_AES__ -march=skylake ../aes_ni.c ../aes_modes.c ../aescrypt.c ../aeskey.c ../aestab.c ./aesmodule.c -o aes.cpython-311-x86_64-linux-gnu.so
RUN python3 demo.py  # works
RUN python3 test_aes.py  # works

# This uses the amd64 asm implementation
RUN yasm -f elf64 -a x86 -D__GNUC__ ../aes_amd64.asm -o asm.o
RUN gcc -fPIC -shared -I.. -O2 -Wall -g -I/usr/include/python3.11 -D__PROFILE_AES__ -march=skylake -U__AES__ -DASM_AMD64_C ./asm.o ../aes_modes.c ../aescrypt.c ../aeskey.c ../aestab.c ./aesmodule.c -o aes.cpython-311-x86_64-linux-gnu.so
RUN python3 demo.py  # works
RUN python3 test_aes.py  # fails

# This uses the C implementation, and doesn't
RUN gcc -fPIC -shared -I.. -O2 -Wall -g -I/usr/include/python3.11 -D__PROFILE_AES__ -march=skylake -U__AES__ ../aes_ni.c ../aes_modes.c ../aescrypt.c ../aeskey.c ../aestab.c ./aesmodule.c -o aes.cpython-311-x86_64-linux-gnu.so
RUN python3 demo.py  # works
RUN python3 test_aes.py  # fails

Am I doing something wrongly?

@BrianGladman
Copy link
Owner

Can you provide the test output when test_aes.py fails? Without this it is impossible to tell what is going wrong.

@yqxk
Copy link
Author

yqxk commented Feb 21, 2024

This is the failure message for test_aes.py with the amd64 asm implementation; here I hexlify-ed and printed the contents of the pt_bytes and ct_bytes variables right before first assert statement in the test (on line 158 of the unmodified test_aes.py).

The C implementation runs into the same failure (with the same pt_bytes/ct_bytes).

Skipping this particular test (test_cfb128_aes192_f315) to execute the OFB test will also show that that fails in a similar fashion.

> [13/16] RUN python3 test_aes.py:

1.754 pt 6ac3bde62b46989ee037751d7e9d18ff832ac472e45e1eed758dee13e03a15a78273644b1132511c40ff3bfe4083c8e6e716247d892f6e198cadee2df83d1ebb
1.754 ct cdc80d6fddf18cab34c25909c99a417467ce7f7f81173621961a2b70171d3d7a2e1e8a1dd59b88b1c8e60fed1efac4c9c05f9f9ca9834fa042ae8fba584b09ff
1.754 Traceback (most recent call last):
1.754   File "/aes/python_binding/test_aes.py", line 232, in <module>
1.755     test_cfb128_aes192_f315()
1.755   File "/aes/python_binding/test_aes.py", line 160, in test_cfb128_aes192_f315
1.755     assert pt_bytes == ct_bytes, "AES CFB mode encryption failure"
1.755            ^^^^^^^^^^^^^^^^^^^^
1.755 AssertionError: AES CFB mode encryption failure
------
Dockerfile:18
--------------------
  16 |     RUN gcc -fPIC -shared -I.. -O2 -Wall -g -I/usr/include/python3.11 -D__PROFILE_AES__ -march=skylake -U__AES__ -DASM_AMD64_C ./asm.o ../aes_modes.c ../aescrypt.c ../aeskey.c ../aestab.c ./aesmodule.c -o aes.cpython-311-x86_64-linux-gnu.so
  17 |     RUN python3 demo.py
  18 | >>> RUN python3 test_aes.py
  19 |     
  20 |     # This uses the C implementation, and doesn't
--------------------
ERROR: failed to solve: process "/bin/sh -c python3 test_aes.py" did not complete successfully: exit code: 1

@BrianGladman
Copy link
Owner

Thank you for the further information. I do not see anything obvious that you are doing wrong. At the same time this code is now very stable and I have not had any other reports of failures for over the last five years so it seems likely that the issue is specific to the code in combination with the specific compiler you are using.

Since both the C and assembler versions fail but the AES_NI version does not, it seems likely that the error occurs in the C code that is common to the C and assembler builds but not shared with the AES_NI build. This leads me to suspect that the AES key generation code is being mis-compiled. A first step would be to turn off all compiler optimisation and see if the error persists. If it doesn't you could then turn on optimisation in a subset of the C files (e.g. aeskey.c) to find which if any are being mis-compiled.

Sadly, I cannot help directly with this myself as I do not use the build environment that you are using. I am happy to offer further advice if you need it.

@yqxk
Copy link
Author

yqxk commented Feb 22, 2024

I tried the following, but I still get the same failure:

  • disabling optimisations
  • running aestst.c/aes_avs.c instead of the python tests
  • using gcc 9, 11, 13 as well as clang 14
  • running on different machines

I understand that you don't have the environment I'm using. I've tidied up the Dockerfile below in case anyone else wants to investigate: this demonstrates (in my environment) that the regular one-block AES works (aestst.c), but the C and assembler versions of the CFB/OFB tests fail (aes_avs.c).

As you said, this code is very stable by now, so this behavior may be peculiar to my environment and so you may want to investigate further only if other people run into the same issue. Fortunately for me I can live without the OFB/CFB/CTR modes, so please feel free to close the issue (though I'll be happy to run commands in my environment to help debug this if you want).

FROM gcc:13.2
RUN apt-get --allow-unauthenticated update && apt-get --allow-unauthenticated install -y git yasm
RUN git clone https://github.com/BrianGladman/aes
WORKDIR /aes/test
RUN git checkout 646c5d4

#-------- AES_NI implementation --------
RUN gcc -O0 -Wall -march=skylake \
   ../aes_modes.c ../aeskey.c ../aestab.c ../aescrypt.c ../aes_ni.c \
   ../aestst.c
RUN ./a.out  # works

RUN gcc -O0 -Wall -march=skylake \
   ../aes_modes.c ../aeskey.c ../aestab.c ../aescrypt.c ../aes_ni.c \
   ../aes_avs.c ../aesaux.c
RUN ! ./a.out | grep Error  # works
#-------- AES_NI implementation --------


#-------- C implementation --------
RUN gcc -O0 -Wall -march=skylake \
   -U__AES__ ../aes_modes.c ../aeskey.c ../aestab.c ../aescrypt.c \
   ../aestst.c
RUN ./a.out  # works

RUN gcc -O0 -Wall -march=skylake \
   -U__AES__ ../aes_modes.c ../aeskey.c ../aestab.c ../aescrypt.c \
   ../aes_avs.c ../aesaux.c
RUN ! ./a.out | grep Error  # fails
#-------- C implementation --------


#-------- asm implementation --------
RUN yasm -f elf64 -a x86 -D__GNUC__ ../aes_amd64.asm -o asm.o
RUN gcc -O0 -Wall -march=skylake \
   -U__AES__ -DASM_AMD64_C ../aes_modes.c ../aeskey.c ../aestab.c ./asm.o \
   ../aestst.c
RUN ./a.out  # works

RUN gcc -O0 -Wall -march=skylake \
   -U__AES__ -DASM_AMD64_C ../aes_modes.c ../aeskey.c ../aestab.c ./asm.o \
   ../aes_avs.c ../aesaux.c
RUN ! ./a.out | grep Error  # fails
#-------- asm implementation --------

@BrianGladman
Copy link
Owner

Thanks for the further work on this. It definitely needs resolving so I will leave it open and see if I can find a colleague who is willing to investigate it.

@BrianGladman
Copy link
Owner

I have tracked down the cause of the difference to aeskey.c where these lines occur in each of the key setting subroutines:

cx->inf.l = 0;
cx->inf.b[0] = 10 * AES_BLOCK_SIZE;

where cx and inf are defined as:

typedef struct ALIGNED_(16)
{ uint32_t ks[KS_LENGTH];
aes_inf inf;
} aes_crypt_ctx;

typedef union
{ uint32_t l;
uint8_t b[4];
} aes_inf;

byte b[2] in the cx_>inf is used by the CFB and OFB routines to hold the position within blocks as the process proceeds and must be set to zero before the encryption/decryption starts. All the bytes are set to zero in the key setting routines by setting 'l' in the union to zero but this doesn't appear to happen since bytes in 'b' are not set to zero when I use an AES DLL that I was able to build with GCC and run with MSVC.

I will see if I can find out why this is happening if I can get an assembly code listing of aeskey.obj

@BrianGladman
Copy link
Owner

This was a bug in the OFB/CFB code that was using a part of the context used by the main code. It should all work now.

Thank you for discovering this bug. I guess it shows that OFB/CFB is pretty rarely used.

@yqxk
Copy link
Author

yqxk commented Feb 26, 2024

Good to know! Would you have the changes uploaded somewhere that I can test in my environment? It doesn’t seem to have been pushed to this repository.

@BrianGladman
Copy link
Owner

I forget to push it, done now.

@yqxk
Copy link
Author

yqxk commented Feb 27, 2024

I can confirm that the tests pass in my environment now. Thank you for your help!

@yqxk yqxk closed this as completed Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants