Skip to content

Conversation

@karthikbhargavan
Copy link
Collaborator

This PR addresses [...] described in issue #.

Type of change

  • [X ] New feature

Motivation and Context

This PR adds panic freedom proofs (and relevant annotations) to the core protocol code.

Changes

TODO

Checklist

  • I have linked an issue to this PR
  • I have described the changes
  • I have read and understood the code of conduct, contribution guidelines, and contributor license agreement
  • I have added tests for all changes
  • I have tested that all tests pass locally and all checks pass
  • I have updated documentation if necessary

Fixes #

Cargo.toml Outdated
]}
hax-lib-macros = { git = "https://github.com/hacspec/hax", optional = true }
hax-lib = { git = "https://github.com/hacspec/hax" }
hax-lib = { git = "https://github.com/hacspec/hax/", branch= "pq11-fstar-libs" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remember to update this before merging.

src/tls13cert.rs Outdated
let mut u32word = [U8(0); 4];
u32word[0..len].copy_from_slice(&b[offset..offset + len]);
Ok(u32_from_be_bytes(u32word).declassify() as usize >> ((4 - len) * 8))
if b.len() >= offset + len {
Copy link
Member

@franziskuskiefer franziskuskiefer Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is how it's used. This requires the caller to pass in only the exact slice that's read. I don't think that's how this should be defined.

if b[offset].declassify() & 0x80u8 == 0u8 {
Ok((b[offset].declassify() & 0x7fu8) as usize)
} else {
if b.len() > offset {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we can use early returns. We should that in checks like these.

@karthikbhargavan
Copy link
Collaborator Author

Made good progress on this.
Things are panic-free up to some hax bugs and some laxed functions that still need work.

@karthikbhargavan
Copy link
Collaborator Author

Not all functions are yet panic-free, but enough passes that we can put this on CI and make improvements.

@karthikbhargavan karthikbhargavan marked this pull request as ready for review March 11, 2025 11:04
@karthikbhargavan karthikbhargavan requested a review from a team as a code owner March 11, 2025 11:04
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. lgtm with a few nits. And the CI needs to be made happy.

For that we also need to add the hax config to cargo to get rid of that annoying warning.

#[cfg(feature = "hax-pv")]
use hax_lib::{pv_constructor, pv_handwritten};

use hax_lib::ToInt;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import

@@ -0,0 +1,211 @@
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file probably shouldn't be here.

Result::Ok(l) => b.len() > offset && l <= 255,
_ => true })]
fn length_length(b: &Bytes, offset: usize) -> Result<usize, Asn1Error> {
if b.len() > offset {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have early returns now, using that to error out early would make all this code much more readable. Doesn't have to happen in this PR though.

@jschneider-bensch
Copy link
Contributor

Closing in favor of #148 which contains all these changes.

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

Successfully merging this pull request may close these issues.

3 participants