Fix #1 #2

oddcoder · 2024-08-19T10:45:28Z

This is patch series that fixes the unsoundness bug at #1

trent-reed · 2024-08-19T17:19:32Z

safe-discriminant-derive/src/lib.rs

+    attrs
+        .iter()
+        .filter(|attr| !attr.path().is_ident("repr"))
+        .count()


Nit: Probably any() is a better use case for this: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.any

attrs.iter().any(|attr| !attr.path().is_ident("repr"))

Side-Note: I actually didn't know that the derive macro can't see the derive attributes. TIL.

Side-Note: I actually didn't know that the derive macro can't see the derive attributes. TIL.

Apparently i is a bit tricky, they cannot see derive attribute, and (I think) they cannot see attributed defined before them if they did not persist after the attribute is being called.

trent-reed · 2024-08-19T17:22:37Z

safe-discriminant-derive/src/lib.rs

+    if contains_attribute_macros(&tagged_enum.attrs) {
+        return Err(Error::new(
+            tagged_enum.ident.span(),
+            "Discriminant is not compatiable with any top \


Nit: I'm still not totally sure how I feel about these \ continuations here. It seems like that will create a string like this: "Discriminant is not compatiable with any top (lots of spaces here) level #[attr] except #[repr(_)]."

Maybe the compiler error is reformatted so the end user can't see this, so probably not a huge deal, but concat! is how I thought you were supposed to do string literals across multiple lines: https://doc.rust-lang.org/std/macro.concat.html

They do the same, I am used to the \ notation because it is similar to C, but we can standardize using concat!

trent-reed · 2024-08-19T17:25:16Z

safe-discriminant-derive/src/lib.rs

@@ -107,3 +124,26 @@ pub fn derive_discriminant(item: TokenStream) -> TokenStream {
        Ok(s) => s,
    }
 }
+
+#[cfg(feature = "test-utils")]


Optional/Nit: Slight preference to put this in a separate module and just #[cfg(feature = "test-utils")] the module instead of doing it per testonly attribute.

I think it'd read a lot clearer in tests too:

use safe_discriminant::Discriminant; use safe_discriminant_derive::testonly::repr; #[derive(Discriminant)] #[repr(u8)] pub enum Foo { A = 0, B = 1, } fn main() {}

That is unfortunately no possible, all proc macros must be defined in the top level lib.rs.

trent-reed · 2024-08-19T17:30:41Z

safe-discriminant/tests/pass/remove_repr.rs

+// this test makes sure that remove repr actually removes repr
+
+use safe_discriminant_derive::remove_repr;
+


Optional/Nit: Some extra spacing and strange formatting in the file.

righ, I forgot that cargo format doesn't not format those files, will format with rustfmt

trent-reed · 2024-08-19T17:36:45Z

safe-discriminant-derive/src/lib.rs

@@ -80,13 +80,30 @@ fn validate_all_variants(variants: impl Iterator<Item = Variant>) -> Result<()>
        .unwrap_or(Ok(()))
 }

+/// Returns true if there is any #[x] where x is not `repr`
+/// returns false otherwise.
+fn contains_attribute_macros(attrs: &[Attribute]) -> bool {


I think if we want to get super technical, what we really want to say is that there are no attributes past the derive macro that is implementing the Discriminant trait.

I actually think that's what this code accomplishes, since I think that the other attribute macros will be consumed in-order, so I don't think you can see the attributes after they've taken effect. But the chat logs that you pointed to seem to suggest otherwise and that you can see other attribute macros after they've been expanded. Am I reading this right?

Anyways, consider this an optional request, but it might be nice to try to create an attribute macro that isn't malicious and does nothing and see if it gets compiled out so that we don't see it as long as it's positioned before the derive attribute. And if so, we can test a non-malicious attribute macro to see that it produces what we expect (an error if placed after the derive attribute, success if placed before).

I'm not totally sure what would happen, but maybe worth playing with.

safe-discriminant-derive/src/lib.rs

trent-reed

Very great catch to the reporter - I'm not used to thinking about attribute macros, so that's definitely a blind spot for me. Sorry about missing it.

Aside from some nits and possible other tests, it looks good to me. Thanks so much for the diligence here!

oddcoder · 2024-08-19T18:16:53Z

I think I found another soundness bug \o/, It follows the same theme!

#[derive(Discriminant)]
#[repr(u8)]
pub enum Foo {
#[remove_disc]
    A = 0,
    B = 1,
}

if #[remove_disc] replaces A = 0 with A, that would break our second safety condition. Will have to prototype it first.

This macro removes `#[repr(_)]` from an enum, this is useful for testing the bug described in #1. `#[remove_repr]` is only available behind the feature flag test-utils. Signed-off-by: Ahmed Abdelraoof <[email protected]>

This patch adds a fake `repr` and add tests to try to confuse the compiler into thinking that it is the real repr. If possible, that would cause problems with safe-discriminant. That is because we assume that there is always a repr attributed. Signed-off-by: Ahmed Abdelraoof <[email protected]>

This patch checks if there is any top level attribute macro expansion, and if there it, we report it as an error. fixes: #1 Signed-off-by: Ahmed Abdelraoof <[email protected]>

Signed-off-by: Ahmed Abdelraoof <[email protected]>

oddcoder · 2024-08-19T19:33:38Z

Very great catch to the reporter - I'm not used to thinking about attribute macros, so that's definitely a blind spot for me. Sorry about missing it.

No need to be sorry, We learned a new trick for rust unsafe, something tells me many library that does AST level checks in macros will have the same kind of bugs, maybe including zero-copy, but I will check later.

I addressed all you comments, I will merge this PR, to keep it short, and start a new fix for the second soundness bug I discovered, I guess the lesson (at least for me) learnt here is that there is no such thing as too simple or too obvious unsafe {..} code.

oddcoder mentioned this pull request Aug 19, 2024

Move unsafe discriminant calculation outside tpm-rs tpm-rs/tpm-rs#93

Merged

oddcoder force-pushed the soundness_bug branch from a4e9d97 to 3e7374a Compare August 19, 2024 15:22

trent-reed reviewed Aug 19, 2024

View reviewed changes

safe-discriminant-derive/src/lib.rs Show resolved Hide resolved

trent-reed approved these changes Aug 19, 2024

View reviewed changes

oddcoder added 5 commits August 19, 2024 20:42

Add remove_repr macro for testing

56afb4a

This macro removes `#[repr(_)]` from an enum, this is useful for testing the bug described in #1. `#[remove_repr]` is only available behind the feature flag test-utils. Signed-off-by: Ahmed Abdelraoof <[email protected]>

Reject enums with attribute macros

7d84cb2

This patch checks if there is any top level attribute macro expansion, and if there it, we report it as an error. fixes: #1 Signed-off-by: Ahmed Abdelraoof <[email protected]>

cargo fmt

3f6254d

test marcro ordering

93a14c1

Signed-off-by: Ahmed Abdelraoof <[email protected]>

oddcoder force-pushed the soundness_bug branch from 3e7374a to 93a14c1 Compare August 19, 2024 19:28

oddcoder merged commit 4b23fc3 into main Aug 19, 2024
4 checks passed

oddcoder deleted the soundness_bug branch August 19, 2024 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1 #2

Fix #1 #2

oddcoder commented Aug 19, 2024 •

edited

Loading

trent-reed Aug 19, 2024

oddcoder Aug 19, 2024

trent-reed Aug 19, 2024 •

edited

Loading

oddcoder Aug 19, 2024

trent-reed Aug 19, 2024

oddcoder Aug 19, 2024

trent-reed Aug 19, 2024

oddcoder Aug 19, 2024

trent-reed Aug 19, 2024

trent-reed left a comment

oddcoder commented Aug 19, 2024 •

edited

Loading

oddcoder commented Aug 19, 2024

		// this test makes sure that remove repr actually removes repr

		use safe_discriminant_derive::remove_repr;

Fix #1 #2

Fix #1 #2

Conversation

oddcoder commented Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trent-reed Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trent-reed left a comment

Choose a reason for hiding this comment

oddcoder commented Aug 19, 2024 • edited Loading

oddcoder commented Aug 19, 2024

oddcoder commented Aug 19, 2024 •

edited

Loading

trent-reed Aug 19, 2024 •

edited

Loading

oddcoder commented Aug 19, 2024 •

edited

Loading