Merged PR 6558632: Address the vast majority of other SymCryptFatals

+ In some cases replace with C_ASSERTs
+ In some cases replace with SYMCRYPT_ASSERTs (fail only in CHK build)
+ In some cases replace with SYMCRYPT_ASSERT and replace a faulty input
  with an input which will give a result which is incorrect but won't
  crash

Related work items: #37153656, #35463330

Author: samuel-lee-msft

inc/symcrypt.h

@@ -1395,7 +1395,7 @@ SymCryptParallelSha256Init(
     _Out_writes_( nStates ) PSYMCRYPT_SHA256_STATE pStates,
                             SIZE_T                 nStates );
 
-VOID
+SYMCRYPT_ERROR
 SYMCRYPT_CALL
 SymCryptParallelSha256Process(
     _Inout_updates_( nStates )      PSYMCRYPT_SHA256_STATE              pStates,
@@ -1412,7 +1412,7 @@ SymCryptParallelSha384Init(
     _Out_writes_( nStates ) PSYMCRYPT_SHA384_STATE pStates,
                             SIZE_T                 nStates );
 
-VOID
+SYMCRYPT_ERROR
 SYMCRYPT_CALL
 SymCryptParallelSha384Process(
     _Inout_updates_( nStates )      PSYMCRYPT_SHA384_STATE              pStates,
@@ -1429,7 +1429,7 @@ SymCryptParallelSha512Init(
     _Out_writes_( nStates ) PSYMCRYPT_SHA512_STATE pStates,
                             SIZE_T                 nStates );
 
-VOID
+SYMCRYPT_ERROR
 SYMCRYPT_CALL
 SymCryptParallelSha512Process(
     _Inout_updates_( nStates )      PSYMCRYPT_SHA512_STATE              pStates,
@@ -3546,7 +3546,7 @@ SymCryptChaCha20Crypt(
 // The key stream used is the one generated from the key and nonce, starting at the specified
 // offset into the key stream. This function updates the offset of the state by adding cbData to
 // it so that the next call will use the next part of the key stream.
-// Any attempt to use the key stream at offset >= 2^38 will result in a fatal error.
+// Any attempt to use the key stream at offset >= 2^38 will result in catastrophic loss of security.
 //
 
 VOID
@@ -6270,14 +6270,12 @@ SymCryptMapUint32(
 // This function is particularly useful when mapping error codes in situations where
 // the actual error cannot be revealed through side channels.
 
-#define SYMCRYPT_HARD_ASSERT( _x ) \
+#if SYMCRYPT_DEBUG
+#define SYMCRYPT_ASSERT( _x ) \
     {\
         if( !(_x) ){ SymCryptFatal( 'asrt' ); }\
     }\
     _Analysis_assume_( _x )
-
-#if SYMCRYPT_DEBUG
-#define SYMCRYPT_ASSERT( _x ) SYMCRYPT_HARD_ASSERT( _x )
 #else
 #define SYMCRYPT_ASSERT( _x ) \
     _Analysis_assume_( _x )

inc/symcrypt_internal.h

@@ -209,6 +209,10 @@ typedef uint8_t         BYTE;
 // Size_t
 typedef size_t          SIZE_T;
 
+#ifndef SIZE_T_MAX
+#define SIZE_T_MAX      SIZE_MAX
+#endif
+
 typedef long INT_PTR, *PINT_PTR;
 typedef unsigned long UINT_PTR, *PUINT_PTR;
 
@@ -1688,11 +1692,11 @@ typedef const SYMCRYPT_ECPOINT * PCSYMCRYPT_ECPOINT;
 //
 
 SYMCRYPT_ASYM_ALIGN_STRUCT _SYMCRYPT_INT {
-    UINT32                  type;
-    UINT32                  nDigits;                    // digit size depends on run-time decisions...
-    UINT32                  cbSize;
-    SYMCRYPT_MAGIC_FIELD
+                                                    UINT32  type;
+    _Field_range_( 1, SYMCRYPT_FDEF_UPB_DIGITS )    UINT32  nDigits;    // digit size depends on run-time decisions...
+                                                    UINT32  cbSize;
 
+    SYMCRYPT_MAGIC_FIELD
     SYMCRYPT_ASYM_ALIGN union {
         struct {
             UINT32          uint32[SYMCRYPT_ANYSIZE];   // FDEF: array UINT32[nDigits * # uint32 per digit]
@@ -1707,13 +1711,13 @@ SYMCRYPT_ASYM_ALIGN_STRUCT _SYMCRYPT_INT {
 #define SYMCRYPT_FDEF_SIZEOF_INT_FROM_BITS( _bits )         SYMCRYPT_FDEF_SIZEOF_INT_FROM_DIGITS( SYMCRYPT_FDEF_DIGITS_FROM_BITS( _bits ))
 
 SYMCRYPT_ASYM_ALIGN_STRUCT _SYMCRYPT_DIVISOR {
-    UINT32                  type;
-    UINT32                  nDigits;                    // digit size depends on run-time decisions...
-    UINT32                  cbSize;
+                                                    UINT32  type;
+    _Field_range_( 1, SYMCRYPT_FDEF_UPB_DIGITS )    UINT32  nDigits;    // digit size depends on run-time decisions...
+                                                    UINT32  cbSize;
 
-    UINT32                  nBits;                  // # bits in divisor
-    SYMCRYPT_MAGIC_FIELD
+                                                    UINT32  nBits;      // # bits in divisor
 
+    SYMCRYPT_MAGIC_FIELD
     union{
         struct {
             UINT64                  W;              // approximate inverse of the divisor. Some implementations will use 64 bits, others 32 bits.
@@ -1727,12 +1731,12 @@ SYMCRYPT_ASYM_ALIGN_STRUCT _SYMCRYPT_DIVISOR {
 #define SYMCRYPT_FDEF_SIZEOF_DIVISOR_FROM_BITS( _bits ) SYMCRYPT_FDEF_SIZEOF_DIVISOR_FROM_DIGITS( SYMCRYPT_FDEF_DIGITS_FROM_BITS( _bits ))
 
 SYMCRYPT_ASYM_ALIGN_STRUCT _SYMCRYPT_MODULUS {
-    UINT32                  type;
-    UINT32                  nDigits;                    // digit size depends on run-time decisions...
-    UINT32                  cbSize;                     // Size of modulus object
+                                                    UINT32  type;
+    _Field_range_( 1, SYMCRYPT_FDEF_UPB_DIGITS )    UINT32  nDigits;        // digit size depends on run-time decisions...
+                                                    UINT32  cbSize;         // Size of modulus object
 
-    UINT32                  flags;          // The flag the modulus was created with
-    UINT32                  cbModElement;   // size of one modElement
+                                                    UINT32  flags;          // The flag the modulus was created with
+                                                    UINT32  cbModElement;   // size of one modElement
 
     SYMCRYPT_MAGIC_FIELD
     union{

inc/symcrypt_low_level.h

@@ -124,10 +124,10 @@ in the current version). This bound is used to ensure that no object sizes and s
 have a value of magnitude more than 32 bits. Note that the computed upper bounds are very loose and the
 actual values are much smaller.
 
-Functions calls and arithmetic operations with values exceeding this bound will trigger a hard fail on the
-calling application. The rationale behind accepting this approach is that executing arithmetic operations
-with integers of such magnitude will consume so much CPU time that it will be indistinguishable from
-an application hang.
+Attempts to create objects larger than this bound will result in NULL being returned. Callers either have
+to ensure they do not exceed the bounds, or check that create objects are not NULL before using them. The
+rationale behind this approach is to avoid any potential route for malicious inputs to trigger DoS by
+taking excessive CPU time which would be indistinguishable from an application hang.
 
 
 Digit size and radix can vary widely; on some CPU steppings the library might use a digit that contains
@@ -191,7 +191,7 @@ a ModElement object.
 //  The object will be able to store values up to R^nDigits where R is the digit radix.
 //  Requirement:
 //      - 1 <= nDigits <= SymCryptDigitsFromBits(SYMCRYPT_INT_MAX_BITS)
-//          If the value is outside these bounds the call will trigger a hard fail.
+//          If the value is outside these bounds it will return NULL
 //      - cbBuffer >= SymCryptSizeofXxxFromDigits( nDigits )
 //      - (pbBuffer,cbBuffer) memory must be exclusively used by this object.
 //  The last requirement ensures that all objects are non-overlapping (except for API functions
@@ -239,8 +239,8 @@ a ModElement object.
 //  Memory size that is sufficient to store an XXX object with nDigits digits.
 //  This is a runtime function as the # digits and size of a digit are run-time decision that depend on the CPU stepping.
 //  Requirement:
-//      - nDigits <= SymCryptDigitsFromBits(SYMCRYPT_INT_MAX_BITS)
-//          If the value is outside these bounds the call will trigger a hard fail.
+//      - 1 <= nDigits <= SymCryptDigitsFromBits(SYMCRYPT_INT_MAX_BITS)
+//          If the value is outside these bounds the returned value will be 0 indicating failure.
 //  This function is has the following property:
 //    SymCryptSizeofXxxFromDigits( a + b ) <= SymCryptSizeofXxxFromDigits( a ) + SymCryptSizeofXxxFromDigits( b )
 //  for all a and b.
@@ -274,8 +274,9 @@ a ModElement object.
 //
 
 // The maximum number of bits in any integer value that the library supports. If the
-// caller's input exceed this bound then the result of the application will be
-// to hard fail.
+// caller's input exceed this bound then the the integer object will not be created.
+// The caller either must ensure the bound is not exceeded, or check for NULL before
+// using created SymCrypt objects.
 // The primary purpose of this limit is to avoid integer overlows in size computations.
 // Having a reasonable upper bound avoids all size overflows, even on 32-bit CPUs
 #define SYMCRYPT_INT_MAX_BITS               ((UINT32)(1 << 20))
@@ -289,14 +290,14 @@ SymCryptDigitsFromBits( UINT32 nBits );
 // Remarks:
 //  If nBits==0 the returned number is 1.
 //
+//  If nBits exceeds SYMCRYPT_INT_MAX_BITS the function will return 0 to indicate an object with
+//  this many bits is not supported.
+//
 //  This is a run-time decision; the return value can depend on the exact CPU stepping
 //  the program is running on, or run-time configurations.
-//  It is always true that
+//  For a and b in the range 0..SYMCRYPT_INT_MAX_BITS, it is always true that
 //  SymCryptDigitsFromBits( a + b ) <= SymCryptDigitsFromBits( a ) + SymCryptDigitsFromBits( b )
 //
-//  If nBits exceeds SYMCRYPT_INT_MAX_BITS the function will cause a hard fail
-//  to the calling application.
-//
 
 //========================================================================
 // INT objects
@@ -1139,6 +1140,7 @@ SymCryptIntExtendedGcd(
 //  - Lcm.nDigits >= Src1.nDigits + Src2.nDigits
 //  - InvSrc1ModSrc2.nDigits >= max(Src1.nDigits, Src2.nDigits)     // Future work: Make these bounds Src2 and Src1 respectively.
 //  - InvSrc2ModSrc1.nDigits >= max(Src1.nDigits, Src2.nDigits)
+//  - if piInvSrc2ModSrc1 is not NULL, max( Src1.nDigits, Src2.nDigits ) * 2 <= SymCryptDigitsFromBits(SYMCRYPT_INT_MAX_BITS)
 //  - cbScratch >= SYMCRYPT_SCRATCH_BYTES_FOR_EXTENDED_GCD( max( Src1.nDigits, Src2.nDigits ) )
 //
 // If only one inverse value is needed, it is most efficient to use only InvSrc1ModSrc2.
@@ -1240,10 +1242,11 @@ SymCryptCrtSolve(
 // The number of inputs nCoprimes is public.
 //
 // Requirements:
-//  - nCoprimes >= 2
+//  - nCoprimes == 2
 //  - ppmCoprimes, ppeCrtInverses, and ppeCrtRemainders must be arrays of pointers of exactly nCoprimes elements. All
 //    of them non-NULL.
 //  - piSolution must be large enough to hold the result modulo the product of all the coprimes.
+//  - max( ppmCoprimes[0].nDigits, ppmCoprimes[1].nDigits ) * 2 <= SymCryptDigitsFromBits(SYMCRYPT_INT_MAX_BITS)
 //  - cbScratch >= SYMCRYPT_SCRATCH_BYTES_FOR_CRT_SOLUTION( nDigits ) where nDigits is the maximum number
 //    of digits of the input moduli.
 //
@@ -1259,7 +1262,7 @@ SymCryptCreateTrialDivisionContext( UINT32 nDigits );
 // The Trial division context can be used in multiple threads in parallel.
 // The context should be freed with SymCryptFreeTrialDivisionContext after use.
 // A context can be fairly large (100 kB) so freeing it is important.
-// Returns NULL if out of memory.
+// Returns NULL if out of memory or an invalid digit count is provided.
 //
 
 VOID
@@ -1365,6 +1368,7 @@ SymCryptIntGenerateRandomPrime(
 // Requirements:
 //  - SymCryptIntBitsizeOfValue( piHigh ) <= SymCryptIntBitsizeOfObject(piDst)
 //  - piLow > 3
+//  - Each public exponent must be greater than 0
 //  - 0 <= nPubExp <= SYMCRYPT_RSAKEY_MAX_NUMOF_PUBEXPS
 //  - cbScratch >=  SYMCRYPT_SCRATCH_BYTES_FOR_INT_PRIME_GEN( Dst.nDigits )
 //
@@ -1420,7 +1424,7 @@ SymCryptIntToModulus(
     _Out_writes_bytes_( cbScratch ) PBYTE               pbScratch,
                                     SIZE_T              cbScratch );
 //
-// Create  a modulus from an INT.
+// Create a modulus from an INT.
 // Requirements:
 //  - Src != 0
 //  - Dst.nDigits == Src.nDigits
@@ -1467,7 +1471,7 @@ SymCryptIntToModElement(
 // Dst = Src mod Mod
 // Requirements:
 //  - Dst.nDigits == Mod.nDigits
-//  - piSrc.nDigits <= 2 * MOd.nDigits
+//  - piSrc.nDigits <= 2 * Mod.nDigits
 //  - cbScratch >= SYMCRYPT_SCRATCH_BYTES_FOR_COMMON_MOD_OPERATIONS( Mod.nDigits )
 //
 // Note: the input is limited in size to be no more than twice the modulus size (in digits).
@@ -1613,7 +1617,7 @@ SymCryptModSetRandom(
 //  - cbScratch >= SYMCRYPT_SCRATCH_BYTES_FOR_COMMON_MOD_OPERATIONS( Mod.nDigits )
 //
 // Random value is chosen uniformly from the set of allowed values.
-// By default this function does not return the values 0, 1, or -1.
+// By default this function does not return the values 0, 1, or -1 (see below NOTE for small moduli exception)
 // Flags parameter can signal that these special values are allowed.
 // flags parameter is published.
 //
@@ -1624,8 +1628,19 @@ SymCryptModSetRandom(
 //  SYMCRYPT_FLAG_MODRANDOM_ALLOW_ZERO
 //  SYMCRYPT_FLAG_MODRANDOM_ALLOW_ONE
 //  SYMCRYPT_FLAG_MODRANDOM_ALLOW_MINUSONE
-// Specifying ALLOW_ZERO is only valid if ALLOW_ONE is also specified.
+// Specifying ALLOW_ZERO implies ALLOW_ONE, there is no way to allow 0 and disallow 1.
+//
+// NOTE:
+// For very small moduli (1, 2, and 3), not allowing 0, 1, or -1 by default does not make sense because this would
+// exclude all possible values! Instead the default behavior is to allow -1 for these moduli.
+//  Modulo 1 => return 0 by default
+//  Modulo 2 => return 1 by default
+//      may also return 0 if SYMCRYPT_FLAG_MODRANDOM_ALLOW_ZERO is specified
+//  Modulo 3 => return 2 by default
+//      may also return 1 if SYMCRYPT_FLAG_MODRANDOM_ALLOW_ONE is specified, and
+//      may also return 0 or 1 if SYMCRYPT_FLAG_MODRANDOM_ALLOW_ZERO is specified,
 //
+// Callers relying on not having 0, 1, or -1 are required to pass a larger modulus.
 
 #define SYMCRYPT_FLAG_MODRANDOM_ALLOW_ZERO      (0x01)
 #define SYMCRYPT_FLAG_MODRANDOM_ALLOW_ONE       (0x02)
@@ -1758,8 +1773,8 @@ SymCryptModInv(
 //
 // - pmMod: Modulus, must have the SYMCRYPT_FLAG_MODULUS_PRIME and SYMCRYPT_FLAG_DATA_PUBLIC flag set.
 //      Non-prime or non-public moduli are currently not supported.
-// - peSrc: Source value, modulu pmMod
-// - peDst: Destination value, mod element modulu pmMod
+// - peSrc: Source value, modulo pmMod
+// - peDst: Destination value, mod element modulo pmMod
 // - flags: SYMCRYPT_FLAG_DATA_PUBLIC signals that peSrc is a public value.
 // - pbScratch/cbScratch: scratch space >= SYMCRYPT_SCRATCH_BYTES_FOR_MODINV( nDigits( pmMod ) )
 //
@@ -1816,7 +1831,7 @@ SymCryptModExp(
 
 #define SYMCRYPT_SCRATCH_BYTES_FOR_MODMULTIEXP( _nDigits, _nBases, _nBitsExp ) SYMCRYPT_INTERNAL_SCRATCH_BYTES_FOR_MODMULTIEXP( _nDigits, _nBases, _nBitsExp )
 
-VOID
+SYMCRYPT_ERROR
 SYMCRYPT_CALL
 SymCryptModMultiExp(
     _In_                            PCSYMCRYPT_MODULUS      pmMod,
@@ -2003,7 +2018,11 @@ SIZE_T
 SYMCRYPT_CALL
 SymCryptRoundUpPow2Sizet( SIZE_T v );
 // Round up to the next power of 2
-
+//
+// Requirements:
+//  v <= (SIZE_T_MAX / 2) + 1
+//    i.e. rounding v up to the next power of 2 fits within SIZE_T, so v is
+//    less than or equal to the maximum power of 2 representable in SIZE_T
 
 
 //=====================================================

lib/IEEE802_11SaeCustom.c

@@ -510,7 +510,11 @@ SymCrypt802_11SaeCustomInit(
     while( notFoundMask != 0 || counter < 40 )
     {
         counter += 1;
-        SYMCRYPT_HARD_ASSERT( counter != 0 );
+        if( counter == 0 )
+        {
+            scError = SYMCRYPT_INVALID_ARGUMENT;
+            goto cleanup;
+        }
 
         // pwd-seed = Hmac-sha256( MacA || MacB , Password || counter )
         SymCryptHmacSha256Init( &hmacState, &hmacSeedKey );
@@ -537,7 +541,10 @@ SymCrypt802_11SaeCustomInit(
 
         // Get the pwd-value into an integer
         scError = SymCryptIntSetValue( abValue, sizeof( abValue ), SYMCRYPT_NUMBER_FORMAT_MSB_FIRST, piTmp );
-        SYMCRYPT_HARD_ASSERT( scError == SYMCRYPT_NO_ERROR );
+        if( scError != SYMCRYPT_NO_ERROR )
+        {
+            goto cleanup;
+        }
 
         // Check that it is less than P
         if( !SymCryptIntIsLessThan( piTmp, SymCryptIntFromModulus( pCurve->FMod ) ) )
@@ -581,7 +588,10 @@ SymCrypt802_11SaeCustomInit(
                                             0,
                                             pbScratch,
                                             cbScratch );
-        SYMCRYPT_HARD_ASSERT( scError == SYMCRYPT_NO_ERROR );
+        if( scError != SYMCRYPT_NO_ERROR )
+        {
+            goto cleanup;
+        }
 
         SymCryptEcpointMaskedCopy( pCurve, poPWECandidate, pState->poPWE, solutionMask );
         pState->counter |= (BYTE)(counter & solutionMask);

lib/ScsTable.c

@@ -108,6 +108,8 @@ SymCryptScsTableSetBuffer(
 
 
 C_ASSERT( SYMCRYPT_SCSTABLE_INTERLEAVE_SIZE == 16 || SYMCRYPT_SCSTABLE_INTERLEAVE_SIZE == 32 );
+// check that an interleave size is exactly 4 words
+C_ASSERT( SYMCRYPT_SCSTABLE_INTERLEAVE_SIZE == 4 * sizeof( SYMCRYPT_SCSTABLE_TYPE ) );
 
 VOID
 SYMCRYPT_CALL
@@ -209,9 +211,6 @@ SymCryptScsTableLoadC(
     SYMCRYPT_ASSERT( cbData == pScsTable->elementSize );
     UNREFERENCED_PARAMETER( cbData );
 
-    // check that an interleave size is exactly 4 words
-    SYMCRYPT_HARD_ASSERT(interleaveSize == 4 * sizeof( SYMCRYPT_SCSTABLE_TYPE ));
-
 #if SYMCRYPT_SCSTABLE_USE64
 #define SCS_MASK_EQUAL32( _a, _b )  ( ~(UINT64) ((INT64) ((UINT64)0 - (_a ^ _b)) >> 32 ) )
 #else