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

Packet packing problem with MLKEM post-quantum support #524

Open
psheer-spirent opened this issue Dec 23, 2024 · 3 comments
Open

Packet packing problem with MLKEM post-quantum support #524

psheer-spirent opened this issue Dec 23, 2024 · 3 comments

Comments

@psheer-spirent
Copy link
Contributor

For MLKEM the key sizes are so large that I get failure. I believe this patch here is a fix for this bug. This change ensures that avail will never be negative. I am still experimenting if this patch is proper, but hoping an experienced lsquic developer can comment if I spotted this correctly.

Thanks.

--- a/src/liblsquic/lsquic_mini_conn_ietf.c
+++ b/src/liblsquic/lsquic_mini_conn_ietf.c
@@ -308,12 +308,14 @@ imico_stream_write (void *stream, const void *bufp, size_t bufsz)
         if (!packet_out)
             return -1;
         // NOTE: reduce the size of first crypto frame to combine packets
+        // NOTE: except in the case of post-quantum keys which are huge
         int avail = lsquic_packet_out_avail(packet_out);
         if (cryst->mcs_enc_level == ENC_LEV_HSK
             && cryst->mcs_write_off == 0
             && avail > conn->imc_hello_pkt_remain - conn->imc_long_header_sz)
         {
-            avail = conn->imc_hello_pkt_remain - conn->imc_long_header_sz;
+            if (conn->imc_hello_pkt_remain > conn->imc_long_header_sz)  // post-quantum KEM's fail this condition
+                avail = conn->imc_hello_pkt_remain - conn->imc_long_header_sz;
         }
         p = msg_ctx.buf;
         len = pf->pf_gen_crypto_frame(packet_out->po_data + packet_out->po_data_sz,
@litespeedtech
Copy link
Owner

Maybe combine the new if(...) with the existing one?

@psheer-spirent
Copy link
Contributor Author

Maybe combine the new if(...) with the existing one?

Would you be so kind to answer if I have indeed found a bug and if my logic properly fixes it? (We ship commercial software based on lsquic.)

Kind regards,

@litespeedtech
Copy link
Owner

Yes, it is indeed a bug, your fix should cover the case.

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