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 support for SHA3 #327

Merged
merged 21 commits into from
Mar 28, 2023
Merged

Add support for SHA3 #327

merged 21 commits into from
Mar 28, 2023

Conversation

brycx
Copy link
Member

@brycx brycx commented Mar 16, 2023

TODO:

  • Panic-section of docs that says N bits of data
  • Should the SHA3 structs be named Sha3_size? seems that this is kind of how it is referred to elsewhere
  • Add cryptofuzz test found in Zig implementation (Streaming SHA3 incorrect output ziglang/zig#14851)

@brycx brycx added the new feature New feature or request label Mar 16, 2023
@brycx brycx added this to the 0.17.5 milestone Mar 16, 2023
Copy link
Contributor

@vlmutolo vlmutolo left a comment

Choose a reason for hiding this comment

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

Generally looks good. Just had a question about method naming / privacy. There are some functions marked with an _ and I wasn't sure if that was to indicate that they were supposed to be private methods or to silence dead-code warnings.

src/hazardous/hash/sha3/mod.rs Show resolved Hide resolved
src/hazardous/hash/sha3/sha256.rs Outdated Show resolved Hide resolved
@brycx
Copy link
Member Author

brycx commented Mar 20, 2023

Answering out of thread here, because I think there may be more to this. I hadn't thought about _ prefix also indicating dead-code warnings, this is of course not optimal, because all methods named so here are definitely intended to be used.

The way this was set up was to keep it consistent with the SHA2 implementation. This morning I was thinking, that this may not be the best approach after all. For SHA2, there's different logic between the variants, where the only thing changing for the variants in SHA3 is the rate.

Perhaps it would make sense to use an approach like that for BLAKE2b instead. We'd have one Sha3 struct, that takes in the rate as const-parameter, where we could make type definitions for those so users pass in Sha3::<SHA3_256>new() instead of having separate structs. Then the Digest could cover the range of possible output sizes, like with BLAKE2b. Lastly, we could provide a similar enum, as BLAKE2b, if that makes sense for faster initialization. It don't think an enum would make things simpler here though.

So deleting the individual structs and simply keeping one Sha3. Would do you think about this @vlmutolo ?

This would however make an API in the library that is somewhat inconsistent with the rest, at least in terms of using consts.

@vlmutolo
Copy link
Contributor

vlmutolo commented Mar 20, 2023

As far as I can tell, the _* methods are all internal. Is that correct?

If they're all internal, maybe we just move forward with this PR and discuss changing the public API in a separate commit. If there are differences between the various hashing APIs, I think that's a good opportunity for improvement.

@brycx
Copy link
Member Author

brycx commented Mar 20, 2023

Yes, that's correct.

Sure, it's just a question of whether this matches BLAKE2b or SHA2 API essentially.

@vlmutolo
Copy link
Contributor

I have to take a deeper look at why there are API differences in the first place. My hope would be that there's a way to unify all three, but there's probably something I don't understand yet that makes it hard to unify all of them. But that should probably be in a separate PR anyway.

@brycx
Copy link
Member Author

brycx commented Mar 21, 2023

@vlmutolo I have started a discussion about the differences which hopefully clarifies them: #329

@brycx
Copy link
Member Author

brycx commented Mar 21, 2023

It seems most other libraries I've glanced at take the naming of Sha3_variant for these functions. I'll change these to this and then merge (don't want to confuse users).

@brycx
Copy link
Member Author

brycx commented Mar 22, 2023

Let's also add the issue found in Zig using cryptofuzz (linked in TODO).

    #[test]
    fn test_zig_cryptofuzz() {
        let expected_hash: [u8; 32] = [0x57, 0x80, 0x4, 0x8d, 0xfa, 0x38, 0x1a, 0x1d, 0x1, 0xc7, 0x47, 0x90, 0x6e, 0x4a, 0x8, 0x71, 0x1d, 0xd3, 0x4f, 0xd7, 0x12, 0xec, 0xd7, 0xc6, 0x80, 0x1d, 0xd2, 0xb3, 0x8f, 0xd8, 0x1a, 0x89];
        let msg: [u8; 613] = [0x97, 0xd1, 0x2d, 0x1a, 0x16, 0x2d, 0x36, 0x4d, 0x20, 0x62, 0x19, 0x0b, 0x14, 0x93, 0xbb, 0xf8,
            0x5b, 0xea, 0x04, 0xc2, 0x61, 0x8e, 0xd6, 0x08, 0x81, 0xa1, 0x1d, 0x73, 0x27, 0x48, 0xbf, 0xa4,
            0xba, 0xb1, 0x9a, 0x48, 0x9c, 0xf9, 0x9b, 0xff, 0x34, 0x48, 0xa9, 0x75, 0xea, 0xc8, 0xa3, 0x48,
            0x24, 0x9d, 0x75, 0x27, 0x48, 0xec, 0x03, 0xb0, 0xbb, 0xdf, 0x33, 0x90, 0xe3, 0x93, 0xed, 0x68,
            0x24, 0x39, 0x12, 0xdf, 0xea, 0xee, 0x8c, 0x9f, 0x96, 0xde, 0x42, 0x46, 0x8c, 0x2b, 0x17, 0x83,
            0x36, 0xfb, 0xf4, 0xf7, 0xff, 0x79, 0xb9, 0x45, 0x41, 0xc9, 0x56, 0x1a, 0x6b, 0x0c, 0xa4, 0x1a,
            0xdd, 0x6b, 0x95, 0xe8, 0x03, 0x0f, 0x09, 0x29, 0x40, 0x1b, 0xea, 0x87, 0xfa, 0xb9, 0x18, 0xa9,
            0x95, 0x07, 0x7c, 0x2f, 0x7c, 0x33, 0xfb, 0xc5, 0x11, 0x5e, 0x81, 0x0e, 0xbc, 0xae, 0xec, 0xb3,
            0xe1, 0x4a, 0x26, 0x56, 0xe8, 0x5b, 0x11, 0x9d, 0x37, 0x06, 0x9b, 0x34, 0x31, 0x6e, 0xa3, 0xba,
            0x41, 0xbc, 0x11, 0xd8, 0xc5, 0x15, 0xc9, 0x30, 0x2c, 0x9b, 0xb6, 0x71, 0xd8, 0x7c, 0xbc, 0x38,
            0x2f, 0xd5, 0xbd, 0x30, 0x96, 0xd4, 0xa3, 0x00, 0x77, 0x9d, 0x55, 0x4a, 0x33, 0x53, 0xb6, 0xb3,
            0x35, 0x1b, 0xae, 0xe5, 0xdc, 0x22, 0x23, 0x85, 0x95, 0x88, 0xf9, 0x3b, 0xbf, 0x74, 0x13, 0xaa,
            0xcb, 0x0a, 0x60, 0x79, 0x13, 0x79, 0xc0, 0x4a, 0x02, 0xdb, 0x1c, 0xc9, 0xff, 0x60, 0x57, 0x9a,
            0x70, 0x28, 0x58, 0x60, 0xbc, 0x57, 0x07, 0xc7, 0x47, 0x1a, 0x45, 0x71, 0x76, 0x94, 0xfb, 0x05,
            0xad, 0xec, 0x12, 0x29, 0x5a, 0x44, 0x6a, 0x81, 0xd9, 0xc6, 0xf0, 0xb6, 0x9b, 0x97, 0x83, 0x69,
            0xfb, 0xdc, 0x0d, 0x4a, 0x67, 0xbc, 0x72, 0xf5, 0x43, 0x5e, 0x9b, 0x13, 0xf2, 0xe4, 0x6d, 0x49,
            0xdb, 0x76, 0xcb, 0x42, 0x6a, 0x3c, 0x9f, 0xa1, 0xfe, 0x5e, 0xca, 0x0a, 0xfc, 0xfa, 0x39, 0x27,
            0xd1, 0x3c, 0xcb, 0x9a, 0xde, 0x4c, 0x6b, 0x09, 0x8b, 0x49, 0xfd, 0x1e, 0x3d, 0x5e, 0x67, 0x7c,
            0x57, 0xad, 0x90, 0xcc, 0x46, 0x5f, 0x5c, 0xae, 0x6a, 0x9c, 0xb2, 0xcd, 0x2c, 0x89, 0x78, 0xcf,
            0xf1, 0x49, 0x96, 0x55, 0x1e, 0x04, 0xef, 0x0e, 0x1c, 0xde, 0x6c, 0x96, 0x51, 0x00, 0xee, 0x9a,
            0x1f, 0x8d, 0x61, 0xbc, 0xeb, 0xb1, 0xa6, 0xa5, 0x21, 0x8b, 0xa7, 0xf8, 0x25, 0x41, 0x48, 0x62,
            0x5b, 0x01, 0x6c, 0x7c, 0x2a, 0xe8, 0xff, 0xf9, 0xf9, 0x1f, 0xe2, 0x79, 0x2e, 0xd1, 0xff, 0xa3,
            0x2e, 0x1c, 0x3a, 0x1a, 0x5d, 0x2b, 0x7b, 0x87, 0x25, 0x22, 0xa4, 0x90, 0xea, 0x26, 0x9d, 0xdd,
            0x13, 0x60, 0x4c, 0x10, 0x03, 0xf6, 0x99, 0xd3, 0x21, 0x0c, 0x69, 0xc6, 0xd8, 0xc8, 0x9e, 0x94,
            0x89, 0x51, 0x21, 0xe3, 0x9a, 0xcd, 0xda, 0x54, 0x72, 0x64, 0xae, 0x94, 0x79, 0x36, 0x81, 0x44,
            0x14, 0x6d, 0x3a, 0x0e, 0xa6, 0x30, 0xbf, 0x95, 0x99, 0xa6, 0xf5, 0x7f, 0x4f, 0xef, 0xc6, 0x71,
            0x2f, 0x36, 0x13, 0x14, 0xa2, 0x9d, 0xc2, 0x0c, 0x0d, 0x4e, 0xc0, 0x02, 0xd3, 0x6f, 0xee, 0x98,
            0x5e, 0x24, 0x31, 0x74, 0x11, 0x96, 0x6e, 0x43, 0x57, 0xe8, 0x8e, 0xa0, 0x8d, 0x3d, 0x79, 0x38,
            0x20, 0xc2, 0x0f, 0xb4, 0x75, 0x99, 0x3b, 0xb1, 0xf0, 0xe8, 0xe1, 0xda, 0xf9, 0xd4, 0xe6, 0xd6,
            0xf4, 0x8a, 0x32, 0x4a, 0x4a, 0x25, 0xa8, 0xd9, 0x60, 0xd6, 0x33, 0x31, 0x97, 0xb9, 0xb6, 0xed,
            0x5f, 0xfc, 0x15, 0xbd, 0x13, 0xc0, 0x3a, 0x3f, 0x1f, 0x2d, 0x09, 0x1d, 0xeb, 0x69, 0x6a, 0xfe,
            0xd7, 0x95, 0x3e, 0x8a, 0x4e, 0xe1, 0x6e, 0x61, 0xb2, 0x6c, 0xe3, 0x2b, 0x70, 0x60, 0x7e, 0x8c,
            0xe4, 0xdd, 0x27, 0x30, 0x7e, 0x0d, 0xc7, 0xb7, 0x9a, 0x1a, 0x3c, 0xcc, 0xa7, 0x22, 0x77, 0x14,
            0x05, 0x50, 0x57, 0x31, 0x1b, 0xc8, 0xbf, 0xce, 0x52, 0xaf, 0x9c, 0x8e, 0x10, 0x2e, 0xd2, 0x16,
            0xb6, 0x6e, 0x43, 0x10, 0xaf, 0x8b, 0xde, 0x1d, 0x60, 0xb2, 0x7d, 0xe6, 0x2f, 0x08, 0x10, 0x12,
            0x7e, 0xb4, 0x76, 0x45, 0xb6, 0xd8, 0x9b, 0x26, 0x40, 0xa1, 0x63, 0x5c, 0x7a, 0x2a, 0xb1, 0x8c,
            0xd6, 0xa4, 0x6f, 0x5a, 0xae, 0x33, 0x7e, 0x6d, 0x71, 0xf5, 0xc8, 0x6d, 0x80, 0x1c, 0x35, 0xfc,
            0x3f, 0xc1, 0xa6, 0xc6, 0x1a, 0x15, 0x04, 0x6d, 0x76, 0x38, 0x32, 0x95, 0xb2, 0x51, 0x1a, 0xe9,
            0x3e, 0x89, 0x9f, 0x0c, 0x79];
        
        let mut ctx = Sha256::new();
        ctx.update(&msg[..64]).unwrap();
        ctx.update(&msg[64..]).unwrap();

        assert_eq!(ctx.finalize().unwrap().as_ref(), &expected_hash);
    }

@brycx brycx merged commit 9c5efa6 into master Mar 28, 2023
@brycx brycx deleted the sha3 branch March 28, 2023 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants