Add support for SHA-3 hash function by mtausig · Pull Request #7881 · RIOT-OS/RIOT

mtausig · 2017-10-26T22:28:23Z

Add the SHA-3 hash function to the hashes module

The output lengths SHA3-256, SHA3-384 and SHA3-512 are supported.

The code is taken from the public domain implementation of the original authors.

Almost finished, just need to test it some more and add the documentation.

emmanuelsearch · 2017-10-30T09:12:49Z

The main arguments for the package approach is that:

we are in contact with the Keccak maintainers, and
the package is currently maintained/developed.

Hence it makes more sense to add a package such as in #7903 rather than copy the code over to the RIOT repo, in my opinion.

mtausig · 2017-10-30T09:20:03Z

@emmanuelsearch I disagree. I think that an essential standard function like SHA-3 should be part of the main distribution, available for easy usage and not hidden away in a package.
Maintainance is also not really an issue here, since the code is pretty static anyway. The code I used had its last commit 2 1/2 years ago and isn't likely to change anytime soon.

What would make sense as a seperate (and additional) package in my opinion, would be a more generic Sponge crypto package featuring a generic sponge implementation plus some different permutations plus all the fancy crypto functions that you can do with a sponge (hash, mac, rng, aead, xof, streamcipher)

emmanuelsearch · 2017-11-06T09:39:05Z

@mtausig OK, I see your point. Maybe you're right. Any other opinions out there?
Else let's go for what you propose, with this PR and in parallel the package approach initiated by #7903

kaspar030 · 2017-11-06T11:12:24Z

What would make sense as a seperate (and additional) package in my opinion, would be a more generic Sponge crypto package featuring a generic sponge implementation plus some different permutations plus all the fancy crypto functions that you can do with a sponge (hash, mac, rng, aead, xof, streamcipher)

What would speak for only using a package is that if sponge crypto and sha3 is used in the same image, it would share code, correct?

mtausig · 2017-11-06T12:40:57Z

What would make sense as a seperate (and additional) package in my opinion, would be a more generic Sponge crypto package featuring a generic sponge implementation plus some different permutations plus all the fancy crypto functions that you can do with a sponge (hash, mac, rng, aead, xof, streamcipher)

What would speak for only using a package is that if sponge crypto and sha3 is used in the same image, it would share code, correct?

Yes, that would prevent code duplication. But it would hide away the sha-3 functions (which are, what most people are going to need) from a lot of users and it would introduce some inconsistency (sha2 and sha2 in the main code, sha3 in a package).

mtausig · 2017-11-13T14:10:55Z

Finished, from my point of view.

emmanuelsearch · 2018-03-01T08:37:47Z

@mtausig can you check the output of Murdock?
Static checks are failing.
Can you fix?

mtausig · 2018-03-01T10:40:44Z

OK. I have fixed everything (apart from squashing) that's in my power.
The one problem remaining is the licence header of sha3.c. Since it is mostly the original Keccak code I have to stick with the original (CC-0) licence. Could Travis be adapted to accept CC-0 licences?

emmanuelsearch · 2018-03-01T13:32:36Z

@miri64 actually we should probably add CC0 in the list of accepted license header patterns, is that right ?

miri64 · 2018-03-01T13:37:54Z

I guess so.

emmanuelsearch · 2018-03-01T15:26:34Z

added a pattern for CC0 in #8716

mtausig · 2018-03-01T15:27:21Z

Do I have to rebase for the pattern to work?

miri64 · 2018-03-05T12:04:17Z

sys/include/hashes/sha3.h

+}
+#endif
+
+#endif  /* HASHES_SHA3_H */


This is really nit-picky, but the script actually complains about the 2 whitespaces between #endif and /* (it seems that it it supposed to be only 1...)

Next try.
It would be great to have those outputs visible.

(header guards are now okay 😌 )

That seems to have done the trick.

mtausig · 2018-03-05T12:53:59Z

Squashed & Rebased

miri64

A little more in-depth review now. Also tested current version on native and samr21-xpro successfully.

miri64 · 2018-03-05T14:19:43Z

tests/unittests/tests-hashes/tests-hashes-sha3.c

+                                     0x58, 0x54, 0xB4, 0x1C, 0xC4, 0x7A, 0xD1, 0x52,
+                                     0x94, 0xBC, 0x41, 0xF3, 0x21, 0x65, 0xDF, 0xBA };
+static const uint8_t hfail_384[] = { 0x89, 0xDB, 0xF4, 0xC3, 0x9B, 0x8F, 0xB4, 0x6F, 0xDF, 0x0A, 0x69, 0x26, 0xCE, 0xC0, 0x35, 0x5A, 0x4B, 0xDB, 0xF9, 0xC6, 0xA4, 0x46, 0xE1, 0x40, 0xB7, 0xC8, 0xBD, 0x08, 0xFF, 0x6F, 0x48, 0x9F, 0x20, 0x5D, 0xAF, 0x8E, 0xFF, 0xE1, 0x60, 0xF4, 0x37, 0xF6, 0x74, 0x91, 0xEF, 0x89, 0x7C, 0x23 };
+static const uint8_t hfail_512[] = { 0x15, 0x0D, 0x78, 0x7D, 0x6E, 0xB4, 0x96, 0x70, 0xC2, 0xA4, 0xCC, 0xD1, 0x7E, 0x6C, 0xCE, 0x7A, 0x04, 0xC1, 0xFE, 0x30, 0xFC, 0xE0, 0x3D, 0x1E, 0xF2, 0x50, 0x17, 0x52, 0xD9, 0x2A, 0xE0, 0x4C, 0xB3, 0x45, 0xFD, 0x42, 0xE5, 0x10, 0x38, 0xC8, 0x3B, 0x2B, 0x4F, 0x8F, 0xD4, 0x38, 0xD1, 0xB4, 0xB5, 0x5C, 0xC5, 0x88, 0xC6, 0xB9, 0x13, 0x13, 0x2F, 0x1A, 0x65, 0x8F, 0xB1, 0x22, 0xCB, 0x52 };


Where are all these values coming from? Please provide a comment.

miri64 · 2018-03-05T14:20:17Z

sys/hashes/sha3.c

+
+   RIOT OS adaptations (c) Mathias Tausig
+
+   This software is released under the Creative Commons CC0 1.0 license. To the extent possible under law, the implementer has waived all copyright and related or neighboring rights to the source code in this file. For more information see: http://creativecommons.org/publicdomain/zero/1.0/


Please address!

miri64 · 2018-03-05T14:26:17Z

sys/hashes/sha3.c

+ * [Keccak Reference] http://keccak.noekeon.org/Keccak-reference-3.0.pdf
+ * [Keccak Specifications Summary] http://keccak.noekeon.org/specs_summary.html
+
+   This file uses UTF-8 encoding, as some comments use Greek letters.


I think some of the content of this could go into the details section of the sha3.h header file (otherwise it isn't added to the online-documentation, since C-files are excluded):

/** * @ingroup sys_hashes * @{ * * @file * @brief Header definitions for the SHA-3 hash function * * <put it here> */

I added that information into the author field. Is that OK?
9a6bf99

miri64 · 2018-03-05T14:27:43Z

sys/hashes/sha3.c

+ *                         from position <i>n</i>+1 to position 7.
+ *                         Some examples:
+ *                             - If no bits are to be appended, then @a delimitedSuffix must be 0x01.
+ *                             - If the 2-bit sequence 0,1 is to be appended (as for SHA3-*), @a delimitedSuffix must be 0x06.


Line length (see coding conventions)

Is there a reason why the line length is not handled by the uncrustify configuration?

miri64 · 2018-03-05T14:28:13Z

sys/hashes/sha3.c

+ * @param  outputByteLen   The number of output bytes desired.
+ * @pre    One must have r+c=1600 and the rate a multiple of 8 bits in this implementation.
+ */
+void Keccak(unsigned int rate, unsigned int capacity, const unsigned char *input, unsigned long long int inputByteLen, unsigned char delimitedSuffix, unsigned char *output, unsigned long long int outputByteLen);


This function is internal to this C-file, so please make it static

miri64 · 2018-03-05T14:39:45Z

sys/include/hashes/sha3.h

+/**
+ * @brief Squeeze data from a sponge
+ *
+ * @param ctx                context handle of the sponge


miri64 · 2018-03-05T14:39:51Z

sys/include/hashes/sha3.h

+/**
+ * @brief SHA-3-256 initialization.  Begins a SHA-3-256 operation.
+ *
+ * @param ctx  keccak_state_t handle to initialise


miri64 · 2018-03-05T14:40:07Z

sys/include/hashes/sha3.h

+/**
+ * @brief Add bytes into the hash
+ *
+ * @param ctx      context handle to use


miri64 · 2018-03-05T14:40:26Z

sys/include/hashes/sha3.h

+ * @brief SHA-3-256 finalization.  Pads the input data and exports the hash value
+ *
+ * @param ctx    context handle to use
+ * @param digest resulting digest, this is the hash of all the bytes


Dito for ctx an digest.

miri64 · 2018-03-05T14:40:42Z

sys/include/hashes/sha3.h

+/**
+ * @brief SHA-3-384 initialization.  Begins a SHA-3-256 operation.
+ *
+ * @param ctx  keccak_state_t handle to initialise


miri64

Forgot to set to change request...

mtausig · 2018-03-06T12:45:13Z

@miri64 I think I have addressed all of your concerns. Thanks for the feedback.

miri64

ACK (except for a minor optional improvement suggestion). Please squash.

miri64 · 2018-03-06T12:48:52Z

sys/hashes/sha3.c

+typedef uint64_t UINT64;
+typedef UINT64 tKeccakLane;
+
+#ifndef LITTLE_ENDIAN


Technically you can define this in this file so the user doesn't need to get active:

#if __BYTE_ORDER__ == __ORDER__LITTLE_ENDIAN__ #define LITTLE_ENDIAN #endif

Done. 71cd9bd

I'll wait for Murdock and squash afterwards.

mtausig · 2018-03-06T14:02:35Z

Squashed.

emmanuelsearch · 2018-03-06T15:04:04Z

There is a typo in the first commit message "Keccap" => "Keccak" ;)

mtausig · 2018-03-06T15:05:47Z

@emmanuelsearch Oops. Fixed.

miri64

Last thing, I swear:

Commit message Import Keccak code from Keccak Code Pacakge has a typo "Pacakge" => "Package"
Please prefix all your commit messages with the module you add/modify, e.g. hashes: or hashes: sha3: .

Add init/update/final interface Add interface functions for direct SHA3 Add unit tests for SHA-3 Document functions and types Reduced var scope in Keccak code Add CCO Copyright notice to Keccak code

Changed integer typedefs to portable stdint types Added Endianness define

Remove unused SHAKE functions Removed unused SHA3-224

Fixed typo

mtausig · 2018-03-06T16:27:32Z

Better?

miri64

Yes, thank you for your patience! ACK.

miri64 requested a review from BytesGalore October 26, 2017 23:13

miri64 assigned BytesGalore Oct 26, 2017

miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: security Area: Security-related libraries and subsystems labels Oct 26, 2017

miri64 added this to the Release 2017.10 milestone Oct 26, 2017

miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 27, 2017

miri64 mentioned this pull request Oct 29, 2017

pkg: add a package for hashing with Keccak with unit test #7903

Draft

mtausig changed the title ~~[WIP] Add support for SHA-3 hash function~~ Add support for SHA-3 hash function Nov 13, 2017

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

smlng removed this from the Release 2018.01 milestone Jan 12, 2018

emmanuelsearch added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Feb 28, 2018

mtausig force-pushed the feature/sha3 branch from 3a43044 to 815dc83 Compare March 1, 2018 10:21

emmanuelsearch added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 1, 2018

miri64 requested changes Mar 5, 2018

View reviewed changes

miri64 mentioned this pull request Mar 5, 2018

dist: header guard checker too nit-picky #8737

Closed

mtausig force-pushed the feature/sha3 branch 2 times, most recently from 74179e0 to 368488b Compare March 5, 2018 12:53

miri64 reviewed Mar 5, 2018

View reviewed changes

miri64 requested changes Mar 5, 2018

View reviewed changes

miri64 reviewed Mar 6, 2018

View reviewed changes

emmanuelsearch added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 6, 2018

mtausig force-pushed the feature/sha3 branch from 71cd9bd to d54a117 Compare March 6, 2018 13:29

mtausig force-pushed the feature/sha3 branch from d54a117 to a60b504 Compare March 6, 2018 15:05

miri64 requested changes Mar 6, 2018

View reviewed changes

Mathias Tausig added 5 commits March 6, 2018 16:17

hashes:sha3: Import Keccak code from Keccak Code Package

a44a313

hashes:sha3: Integrate SHA-3 into RIOT API

792137f

Add init/update/final interface Add interface functions for direct SHA3 Add unit tests for SHA-3 Document functions and types Reduced var scope in Keccak code Add CCO Copyright notice to Keccak code

hashes:sha3: Alterations to Keccak code

91a473a

Changed integer typedefs to portable stdint types Added Endianness define

hashes:sha3: Remove unused functions from Keccak code

ce099c6

Remove unused SHAKE functions Removed unused SHA3-224

hashes: SHA256 documentation updated

44723c9

Fixed typo

mtausig force-pushed the feature/sha3 branch from a60b504 to 44723c9 Compare March 6, 2018 15:18

miri64 approved these changes Mar 6, 2018

View reviewed changes

miri64 merged commit 0b928b7 into RIOT-OS:master Mar 6, 2018

mtausig deleted the feature/sha3 branch March 6, 2018 16:35


		RIOT OS adaptations (c) Mathias Tausig

		This software is released under the Creative Commons CC0 1.0 license. To the extent possible under law, the implementer has waived all copyright and related or neighboring rights to the source code in this file. For more information see: http://creativecommons.org/publicdomain/zero/1.0/

Conversation

mtausig commented Oct 26, 2017

Uh oh!

emmanuelsearch commented Oct 30, 2017

Uh oh!

mtausig commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emmanuelsearch commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Nov 6, 2017

Uh oh!

mtausig commented Nov 6, 2017

Uh oh!

mtausig commented Nov 13, 2017

Uh oh!

emmanuelsearch commented Mar 1, 2018

Uh oh!

mtausig commented Mar 1, 2018

Uh oh!

emmanuelsearch commented Mar 1, 2018

Uh oh!

miri64 commented Mar 1, 2018

Uh oh!

emmanuelsearch commented Mar 1, 2018

Uh oh!

mtausig commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtausig commented Mar 5, 2018

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

mtausig commented Mar 6, 2018

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtausig Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mtausig commented Oct 30, 2017 •

edited

Loading

emmanuelsearch commented Nov 6, 2017 •

edited

Loading

mtausig Mar 6, 2018 •

edited

Loading

emmanuelsearch commented Mar 6, 2018 •

edited

Loading