Skip to content

Security patch: fix decapsulation bug in HQC#578

Merged
thomwiggers merged 2 commits intomasterfrom
sw-hqc-patch
Dec 11, 2024
Merged

Security patch: fix decapsulation bug in HQC#578
thomwiggers merged 2 commits intomasterfrom
sw-hqc-patch

Conversation

@SWilson4
Copy link
Copy Markdown
Collaborator

@SWilson4 SWilson4 commented Dec 10, 2024

In liboqs we have recently patched HQC code to avoid a correctness error in decapsulation. This PR backports the changes to PQClean. Please see GHSA-gpf4-vrrw-r8v7 for details, including a CVE assigned by GitHub.

Sorry about all the whitespace changes... CI passes, so I assume that is the desired formatting? To make review easier, here is the output of git diff --ignore-blank-lines master.

diff --git a/crypto_kem/hqc-128/META.yml b/crypto_kem/hqc-128/META.yml
index 7cca1eac..553fbbb6 100644
--- a/crypto_kem/hqc-128/META.yml
+++ b/crypto_kem/hqc-128/META.yml
@@ -25,4 +25,4 @@ principal-submitters:
   - Gilles Zémor
 implementations:
     - name: clean
-      version: hqc-submission_2023-04-30 via https://github.com/SWilson4/package-pqclean/tree/8db1b24b/hqc
+      version: hqc-submission_2023-04-30 via https://github.com/SWilson4/package-pqclean/tree/9b509aa7/hqc
diff --git a/crypto_kem/hqc-128/clean/kem.c b/crypto_kem/hqc-128/clean/kem.c
index ad09b35a..e0a4681f 100644
--- a/crypto_kem/hqc-128/clean/kem.c
+++ b/crypto_kem/hqc-128/clean/kem.c
@@ -87,7 +94,7 @@ int PQCLEAN_HQC128_CLEAN_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const ui
     uint8_t result;
     uint64_t u[VEC_N_SIZE_64] = {0};
     uint64_t v[VEC_N1N2_SIZE_64] = {0};
-    const uint8_t *pk = sk + SEED_BYTES;
+    const uint8_t *pk = sk + SEED_BYTES + VEC_K_SIZE_BYTES;
     uint8_t sigma[VEC_K_SIZE_BYTES] = {0};
     uint8_t theta[SHAKE256_512_BYTES] = {0};
     uint64_t u2[VEC_N_SIZE_64] = {0};
@@ -115,7 +122,7 @@ int PQCLEAN_HQC128_CLEAN_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const ui
     result |= PQCLEAN_HQC128_CLEAN_vect_compare((uint8_t *)u, (uint8_t *)u2, VEC_N_SIZE_BYTES);
     result |= PQCLEAN_HQC128_CLEAN_vect_compare((uint8_t *)v, (uint8_t *)v2, VEC_N1N2_SIZE_BYTES);
 
-    result = (uint8_t) (-((int16_t) result) >> 15);
+    result -= 1;
 
     for (size_t i = 0; i < VEC_K_SIZE_BYTES; ++i) {
         mc[i] = (m[i] & result) ^ (sigma[i] & ~result);
@@ -126,5 +133,6 @@ int PQCLEAN_HQC128_CLEAN_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const ui
     PQCLEAN_HQC128_CLEAN_store8_arr(mc + VEC_K_SIZE_BYTES + VEC_N_SIZE_BYTES, VEC_N1N2_SIZE_BYTES, v, VEC_N1N2_SIZE_64);
     PQCLEAN_HQC128_CLEAN_shake256_512_ds(&shake256state, ss, mc, VEC_K_SIZE_BYTES + VEC_N_SIZE_BYTES + VEC_N1N2_SIZE_BYTES, K_FCT_DOMAIN);
 
-    return -(~result & 1);
+
+    return (result & 1) - 1;
 }
diff --git a/crypto_kem/hqc-192/META.yml b/crypto_kem/hqc-192/META.yml
index 2a1962e6..20730767 100644
--- a/crypto_kem/hqc-192/META.yml
+++ b/crypto_kem/hqc-192/META.yml
@@ -25,4 +25,4 @@ principal-submitters:
   - Gilles Zémor
 implementations:
     - name: clean
-      version: hqc-submission_2023-04-30 via https://github.com/SWilson4/package-pqclean/tree/8db1b24b/hqc
+      version: hqc-submission_2023-04-30 via https://github.com/SWilson4/package-pqclean/tree/9b509aa7/hqc
diff --git a/crypto_kem/hqc-192/clean/kem.c b/crypto_kem/hqc-192/clean/kem.c
index f611ebb1..56be3114 100644
--- a/crypto_kem/hqc-192/clean/kem.c
+++ b/crypto_kem/hqc-192/clean/kem.c
@@ -87,7 +94,7 @@ int PQCLEAN_HQC192_CLEAN_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const ui
     uint8_t result;
     uint64_t u[VEC_N_SIZE_64] = {0};
     uint64_t v[VEC_N1N2_SIZE_64] = {0};
-    const uint8_t *pk = sk + SEED_BYTES;
+    const uint8_t *pk = sk + SEED_BYTES + VEC_K_SIZE_BYTES;
     uint8_t sigma[VEC_K_SIZE_BYTES] = {0};
     uint8_t theta[SHAKE256_512_BYTES] = {0};
     uint64_t u2[VEC_N_SIZE_64] = {0};
@@ -115,7 +122,7 @@ int PQCLEAN_HQC192_CLEAN_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const ui
     result |= PQCLEAN_HQC192_CLEAN_vect_compare((uint8_t *)u, (uint8_t *)u2, VEC_N_SIZE_BYTES);
     result |= PQCLEAN_HQC192_CLEAN_vect_compare((uint8_t *)v, (uint8_t *)v2, VEC_N1N2_SIZE_BYTES);
 
-    result = (uint8_t) (-((int16_t) result) >> 15);
+    result -= 1;
 
     for (size_t i = 0; i < VEC_K_SIZE_BYTES; ++i) {
         mc[i] = (m[i] & result) ^ (sigma[i] & ~result);
@@ -126,5 +133,6 @@ int PQCLEAN_HQC192_CLEAN_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const ui
     PQCLEAN_HQC192_CLEAN_store8_arr(mc + VEC_K_SIZE_BYTES + VEC_N_SIZE_BYTES, VEC_N1N2_SIZE_BYTES, v, VEC_N1N2_SIZE_64);
     PQCLEAN_HQC192_CLEAN_shake256_512_ds(&shake256state, ss, mc, VEC_K_SIZE_BYTES + VEC_N_SIZE_BYTES + VEC_N1N2_SIZE_BYTES, K_FCT_DOMAIN);
 
-    return -(~result & 1);
+
+    return (result & 1) - 1;
 }
diff --git a/crypto_kem/hqc-256/META.yml b/crypto_kem/hqc-256/META.yml
index b9707b74..93db4367 100644
--- a/crypto_kem/hqc-256/META.yml
+++ b/crypto_kem/hqc-256/META.yml
@@ -25,4 +25,4 @@ principal-submitters:
   - Gilles Zémor
 implementations:
     - name: clean
-      version: hqc-submission_2023-04-30 via https://github.com/SWilson4/package-pqclean/tree/8db1b24b/hqc
+      version: hqc-submission_2023-04-30 via https://github.com/SWilson4/package-pqclean/tree/9b509aa7/hqc
diff --git a/crypto_kem/hqc-256/clean/kem.c b/crypto_kem/hqc-256/clean/kem.c
index 4e47e87d..2929ba08 100644
--- a/crypto_kem/hqc-256/clean/kem.c
+++ b/crypto_kem/hqc-256/clean/kem.c
@@ -87,7 +94,7 @@ int PQCLEAN_HQC256_CLEAN_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const ui
     uint8_t result;
     uint64_t u[VEC_N_SIZE_64] = {0};
     uint64_t v[VEC_N1N2_SIZE_64] = {0};
-    const uint8_t *pk = sk + SEED_BYTES;
+    const uint8_t *pk = sk + SEED_BYTES + VEC_K_SIZE_BYTES;
     uint8_t sigma[VEC_K_SIZE_BYTES] = {0};
     uint8_t theta[SHAKE256_512_BYTES] = {0};
     uint64_t u2[VEC_N_SIZE_64] = {0};
@@ -115,7 +122,7 @@ int PQCLEAN_HQC256_CLEAN_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const ui
     result |= PQCLEAN_HQC256_CLEAN_vect_compare((uint8_t *)u, (uint8_t *)u2, VEC_N_SIZE_BYTES);
     result |= PQCLEAN_HQC256_CLEAN_vect_compare((uint8_t *)v, (uint8_t *)v2, VEC_N1N2_SIZE_BYTES);
 
-    result = (uint8_t) (-((int16_t) result) >> 15);
+    result -= 1;
 
     for (size_t i = 0; i < VEC_K_SIZE_BYTES; ++i) {
         mc[i] = (m[i] & result) ^ (sigma[i] & ~result);
@@ -126,5 +133,6 @@ int PQCLEAN_HQC256_CLEAN_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const ui
     PQCLEAN_HQC256_CLEAN_store8_arr(mc + VEC_K_SIZE_BYTES + VEC_N_SIZE_BYTES, VEC_N1N2_SIZE_BYTES, v, VEC_N1N2_SIZE_64);
     PQCLEAN_HQC256_CLEAN_shake256_512_ds(&shake256state, ss, mc, VEC_K_SIZE_BYTES + VEC_N_SIZE_BYTES + VEC_N1N2_SIZE_BYTES, K_FCT_DOMAIN);
 
-    return -(~result & 1);
+
+    return (result & 1) - 1;
 }
diff --git a/test/duplicate_consistency/hqc-128_clean.yml b/test/duplicate_consistency/hqc-128_clean.yml
index be038690..0870de5a 100644
--- a/test/duplicate_consistency/hqc-128_clean.yml
+++ b/test/duplicate_consistency/hqc-128_clean.yml
@@ -19,6 +19,7 @@ consistency_checks:
       - gf.c
       - gf2x.c
       - hqc.c
+      - kem.c
       - parsing.c
       - reed_muller.c
       - reed_solomon.c
diff --git a/test/duplicate_consistency/hqc-192_clean.yml b/test/duplicate_consistency/hqc-192_clean.yml
index 26828ff6..035ec9f2 100644
--- a/test/duplicate_consistency/hqc-192_clean.yml
+++ b/test/duplicate_consistency/hqc-192_clean.yml
@@ -19,6 +19,7 @@ consistency_checks:
       - gf.c
       - gf2x.c
       - hqc.c
+      - kem.c
       - parsing.c
       - reed_muller.c
       - reed_solomon.c

dstebila
dstebila previously approved these changes Dec 10, 2024
View reviewed changes
@mkannwischer
Copy link
Copy Markdown
Contributor

mkannwischer commented Dec 11, 2024

Thanks! Can you add this to the https://github.com/PQClean/PQClean/blob/master/SECURITY.md, please?

@thomwiggers
Copy link
Copy Markdown
Member

thomwiggers commented Dec 11, 2024

I created a security advisory here GHSA-753p-wrj5-g8fj

@dstebila you might want to add oqs-rust to the OQS security advisory as an affected package.

@dstebila
Copy link
Copy Markdown
Member

dstebila commented Dec 11, 2024

@dstebila you might want to add oqs-rust to the OQS security advisory as an affected package.

Good suggestion. I guess I would tag both the oqs and oqs-sys crates in the advisory? And then as for what version is affect and what version fixes it, do we actually need to make a new release of the Rust crates? Or do they pick up the most recent version of liboqs automatically? I don't fully understand how the versioning works here.

@SWilson4
Copy link
Copy Markdown
Collaborator Author

SWilson4 commented Dec 11, 2024

Thanks! Can you add this to the https://github.com/PQClean/PQClean/blob/master/SECURITY.md, please?

Done! Although it looks like the new commit automatically dismissed @dstebila's review.

@SWilson4 SWilson4 requested a review from dstebila December 11, 2024 15:39
@thomwiggers thomwiggers merged commit 1eacfda into master Dec 11, 2024
@thomwiggers thomwiggers deleted the sw-hqc-patch branch December 11, 2024 15:46
@thomwiggers
Copy link
Copy Markdown
Member

thomwiggers commented Dec 11, 2024

Thanks!

tniessen added a commit to tniessen/node-pqclean that referenced this pull request Dec 12, 2024
@tniessen
Update PQClean to 1eacfda
b83750b 
Refs: PQClean/PQClean#578
@SWilson4 SWilson4 mentioned this pull request Dec 19, 2024
Merged
2 tasks
data-wardenb6ym added a commit to data-wardenb6ym/node-pqclean that referenced this pull request Sep 29, 2025
@data-wardenb6ym
Update PQClean to 1eacfda
3c37dd0 
Refs: PQClean/PQClean#578
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomwiggers thomwiggers thomwiggers approved these changes

@mkannwischer mkannwischer Awaiting requested review from mkannwischer

@dstebila dstebila Awaiting requested review from dstebila

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SWilson4 @mkannwischer @thomwiggers @dstebila