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

Add CBMC proof for s2n_stuffer_printf #4309

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Dec 3, 2023

Resolved issues:

Resolves #4298 (comment)

Description of changes:

Add a proof for the new s2n_stuffer_printf method.

I also changed the existing s2n_stuffer_wipe_n behavior and rewrote the associated proof. My s2n_stuffer_printf tracked the tainted flag closely and found some unexpected behavior in how s2n_stuffer_wipe_n handled the tainted flag. Previously, if s2n_stuffer_wipe_n resulted in wiping the last few bytes of a stuffer, the stuffer was also wiped-- specifically, the high_water_mark and tainted fields. This makes s2n_stuffer_wipe_n hard to reason about, and seems like a potential way to accidentally "untaint" a stuffer. wipe_n is about wiping individual bytes, not wiping the whole stuffer. I don't know why you'd have an outstanding raw pointer to a stuffer with write_cursor==0, but the memory still exists so you could, and it doesn't seem any different from having an outstanding raw pointer to wiped memory with write_cursor>0.

Testing:

Proofs pass.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Dec 3, 2023
@lrstewart lrstewart marked this pull request as ready for review December 4, 2023 18:29
Copy link
Contributor

@camshaft camshaft left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@lrstewart lrstewart enabled auto-merge (squash) December 15, 2023 20:42
Copy link
Contributor

@feliperodri feliperodri left a comment

Choose a reason for hiding this comment

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

These updates are great! I'm quite happy to see such improvements over the CBMC harnesses. I only have a few remarks. First, I got this list of warning from the CBMC HTML report:

 Warnings

Functions omitted from test (unexpected):

    madvise
    s2n_blob_slice
    s2n_stuffer_wipe
    vsnprintf

Are these expected?

@feliperodri
Copy link
Contributor

The function madvise is reachable in these harness, so I recommend you to include the stub we created for it. You only need to include it in your Makefile.

PROOF_SOURCES += $(PROOF_STUB)/madvise.c

@lrstewart
Copy link
Contributor Author

These updates are great! I'm quite happy to see such improvements over the CBMC harnesses. I only have a few remarks. First, I got this list of warning from the CBMC HTML report:

 Warnings

Functions omitted from test (unexpected):

    madvise
    s2n_blob_slice
    s2n_stuffer_wipe
    vsnprintf

Are these expected?

You called out how to fix madvise in your other comment, and I fixed that. I thought I was handling s2n_blob_slice and s2n_stuffer_wipe by using REMOVE_BODY, but I guess not so I removed that. They're never hit because the stuffer never shrinks.

vsnprintf is the only one on the list now, and I think that's expected. We could stub it, but idk what we'd do other than the current default "assume it could return anything" behavior.

@lrstewart lrstewart merged commit ea62545 into aws:main Dec 18, 2023
28 checks passed
@lrstewart lrstewart deleted the fips_comp_1b branch December 19, 2023 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants