Subject: RFC 4634 errata and other issues

From: Alfred Hönes <[email-masked]>
Date: Sun, 13 Aug 2006 21:22:20 +0200 (MESZ)

Hello,
studying the recently published RFC 4634 (SHAs and HMAC-SHAs)
authored by you, I found a lot of flaws, including errors in the
sample code.  Here are my comments on this memo.

The items below are grouped into two parts:

The first part lists typos and errors in the code (in textual order of
occurrence) that perhaps should be addressed by an RFC Errata Note.

The second part will present other issues and proposals for further
enhancement of the sample code; that might just be addressed by
updating a publicly available copy of the sample source code, or
any other derived work.
These issues often are related to multiple parts of the sample source
code, and they are presented in order of first occurrence of related
sample source code, unrelated to any scale of importance.

Original text is reproduced without change in indentation.
I use change bars ('|' in column 1) to mark issues/changes.
Where deemed useful, additional lines with up/down pointing tags
('^^^'/'vvv') will be used to emphasize the location of textual
issues and/or proposed changes.
The proposed text changes intentionally have been aligned in style
and wording with the remainder of the RFC; proposed text generally
is re-adjusted to conform with RFC formatting rules.

===  Part 1 -- errata  ===

(1)  [incomplete specification]

Section 3 of RFC 4634, on page 5, defines the elementary word
operations to be used subsequently in the text, including the
left shift operation, '<<'.  Unfortunately, the right shift
operation '>>' is used frequently as well, but not defined
in Section 3.

I propose to amend the second paragraph of Section 3, on page 5,

   In the operations below, x<<n is obtained as follows: discard the
   left-most n bits of x and then pad the result with n zeroed bits on
   the right (the result will still be the same number of bits).

to read:

   In the operations below, x<<n is obtained as follows: discard the
   left-most n bits of x and then pad the result with n zeroed bits on
   the right (the result will still be the same number of bits).
|  Similarly, x>>n is obtained as follows: discard the right-most n bits
|  of x and then prepend the result with n zeroed bits on the left (the
|  result will still be the same number of bits).

(2)  [typo]

The last two text lines of Section 3, on mid-page 6, say:
                                 v
|            ROTL^n(x) = ROTR^(w-x)(x)

             ROTR^n(x) = ROTL^(w-n)(x)

They should say:
                                 v
|            ROTL^n(x) = ROTR^(w-n)(x)

             ROTR^n(x) = ROTL^(w-n)(x)

(3)  [inconsistent prototypes, unpleasant/inconsistent indentation]

In the introductory text in Section 8, function prototype arguments
are inconsistently presented; all should be presented in ANSI-C style
consistently.
Also, the indentation of two lines breaks the otherwise consistent
layout.

On mid-page 16, the lines,

   Functions:
|                 int SHA$$$Reset(SHA$$$Context *);
            Reset the hash context state
|     int SHA$$$Input(SHA$$$Context *, const uint8_t *octets,
                  unsigned int bytecount);
            Incorporate bytecount octets into the hash.

should read:

   Functions:
|     int SHA$$$Reset(SHA$$$Context *context);
            Reset the hash context state
|     int SHA$$$Input(SHA$$$Context *context, const uint8_t *octets,
                  unsigned int bytecount);
            Incorporate bytecount octets into the hash.

and on page 17, the lines,

   Functions:
|     int USHAReset(USHAContext *, SHAversion whichSha);
            Reset the hash context state.
|     int USHAInput(USHAContext *,
                  const uint8_t *bytes, unsigned int bytecount);
            Incorporate bytecount octets into the hash.
|     int USHAFinalBits(USHAContext *,
                  const uint8_t bits, unsigned int bitcount);
|                 Incorporate bitcount bits into the hash.

should read:

   Functions:
|     int USHAReset(USHAContext *context, SHAversion whichSha);
            Reset the hash context state.
|     int USHAInput(USHAContext *context,
                  const uint8_t *bytes, unsigned int bytecount);
            Incorporate bytecount octets into the hash.
|     int USHAFinalBits(USHAContext *context,
                  const uint8_t bits, unsigned int bitcount);
|           Incorporate bitcount bits into the hash.

(4)  Section 8.1 -- sha.h  [terminology]

The initial Description in the file, on page 18 of the RFC, says:

                                             vvvvvvvvvvvvvvvvvv
 *  Description:
 *      This file implements the Secure Hash Signature Standard
 *      algorithms as defined in the National Institute of Standards
 *      and Technology Federal Information Processing Standards
 *      Publication (FIPS PUB) 180-1 published on April 17, 1995, 180-2
 *      published on August 1, 2002, and the FIPS PUB 180-2 Change
 *      Notice published on February 28, 2004.

It should say:

 *  Description:
|*      This file implements the Secure Hash Algorithms
 *      as defined in the National Institute of Standards
 *      and Technology Federal Information Processing Standards
 *      Publication (FIPS PUB) 180-1 published on April 17, 1995, 180-2
 *      published on August 1, 2002, and the FIPS PUB 180-2 Change
 *      Notice published on February 28, 2004.

Rationale:

Avoiding the term "Signature" in accordance with the Standards
mentioned.  The NIST consistently uses the acronyms "SHA" for
"Secure Hash Algorithm" and "SHS" for "Secure Hash Standard"
and precisely distinguished between these two terms.

(5)  Section 8.2.1 -- sha.c  [terminology, and clarification]

The initial Description in the file, on page 24 of the RFC, says:

                                             vvvvvvvvvvvvvvvvvv
 *  Description:
 *      This file implements the Secure Hash Signature Standard
 *      algorithms as defined in the National Institute of Standards
 *      and Technology Federal Information Processing Standards
 *      Publication (FIPS PUB) 180-1 published on April 17, 1995, 180-2
 *      published on August 1, 2002, and the FIPS PUB 180-2 Change
 *      Notice published on February 28, 2004.

It should say:

 *  Description:
|*      This file implements the Secure Hash Algorithm SHA-1
 *      as defined in the National Institute of Standards
 *      and Technology Federal Information Processing Standards
 *      Publication (FIPS PUB) 180-1 published on April 17, 1995, 180-2
 *      published on August 1, 2002, and the FIPS PUB 180-2 Change
 *      Notice published on February 28, 2004.

Rationale:

See item (4) above.
Also replace "algorithms" by "Algorithm SHA-1" to properly match
the description with the scope of the file.

(6)  Section 8.2.1 -- sha1.c  [clarification in comment text]

The comment text, at the bottom of page 24, says:

 *  Caveats:
 *      SHA-1 is designed to work with messages less than 2^64 bits
 *      long. This implementation uses SHA1Input() to hash the bits
 *      that are a multiple of the size of an 8-bit character, and then
 *      uses SHA1FinalBits() to hash the final few bits of the input.
 */

It should better say:

 *  Caveats:
 *      SHA-1 is designed to work with messages less than 2^64 bits
 *      long. This implementation uses SHA1Input() to hash the bits
 *      that are a multiple of the size of an 8-bit character, and then
|*      optionally uses SHA1FinalBits() to hash the final few bits of
 *      the input.
 */

(7)  Section 8.2.1 -- sha1.c  [wrong constants used]

Near the top of page 25, there is the code:

/*
 * add "length" to the length
 */
static uint32_t addTemp;
#define SHA1AddLength(context, length)                     \
    (addTemp = (context)->Length_Low,                      \
     (context)->Corrupted =                                \
        (((context)->Length_Low += (length)) < addTemp) && \
        (++(context)->Length_High == 0) ? 1 : 0)

It should say (modifying the last line):

/*
 * add "length" to the length
 */
static uint32_t addTemp;
#define SHA1AddLength(context, length)                     \
    (addTemp = (context)->Length_Low,                      \
     (context)->Corrupted =                                \
        (((context)->Length_Low += (length)) < addTemp) && \
        (++(context)->Length_High == 0) ? shaInputTooLong : shaSuccess )

Rationale:

As can be found on page 19 (upper half), sha.h contains:

#ifndef _SHA_enum_
#define _SHA_enum_
/*
 *  All SHA functions return one of these values.
 */
enum {
    shaSuccess = 0,
    shaNull,            /* Null pointer parameter */
    shaInputTooLong,    /* input data too long */
    shaStateError,      /* called Input after FinalBits or Result */
    shaBadParam         /* passed a bad parameter */
};
#endif /* _SHA_enum_ */

This leaves it to the compiler to assign values, but ordinarily,
  shaNull          will be assigned the value 1,
  shaInputTooLong  will be assigned the value 2, etc. ...

The value assigned to context->Corrupted in the #define listed
above will later on repeatedly be used to generate return values,
via code lines:
                 return context->Corrupted;

These return values are expected to be SHA_enum values.
In the case where Corrupted gets assigned the value 0, it apparently
was intended to eventually get the return value 'shaSuccess', and
in the case where Corrupted gets assigned the value 1, it apparently
was intended to eventually get the return value 'shaInputTooLong'.
With the code shown above, the former will work, but the latter
will usually *not* work as intended.

To obtain portable source code behaving as documented, the proposed
change has to be applied.

(8)  Section 8.2.1 -- sha1.c  [inconsistent function prototypes]

Througout the sample source code, ANSI-C style is used for the
function prototypes, i.e. giving type and name for function
arguments.  This rule is broken on mid-page 25, just below
the offending snippit from item (5) above.
For consistency and portability, the source code fragment:

/* Local Function Prototypes */
static void SHA1Finalize(SHA1Context *context, uint8_t Pad_Byte);
static void SHA1PadMessage(SHA1Context *, uint8_t Pad_Byte);
static void SHA1ProcessMessageBlock(SHA1Context *);

should better say, amending the last two lines:

/* Local Function Prototypes */
static void SHA1Finalize(SHA1Context *context, uint8_t Pad_Byte);
static void SHA1PadMessage(SHA1Context *context, uint8_t Pad_Byte);
static void SHA1ProcessMessageBlock(SHA1Context *context);

(9)  Section 8.2.1 -- sha1.c  [improper comment text]

The Description comments for the SHA$$$Result functions repeatedly
contains improper wording and unpleasent formatting.
See below for significant flaws.
This change is proposed for the sake of consistency.

The Description of SHA1Result on page 28, says:

 * Description:
 *   This function will return the 160-bit message digest into the
 *   Message_Digest array provided by the caller.
 *   NOTE: The first octet of hash is stored in the 0th element,
 *      the last octet of hash in the 19th element.

For correctness and consistency, it should better say:

 * Description:
 *   This function will return the 160-bit message digest
 *   into the Message_Digest array provided by the caller.
 *   NOTE:
 *    The first octet of the hash is stored in the element with index 0,
 *    the last octet of the hash in the element with index 19.

[The additional line break has been added to keep the first part
of the last sentence on a single line, under RFC formatting rules.]

(10)  Section 8.2.1 -- sha1.c  [improper comment text]

The Description of SHA1PadMessage, on page 30, says:

 * Description:
 *   According to the standard, the message must be padded to an
 *   even 512 bits. The first padding bit must be a '1'. The last
 *   64 bits represent the length of the original message. All bits
 *   in between should be 0. This helper function will pad the
 *   message according to those rules by filling the Message_Block
 *   array accordingly. When it returns, it can be assumed that the
 *   message digest has been computed.

For clarity, it should say:

 * Description:
|*   According to the standard, the message must be padded to the next
|*   proper multiple of 512 bits. The first padding bit must be a '1'.
 *   The last 64 bits represent the length of the original message.
 *   All bits in between should be 0. This helper function will pad
 *   the message according to those rules by filling the Message_Block
 *   array accordingly. When it returns, it can be assumed that the
 *   message digest has been computed.

(11)  Section 8.2.1 -- sha1.c  [wrong comment text]

The Description of SHA1ProcessMessageBlock, on page 31, says:

 * Parameters:
 *   None.

This is not true, as can be seen subsequently in the source code.
The RFC should say:

 * Parameters:
 *   context: [in/out]
 *     The SHA context to update

(12)  Section 8.2.2 -- sha224-256.c  [wrong comment text]

The initial Description in this file, on page 33, says:

 * Description:
 *   This file implements the Secure Hash Signature Standard
 *   algorithms as defined in the National Institute of Standards
 *   and Technology Federal Information Processing Standards
 *   Publication (FIPS PUB) 180-1 published on April 17, 1995, 180-2
 *   published on August 1, 2002, and the FIPS PUB 180-2 Change
 *   Notice published on February 28, 2004.

It should say:

 * Description:
 *   This file implements the Secure Hash Algorithms SHA-224 and
 *   SHA-256, as defined in the National Institute of Standards
 *   and Technology Federal Information Processing Standards
 *   Publication (FIPS PUB) 180-2 published on August 1, 2002, and
 *   the FIPS PUB 180-2 Change Notice published on February 28, 2004.

Rationale:

FIPS-PUB 180-1 only specified SHA-1, neither SHA-224 nor SHA-256.
FIPS-PUB 180-2 has introduced SHA-256 (and SHA-384 and SHA-512 as
well), and SHA-224 has been introduced by the "Change Notice 1".
Thus, citation of FIPS PUB 180-1 is void and inappropriate in the
context of SHA-224 and SHA-256.
Avoiding the term "Signature" also conforms to the above Standards
-- cf. item (4) and (5) above.
Restricting the text to the scope of the file -- cf. item (5) above.

(13)  Section 8.2.2 -- sha224-256.c  [clarification in comment text]

The comment text, near the bottom of page 33, says:

 * Caveats:
 *   SHA-224 and SHA-256 are designed to work with messages less
 *   than 2^64 bits long. This implementation uses SHA224/256Input()
 *   to hash the bits that are a multiple of the size of an 8-bit
 *   character, and then uses SHA224/256FinalBits() to hash the
 *   final few bits of the input.

It should better say -- cf. item (6) above:

 * Caveats:
 *   SHA-224 and SHA-256 are designed to work with messages less
 *   than 2^64 bits long. This implementation uses SHA224/256Input()
 *   to hash the bits that are a multiple of the size of an 8-bit
|*   character, and then optionally uses SHA224/256FinalBits()
 *   to hash the final few bits of the input.

(14)  Section 8.2.2 -- sha224-256.c  [wrong constants used]

On mid-page 34, there is the code:

/*
 * add "length" to the length
 */
static uint32_t addTemp;
#define SHA224_256AddLength(context, length)               \
  (addTemp = (context)->Length_Low, (context)->Corrupted = \
    (((context)->Length_Low += (length)) < addTemp) &&     \
    (++(context)->Length_High == 0) ? 1 : 0)

It should say (modifying the last line):

/*
 * add "length" to the length
 */
static uint32_t addTemp;
#define SHA224_256AddLength(context, length)               \
  (addTemp = (context)->Length_Low, (context)->Corrupted = \
    (((context)->Length_Low += (length)) < addTemp) &&     \
    (++(context)->Length_High == 0) ? shaInputTooLong : shaSuccess )

Rationale:  Same as for item (7) above.

(15)  Section 8.2.2 -- sha224-256.c  [misleading typo]

On top of page 35, the Description of SHA224Reset says:

                                          vvv
 * Description:
 *   This function will initialize the SHA384Context in preparation
 *   for computing a new SHA224 message digest.

It should say:

 * Description:
|*   This function will initialize the SHA224Context in preparation
 *   for computing a new SHA224 message digest.

(16)  Section 8.2.2 -- sha224-256.c  [improper comment text]

Similar to item (9) above, the Description comment of SHA224Result
contains improper wording and unpleasent formatting. Additionally,
counting 28 elements as ranging from the "0th" up to the "28th" is
unpleasant and wrong -- it would indicate 29 elements (octets) !

On mid-page 36, the RFC says:

 * Description:
 *   This function will return the 224-bit message
 *   digest into the Message_Digest array provided by the caller.
 *   NOTE: The first octet of hash is stored in the 0th element,
 *      the last octet of hash in the 28th element.

For correctness and consistency, and for improved readability,
it should say:

 * Description:
 *   This function will return the 224-bit message digest
 *   into the Message_Digest array provided by the caller.
 *   NOTE:
 *    The first octet of the hash is stored in the element with index 0,
 *    the last octet of the hash in the element with index 27.

(17)  Section 8.2.2 -- sha224-256.c  [improper comment text]

Similar to item (9) and (16) above, the Description of SHA256Result
contains improper wording and unpleasent formatting. Additionally,
counting 32 elements as ranging from the "0th" up to the "32nd" is
unpleasant and wrong -- it would indicate 33 elements (octets) !

On page 39, the RFC says:

 * Description:
 *   This function will return the 256-bit message
 *   digest into the Message_Digest array provided by the caller.
 *   NOTE: The first octet of hash is stored in the 0th element,
 *      the last octet of hash in the 32nd element.

For correctness and consistency, and for improved readability,
it should say:

 * Description:
 *   This function will return the 256-bit message digest
 *   into the Message_Digest array provided by the caller.
 *   NOTE:
 *    The first octet of the hash is stored in the element with index 0,
 *    the last octet of the hash in the element with index 31.

(18)  Section 8.2.2 -- sha224-256.c  [inconsistent arguments]

The sample code presents almost all formal function arguments of type
array with predefined (constant) length with this explicit length.
Contrary to that, the definition of SHA256Result does not supply
the expected size of the formal argument 'Message_Digest'.

At the bottom of page 39, RFC 4634 says:

int SHA256Result(SHA256Context *context, uint8_t Message_Digest[])
{

For consistency and clarity, it should say:

int SHA256Result(SHA256Context *context,
                 uint8_t Message_Digest[SHA256HashSize])
{

(19)  Section 8.2.2 -- sha224-256.c  [improper comment text]

The Description of SHA224_256PadMessage, near the bottom of page 40,
says:

 * Description:
 *   According to the standard, the message must be padded to an
 *   even 512 bits. The first padding bit must be a '1'. The
 *   last 64 bits represent the length of the original message.
 *   All bits in between should be 0. This helper function will pad
 *   the message according to those rules by filling the
 *   Message_Block array accordingly. When it returns, it can be
 *   assumed that the message digest has been computed.

For clarity, it should say (cf. item (10) above):

 * Description:
|*   According to the standard, the message must be padded to the next
|*   proper multiple of 512 bits. The first padding bit must be a '1'.
 *   The last 64 bits represent the length of the original message.
 *   All bits in between should be 0. This helper function will pad
 *   the message according to those rules by filling the
 *   Message_Block array accordingly. When it returns, it can be
 *   assumed that the message digest has been computed.

(20)  Section 8.2.2 -- sha224-256.c  [imprecise comment text]

The Description of SHA224_256ProcessMessageBlock, on top of page 42,
says:

 * Description:
 *   This function will process the next 512 bits of the message
 *   stored in the Message_Block array.

Consistently with the remainder of the test, it should say:

 * Description:
|*   This helper function will process the next 512 bits of the
 *   message stored in the Message_Block array.

(21)  Section 8.2.2 -- sha224-256.c  [incomplete comment text]

Near the bottom of page 43, the Description of SHA224_256Reset says:

 * Description:
 *   This helper function will initialize the SHA256Context in
 *   preparation for computing a new SHA256 message digest.

For completeness and consistency, it should say:

 * Description:
 *   This helper function will initialize the SHA256Context in
|*   preparation for computing a new SHA-224 or SHA-256 message digest.
                                     ^^^^^^^^^^    ^

(22)  Section 8.2.2 -- sha224-256.c  [improper comment text]

Similar to item (9), (16), and (17) above, the Description of
SHA224_256ResultN contains improper wording. Additionally,
counting 28/32 elements as ranging from the "0th" up to the
"28th/32nd" is unpleasant and wrong -- that erroneously indicates
29/33 elements (octets) !

Near the bottom of page 44, the RFC says:

 * Description:
 *   This helper function will return the 224-bit or 256-bit message
 *   digest into the Message_Digest array provided by the caller.
 *   NOTE: The first octet of hash is stored in the 0th element,
 *      the last octet of hash in the 28th/32nd element.

For correctness and consistency, it should say:

 * Description:
 *   This helper function will return the 224-bit or 256-bit message
 *   digest into the Message_Digest array provided by the caller.
 *   NOTE:
 *    The first octet of the hash is stored in the element with index 0,
 *    the last octet of the hash in the element with index 27/31.

(23)  Section 8.2.3 -- sha384-512.c  [wrong comment text]

The initial Description in this file, on page 45, says:

 * Description:
 *   This file implements the Secure Hash Signature Standard
 *   algorithms as defined in the National Institute of Standards
 *   and Technology Federal Information Processing Standards
 *   Publication (FIPS PUB) 180-1 published on April 17, 1995, 180-2
 *   published on August 1, 2002, and the FIPS PUB 180-2 Change
 *   Notice published on February 28, 2004.

It should say:

 * Description:
 *   This file implements the Secure Hash Algorithms SHA-384 and
 *   SHA-512, as defined in the National Institute of Standards
 *   and Technology Federal Information Processing Standards
 *   Publication (FIPS PUB) 180-2 published on August 1, 2002, and
 *   the FIPS PUB 180-2 Change Notice published on February 28, 2004.

Rationale:

FIPS-PUB 180-1 only specified SHA-1, neither SHA-384 nor SHA-512.
FIPS-PUB 180-2 has introduced SHA-384 and SHA-512 (and SHA-256 as
well), and the "Change Notice 1" has introduced SHA-224.
Thus, citation of FIPS PUB 180-1 is void and inappropriate in the
context of SHA-384 and SHA-512.
Avoiding the term "Signature" also conforms to the above Standards
-- cf. item (4), (5), and (12) above.
Restricting the text to the scope of the file -- cf. item (5) and
(12) above.

(24)  Section 8.2.3 -- sha384-512.c  [clarification in comment text]

The comment text, near the top of page 46, says:

 * Caveats:
 *   SHA-384 and SHA-512 are designed to work with messages less
 *   than 2^128 bits long. This implementation uses
 *   SHA384/512Input() to hash the bits that are a multiple of the
 *   size of an 8-bit character, and then uses SHA384/256FinalBits()
 *   to hash the final few bits of the input.

It should better say -- cf. item (6) and (13) above:

 * Caveats:
 *   SHA-384 and SHA-512 are designed to work with messages less
 *   than 2^128 bits long. This implementation uses SHA384/512Input()
 *   to hash the bits that are a multiple of the size of an 8-bit
|*   character, and optionally then uses SHA384/256FinalBits()
 *   to hash the final few bits of the input.

(25)  Section 8.2.3 -- sha384-512.c  [wrong comment text]

The first line on page 50 says:

#else /* !USE_32BIT_ONLY */

It should say:

#else /* !USE_MODIFIED_MACROS */

Rationale:  Look at the #if[n]def structure of the file.

(26)  Section 8.2.3 -- sha384-512.c  [erroneous code]

Within the '#ifdef USE_32BIT_ONLY' macro definition branch of the
file, on mid-page 50 the RFC says:

/*
 * add "length" to the length
 */
static uint32_t addTemp[4] = { 0, 0, 0, 0 };
#define SHA384_512AddLength(context, length) (                        \
    addTemp[3] = (length), SHA512_ADDTO4((context)->Length, addTemp), \
    (context)->Corrupted = (((context)->Length[3] == 0) &&            \
       ((context)->Length[2] == 0) && ((context)->Length[1] == 0) &&  \
       ((context)->Length[0] < 8)) ? 1 : 0 )

It should say:

/*
 * add "length" to the length
 */
static uint32_t addTemp[4] = { 0, 0, 0, 0 };
#define SHA384_512AddLength(context, length) (                        \
    addTemp[3] = (length), SHA512_ADDTO4((context)->Length, addTemp), \
    (context)->Corrupted = (((context)->Length[0] < addTemp[3]) &&    \
       ((context)->Length[1] == 0) && ((context)->Length[1] == 0) &&  \
       ((context)->Length[0] == 0)) ? shaInputTooLong : shaSuccess )

Rationale:

The context words  Lenght[0] ... Length[3]  represent the unsigned
128-bit-wide running (bit-)length of the message text hash so far,
in most-significant word first order.
The code fragment above is intended to add to this value the
unsigned 32-bit value (uint32_t) length, and to detect overflow
(to 2^128 and above).
The given code is wrong.
(Apparently it has never been tested with messages long enough to
exhibit this misbehaviour.)
Other parts of the sample code show how this can be done correctly
in the case of long accumulators consisting of two 32-bit words
-- cf. the code snippits in item (7) and (14) above, and item (27)
below, as well,
The replacement code corrects this issue.

Furthermore, the original code suffers from the same problem as
in item (7) and (14) above; this has been corrected accordingly,
as well.

(27)  Section 8.2.3 -- sha384-512.c  [wrong constants used]

Within the '#ifndef USE_32BIT_ONLY' macro definition branch of the
file, on mid-page 51 the RFC says:

/*
 * add "length" to the length
 */
static uint64_t addTemp;
#define SHA384_512AddLength(context, length)                   \
   (addTemp = context->Length_Low, context->Corrupted =        \
    ((context->Length_Low += length) < addTemp) &&             \
    (++context->Length_High == 0) ? 1 : 0)

It should say:

/*
 * add "length" to the length
 */
static uint64_t addTemp;
#define SHA384_512AddLength(context, length)                   \
   (addTemp = (context)->Length_Low, (context)->Corrupted =    \
    (((context)->Length_Low += length) < addTemp) &&           \
    (++(context)->Length_High == 0) ? shaInputTooLong : shaSuccess )

Rationale:

Same as for item (7) and (14) above, cf. item (26) as well.

Additionally, parentheses have been added around all invocations of
the macro argument `context` to protect it against various artifacts,
as has been done consistently in the remainder of the sample code.

(28)  Section 8.2.3 -- sha384-512.c  [inconsistent arguments]

The sample code presents almost all formal function arguments of type
array with predefined (constant) length with this explicit length.
Contrary to that, the function prototypes for SHA384_512Reset do not
supply the expected size of the formal argument 'H0'.
This inconsistency should be corrected -- as in item (18) above.

There are two instances of the issue (see item (xx) below for
another two similar instances, with the function declarations):

(28a)
Within the function prototype definition part of the
  '#ifdef USE_32BIT_ONLY'
branch of the sample code, near the bottom of page 50, the RFC says:

static int SHA384_512Reset(SHA512Context *context, uint32_t H0[]);

For consistency and clarity, it should say:

static int SHA384_512Reset(SHA512Context *context,
                           uint32_t H0[SHA512HashSize/4]);

(28b)
Within the function prototype definition part of the
  '#else /* !USE_32BIT_ONLY */'
branch of the sample code, near the bottom of page 51, the RFC says:

static int SHA384_512Reset(SHA512Context *context, uint64_t H0[]);

For consistency and clarity, it should say:

static int SHA384_512Reset(SHA512Context *context,
                           uint64_t H0[SHA512HashSize/8]);

(29)  Section 8.2.3 -- sha384-512.c  [improper comment text]

Similar to item (9), (16), (17), and (22) above, the Description
of SHA384Result contains improper wording and unpleasent formatting.
Additionally, counting 48 elements as ranging from the "0th" up to the
"48th" is unpleasant and wrong -- indicating 49 elements (octets) !

Near the bottom of page 53, the RFC says:

 * Description:
 *   This function will return the 384-bit message
 *   digest into the Message_Digest array provided by the caller.
 *   NOTE: The first octet of hash is stored in the 0th element,
 *      the last octet of hash in the 48th element.

For correctness and consistency, and for improved readability,
it should say:

 * Description:
 *   This function will return the 384-bit message digest
 *   into the Message_Digest array provided by the caller.
 *   NOTE:
 *    The first octet of the hash is stored in the element with index 0,
 *    the last octet of the hash in the element with index 47.

(30)  Section 8.2.3 -- sha384-512.c  [improper comment text]

Similar to item (9), (16), (17), (22), and (29) above, the Description
of SHA512Result contains improper wording and unpleasent formatting.
Additionally, counting 64 elements as ranging from the "0th" up to the
"64th" is unpleasant and wrong -- indicating 65 elements (octets) !

Near the bottom of page 44, the RFC says:

 * Description:
 *   This function will return the 512-bit message
 *   digest into the Message_Digest array provided by the caller.
 *   NOTE: The first octet of hash is stored in the 0th element,
 *      the last octet of hash in the 64th element.

For correctness and consistency, and for improved readability,
it should say:

 * Description:
 *   This function will return the 512-bit message digest
 *   into the Message_Digest array provided by the caller.
 *   NOTE:
 *    The first octet of the hash is stored in the element with index 0,
 *    the last octet of the hash in the element with index 63.

(31)  Section 8.2.3 -- sha384-512.c  [improper comment text]

The Description of SHA384_512PadMessage, on page 58, says:

 * Description:
 *   According to the standard, the message must be padded to an
 *   even 1024 bits. The first padding bit must be a '1'. The
 *   last 128 bits represent the length of the original message.
 *   All bits in between should be 0. This helper function will
 *   pad the message according to those rules by filling the
 *   Message_Block array accordingly. When it returns, it can be
 *   assumed that the message digest has been computed.

For clarity, it should say (cf. items (10) and (19) above):

 * Description:
|*   According to the standard, the message must be padded to the next
|*   proper multiple of 1024 bits. The first padding bit must be a '1'.
 *   The last 128 bits represent the length of the original message.
 *   All bits in between should be 0. This helper function will
 *   pad the message according to those rules by filling the
 *   Message_Block array accordingly. When it returns, it can be
 *   assumed that the message digest has been computed.

(32)  Section 8.2.3 -- sha384-512.c  [inconsistent arguments]

The sample code presents almost all formal function arguments of type
array with predefined (constant) length with this explicit length.
Contrary to that, the function definitions for SHA384_512Reset do not
supply the expected size of the formal argument 'H0'.
This inconsistency should be corrected -- as in items (18) and (28)
above.

Near the top of page 65, RFC 4634 says:

#ifdef USE_32BIT_ONLY
static int SHA384_512Reset(SHA512Context *context, uint32_t H0[])
#else /* !USE_32BIT_ONLY */
static int SHA384_512Reset(SHA512Context *context, uint64_t H0[])
#endif /* USE_32BIT_ONLY */

For consistency and clarity, it should say:

#ifdef USE_32BIT_ONLY
static int SHA384_512Reset(SHA512Context *context,
                           uint32_t H0[SHA512HashSize/4]);
#else /* !USE_32BIT_ONLY */
static int SHA384_512Reset(SHA512Context *context,
                           uint64_t H0[SHA512HashSize/8]);
#endif /* USE_32BIT_ONLY */

(33)  Section 8.2.3 -- sha384-512.c  [improper comment text]

Similar to item (9), (16), (17), (22), (29), and (30) above, the
Description of SHA384_512ResultN contains improper wording.
Additionally, counting 48/64 elements as ranging from the "0th" up to
the "48th/64nd" is unpleasant and wrong -- that erroneously indicates
49/65 elements (octets) !

On page 65/66, the RFC says:

 * Description:
 *   This helper function will return the 384-bit or 512-bit message
<< page break >>
 *   digest into the Message_Digest array provided by the caller.
 *   NOTE: The first octet of hash is stored in the 0th element,
 *      the last octet of hash in the 48th/64th element.

For correctness, it should say:

 * Description:
 *   This helper function will return the 384-bit or 512-bit message
<< page break >>
 *   digest into the Message_Digest array provided by the caller.
 *   NOTE:
 *    The first octet of the hash is stored in the element with index 0,
 *    the last octet of the hash in the element with index 47/63.

(34)  Section 8.2.4 -- usha.c  [wrong comment text]

The Description for USHAResult, on top of page 69, says:

 * Description:
 *   This function will return the 160-bit message digest into the
 *   Message_Digest array provided by the caller.
 *   NOTE: The first octet of hash is stored in the 0th element,
 *      the last octet of hash in the 19th element.

It should say:

 * Description:
 *   This function will return the message digest of the appropriate
 *   bit size, as returned by USHAHashSizeBits(whichSHA) for the
 *   'whichSHA' value used in the preceeding call to USHAReset,
 *   into the Message_Digest array provided by the caller.

Rationale:

The given text roughly matches the SHA-1 case, it is wrong for all
other cases.  The arguments presented for items (9), (16), (17),
(22), (29), (30), and (33) above apply as well.
The changed text tries to remain precise while avoiding too much
repetition of facts presented elsewhere in the sample code.

(35)  Section 8.3 -- hmac.c  [argument inconsistency]

The code for (the message oriented) function hmac,
on page 73/74, reads:

int hmac(SHAversion whichSha, const unsigned char *text, int text_len,
    const unsigned char *key, int key_len,
    uint8_t digest[USHAMaxHashSize])
<< page break >>
{
  HMACContext ctx;
  return hmacReset(&ctx, whichSha, key, key_len) ||
         hmacInput(&ctx, text, text_len) ||
         hmacResult(&ctx, digest);
}

It should say:

int hmac(SHAversion whichSha,
         const unsigned char *message_array, int length,
         const unsigned char *key, int key_len,
         uint8_t digest[USHAMaxHashSize])
<< page break >>
{
  HMACContext ctx;
  return hmacReset(&ctx, whichSha, key, key_len) ||
         hmacInput(&ctx, message_array, length) ||
         hmacResult(&ctx, digest);
}

Rationale:

The argument names `message_array` and `length` are used
throughout the sample code, including the Description of the
function hmac, on page 73.
The code shown above was not aligned with this practise and
hence inconsistent with the Description.
This has been resolved by the proposed update, bay changing
the names of 'text' and 'text_len'.

>>>>>  NOTE / Caution :
>>>>>
>>>>>  Similar (and additional) inconsistencies between the
>>>>>  argument names in the 'Parameters:' documentation
>>>>>  and the variable names used in the subsequent code
>>>>>  exist for all hmac* functions, on pages 74..77 ;
>>>>>  in particular, the described 'context' is always
>>>>>  named `ctx` in the code.
>>>>>  Also, capitalization of the leading "HMAC"/"hmac"
>>>>>  in the function names is totally inconsistent.
>>>>>
>>>>>  Resolution of these issues is left as an exercise
>>>>>  to the reader of this note -- or the author of any
>>>>>  future update of the sample code.
>>>>>
>>>>>  Furthermore, the use of "characters" as units of the
>>>>>  message_text in the descriptions is dangerous in the
>>>>>  days of Unicode and UTF-8; "characters" should better
>>>>>  be replaced by "octets" throughout hmac.c !

(36)  Section 8.3 -- hmac.c  [inconsistent comment]

On mid-page 75, the comment within the function hmacReset says:

   * The HMAC transform looks like:
   *
   * SHA(K XOR opad, SHA(K XOR ipad, text))
   *
   * where K is an n byte key.
   * ipad is the byte 0x36 repeated blocksize times
   * opad is the byte 0x5c repeated blocksize times
   * and text is the data being protected.

It should say:

   * The HMAC transform looks like:
   *
   * SHA(K XOR opad, SHA(K XOR ipad, text))
   *
|  * where K is an n byte key, 0-padded to a total of blocksize bytes,
|  * ipad is the byte 0x36 repeated blocksize times,
|  * opad is the byte 0x5c repeated blocksize times,
   * and text is the data being protected.

Rationale:

Without the addition, the 'XOR' operations in the formula are
undefined.  Additionally, punctuation is made uniform.

(37)  Section 8.3 -- hmac.c  [improper comment text]

The Description of hmacResult, on top of page 77, says:

 * Description:
 *   This function will return the N-byte message digest into the
 *   Message_Digest array provided by the caller.
 *   NOTE: The first octet of hash is stored in the 0th element,
 *      the last octet of hash in the Nth element.

It should perhaps just say:

 * Description:
 *   This function will return the N-byte message digest into the
 *   Message_Digest array provided by the caller.

Rationale:
Cf. items (9), (16), (17), (22), (29), (30), and (33) above.
Full replacement text for the last two lines is deemed unnecessary.

(37)  Section 8.4 -- shatest.c  [comment clarification]

The initial Description of the file, as presented on page 78,
contains in its first paragraph the lines:

 *      the seven tests documented for each algorithm in
 *        "The Secure Hash Algorithm Validation System (SHAVS)",
 *        three of which are bit-level tests
 *        (http://csrc.nist.gov/cryptval/shs/SHAVS.pdf)

For clarity, it should better say:

 *      the seven tests documented for each algorithm in
|*        "The Secure Hash Algorithm Validation System (SHAVS)"
|*        (http://csrc.nist.gov/cryptval/shs/SHAVS.pdf),
|*        three of which are bit-level tests

(38)  Section 8.4 -- shatest.c  [unused #define]

On mid-page 82, the RFC text defines the constant:

#define PRINTBASE64 4

which is never used in the subsequent test driver code
(Base64 output is not supported).
Hence, this line should be deleted.

(39)  Section 8.4 -- shatest.c  [misleading comment]

On mid-page 91, there is the comment:

/*
 * Print the string, converting non-printable characters to hex "## ".
 */

It should say:

/*
 * Print the string, converting all characters to hex "## ".
 */

Rationale:

There is a significant mismatch between the comment and the
subsequent code.
This is being resolved by the replacement text.

(40)  Section 8.4 -- shatest.c  [wrong text in code]

on mid-page 96, the code section,

  if (bitcount > 0)
    err = keyarray ? hmacFinalBits(&hmac, bits, bitcount) :
                   USHAFinalBits(&sha, bits, bitcount);
  if (err != shaSuccess) {
    fprintf(stderr, "hashfile(): %s Error %d.\n",
            keyarray ? "hmacResult" : "shaResult", err);
    if (hashfp != stdin) fclose(hashfp);
    return err;
  }

should in fact say:

  if (bitcount > 0)
    err = keyarray ? hmacFinalBits(&hmac, bits, bitcount) :
                   USHAFinalBits(&sha, bits, bitcount);
  if (err != shaSuccess) {
    fprintf(stderr, "hashfile(): %s Error %d.\n",
            keyarray ? "hmacFinalBits" : "shaFinalBits", err);
    if (hashfp != stdin) fclose(hashfp);
    return err;
  }

Rationale:

Self-evident; perhaps cloning error.

===  Part 2 -- further enhancements  ===             delayed!

Ooouuuch !!!
    Part 1 has taken much more time to create,
    and it has become much longer than expected.
    Therefore, to not hold off the above material:
==>
==> I have deleted the [still raw] draft of part 2 from this message.
==> Part 2 will be [finalized and] sent soon, in a separate message.
==>

Best regards,
  Alfred Hönes.

-- 

+------------------------+--------------------------------------------+
| TR-Sys Alfred Hoenes   |  Alfred Hoenes   Dipl.-Math., Dipl.-Phys.  |
| Gerlinger Strasse 12   |  Phone: (+49)7156/9635-0, Fax: -18         |
| D-71254  Ditzingen     |  E-Mail:  [email-masked]                     |
+------------------------+--------------------------------------------+