open-quantum-safe
Add cf protection to assembler files
Describe the bug
Our annobin check has found out that the library doesn't implement the CET protection https://sourceware.org/annobin/annobin.html/Test-cf-protection.html
because of lack of the corresponding code in ASM files (see the example in the link).
SO even when the C sources are compiled with -fcf-protection the protection is still not enabled
An example patch:
diff -rup liboqs-0.12.0.orig/src/kem/ml_kem/pqcrystals-kyber-standard_ml-kem-768_avx2/invntt.S liboqs-0.12.0/src/kem/ml_kem/pqcrystals-kyber-standard_ml-kem-768_avx2/invntt.S
--- liboqs-0.12.0.orig/src/kem/ml_kem/pqcrystals-kyber-standard_ml-kem-768_avx2/invntt.S 2025-01-06 10:08:13.282017683 +0000
+++ liboqs-0.12.0/src/kem/ml_kem/pqcrystals-kyber-standard_ml-kem-768_avx2/invntt.S 2025-01-06 10:09:33.407463609 +0000
@@ -183,6 +183,7 @@ vmovdqa %ymm11,(64*\off+176)*2(%rdi)
.text
.global cdecl(invntt_avx)
cdecl(invntt_avx):
+ENDBR64
vmovdqa _16XQ*2(%rsi),%ymm0
intt_levels0t5 0
@@ -191,3 +192,20 @@ intt_levels0t5 1
intt_level6 0
intt_level6 1
ret
+
+ .section .note.gnu.property,"a"
+ .align 8
+ .long 1f - 0f
+ .long 4f - 1f
+ .long 5
+0:
+ .string "GNU"
+1:
+ .align 8
+ .long 0xc0000002
+ .long 3f - 2f
+2:
+ .long 0x3
+3:
+ .align 8
+4:
The same should be implemented for all the ASM sources
Thanks for the report. Is it accurate to say that this is something that should ideally be fixed upstream? (i.e., in https://github.com/pq-crystals/kyber for this example)
This looks like a "mechanical" (automat-able) code addition: Am I right with this @beldmit ? All that's needed is adding these lines (always the same?!) to all .S files? Or is there more to it? Or asked differently, could you add such a cat cfgnusection.S >> *.S script to your distro? If that were the case, we could of course also do it in liboqs and not wait for all upstreams to do this.
Unfortunately, it's not enough
Quoting the link https://sourceware.org/annobin/annobin.html/Test-cf-protection.html
If an assembler source file is used as part of an application then it too needs to be updated. Any location in the source code where an indirect branch or function call can land must now have either ENDBR64 (for 64-bit assembler) or ENDBR32 (for 32-bit assembler) as the first instruction executed.
Thanks for the clarification/pointer. In that case, it's a task for the upstreams, indeed.
Looping @hanno-becker @mkannwischer @praveksharma: Does mlkem-native implement these protections?
Looping @hanno-becker @mkannwischer @praveksharma: Does mlkem-native implement these protections?
Not yet. I opened an issue to track: https://github.com/pq-code-package/mlkem-native/pull/762 I have a draft PR https://github.com/pq-code-package/mlkem-native/pull/761, but still need to work around some issues.
Looping @hanno-becker @mkannwischer @praveksharma: Does mlkem-native implement these protections?
Not yet. I opened an issue to track: https://github.com/pq-code-package/mlkem-native/pull/762 I have a draft PR https://github.com/pq-code-package/mlkem-native/pull/761, but still need to work around some issues.
We added it to mlkem-native now. @beldmit could you please have a look and let us know if that works for you.
@mkannwischer We're thinking of doing a liboqs release in mid-March. Would there be a tagged mlkem-native release sometime before then that would include this (and other recent updates) that we could include in our release?
@mkannwischer We're thinking of doing a liboqs release in mid-March. Would there be a tagged mlkem-native release sometime before then that would include this (and other recent updates) that we could include in our release?
Yes, we can certainly create a release soon. When would you need that at the latest? If we do want to include the extended API we will need a few more days.
If we stick to the existing standard FIPS 203 API for the release, we can wrap things up quickly. It depends on whether you want/need the extended API for the next release, really.
When would you need that at the latest?
As stated in https://github.com/open-quantum-safe/liboqs/issues/2028#issuecomment-2684252607 I'm sure we'd rather relax a tentative "mid March" release date than not include valuable additions.
It depends on whether you want/need the extended API for the next release, really.
Question back (and to @SWilson4 ): What does this extended API offer? Is it not required for landing https://github.com/open-quantum-safe/liboqs/pull/2070 , is it?
I used the following command:
LDFLAGS=-Wl,-z,now CFLAGS=-fcf-protection=full cmake -GNinja -DBUILD_SHARED_LIBS=ON -DOQS_USE_AES_OPENSSL=ON -DOQS_USE_AES_INSTRUCTIONS=OFF -DOQS_DIST_BUILD=ON -DOQS_ALGS_ENABLED=NIST -DOQS_USE_SHA3_OPENSSL=ON -DOQS_DLOPEN_OPENSSL=ON -DCMAKE_BUILD_TYPE=Release -LAH ..
and
$ annocheck -f lib/liboqs.so
annocheck: Version 12.69.
Hardened: liboqs.so: FAIL: gaps test because gaps were detected in the annobin coverage
Hardened: liboqs.so: FAIL: cf-protection test because no .note.gnu.property section = no control flow information
Hardened: liboqs.so: FAIL: property-note test because a property note was found but it shows that cf-protection is not enabled
Hardened: Rerun annocheck with --verbose to see more information on the tests.
Hardened: liboqs.so: Overall: FAIL.
Thanks for checking @beldmit -- but as the issue is not resolved, this failure is expected, right? Then, is annocheck a RHEL-only tool or could it also be used as part of a vendor-agnostic CI pipeline (didn't find it by apt search)?
It is a part of Fedora, I believe:
$ yum info annobin-annocheck
Updating and loading repositories:
Name : annobin-annocheck
Epoch : 0
Version : 12.69
Release : 1.fc41
Architecture : x86_64
Installed size : 333.0 KiB
Source : annobin-12.69-1.fc41.src.rpm
From repository : <unknown>
Summary : A tool for checking the security hardening status of binaries
URL : https://sourceware.org/annobin/
License : GPL-3.0-or-later AND LGPL-2.0-or-later AND (GPL-2.0-or-later WITH GCC-exception-2.0) AND (LGPL-2.0-or-later WITH GCC-exception-2.0) AND GFDL-1.3-or-later
Description : Installs the annocheck program which uses the notes generated by annobin to
: check that the specified files were compiled with the correct security
: hardening options.
Vendor : Fedora Project
Even when the updated mlkem-native implementation lands, this check will still likely fail due to assembly files in other algorithms (e.g., McEliece). However, if we could get it to run in CI on a minimal ML-KEM build, that might be cool.
this check will still likely fail due to assembly files in other algorithms (e.g., McEliece)
Not really: Please note the presence of -DOQS_ALGS_ENABLED=NIST above. Hence, the above won't be a problem -- as long as the new CI test also IMO primarily targets (only?) that config option.
Sorry, I missed that only the ML-KEM code is changed, so I can repeat the test if you give me proper build options
Sorry, I missed that only the ML-KEM code is changed, so I can repeat the test if you give me proper build options
FWIW and AFAIK, the ML-KEM code imported to OQS did not yet get changed to add this protection (right @mkannwischer @SWilson4 ?), so I'd recommend you wait with re-test until that has happened @beldmit .
Sorry, I missed that only the ML-KEM code is changed, so I can repeat the test if you give me proper build options
FWIW and AFAIK, the ML-KEM code imported to OQS did not yet get changed to add this protection (right @mkannwischer @SWilson4 ?), so I'd recommend you wait with re-test until that has happened @beldmit .
Yes, we have yet to update in liboqs.
Not really: Please note the presence of
-DOQS_ALGS_ENABLED=NISTabove. Hence, the above won't be a problem -- as long as the new CI test also IMO primarily targets (only?) that config option.
Ah, I missed that—though I don't think we actually have a "NIST" option for alg selection (the documented ones are STD, NIST_R4, NIST_ON_RAMP), so perhaps this config is building all algs anyway. Even if it's restricted to standardized algs, ML-DSA will still have assembly files that don't implement the protection.
Not really: Please note the presence of
-DOQS_ALGS_ENABLED=NISTabove. Hence, the above won't be a problem -- as long as the new CI test also IMO primarily targets (only?) that config option.Ah, I missed that—though I don't think we actually have a "NIST" option for alg selection (the documented ones are STD, NIST_R4, NIST_ON_RAMP), so perhaps this config is building all algs anyway. Even if it's restricted to standardized algs, ML-DSA will still have assembly files that don't implement the protection.
True, mldsa currently still has the old/less-maintained provenance. And the config setting indeed seems worth while checking out: What are you really using for Fedora/RHEL @beldmit ?
I tried to duplicate this test now that the mlkem-native implementation has been updated. I observed something weird in the process:
- After the initial configure and build commands, the test fails.
- If the configure and build commands are repeated (without cleaning out the build directory) the test passes.
The behaviour is the same on both 0.12.0 (before the ML-KEM code was updated to include cf protection) and the latest main (which includes ML-KEM code with cf protection).
This seems to indicate a bug in either our build system or annocheck. Tagging @beldmit for more insight.
Here's my testing setup in case anyone else wants to duplicate my results:
Directory structure
tree
.
├── check_cf.sh
└── Dockerfile
1 directory, 2 files
Dockerfile
FROM fedora:latest
RUN dnf install -y annocheck cmake git gcc ninja openssl-devel
COPY check_cf.sh /root
WORKDIR /root
RUN git clone https://github.com/open-quantum-safe/liboqs.git liboqs
ENTRYPOINT ["./check_cf.sh"]
check_cf.sh
#!/bin/sh
build_liboqs() {
LDFLAGS=-Wl,-z,now CFLAGS=-fcf-protection=full cmake -GNinja -DBUILD_SHARED_LIBS=ON -DOQS_USE_AES_OPENSSL=ON -DOQS_USE_AES_INSTRUCTIONS=OFF -DOQS_DIST_BUILD=ON -DOQS_USE_SHA3_OPENSSL=ON -DOQS_DLOPEN_OPENSSL=ON -DCMAKE_BUILD_TYPE=Release -DOQS_MINIMAL_BUILD="KEM_ml_kem_512" ..
ninja
}
cd liboqs
git checkout a20597c # current main
git clean -xdf
mkdir build && cd build
build_liboqs
annocheck -f --skip-gaps --verbose lib/liboqs.so
build_liboqs
annocheck -f --skip-gaps --verbose lib/liboqs.so
cd ..
git checkout 0.12.0
git clean -xdf
mkdir build && cd build
build_liboqs
annocheck -f --skip-gaps --verbose lib/liboqs.so
build_liboqs
annocheck -f --skip-gaps --verbose lib/liboqs.so
To execute:
docker build -t check-cf .
docker run --rm check-cf