Merged PR 10578579: Fixed debug assertion failure in AES-GCM with nonce < 12 bytes when ASM is disabled

mlindgren · mlindgren · commit d5520b898b4c · 2024-04-11T20:04:47.000Z
`SymCryptGHashAppendDataC` has an assertion `SYMCRYPT_ASSERT(cbData &gt;= SYMCRYPT_GF128_BLOCK_SIZE)`, because the function does nothing if the length of the provided data is less than the block size. However, `SymCryptGcmSetNonce` was unconditionally calling `SymCryptGHashAppendData` for the first `(cbNonce - cbNonceRemainder)` bytes of the nonce, even if the nonce was less than the block size. In this case, `(cbNonce - cbNonceRemainder) == 0`, so the assertion fails.

This is not a functional issue, because the subsequent call will append the remaining bytes, padded to the block size. But it does cause an assertion failure in debug builds when ASM is disabled, so it should be fixed.

This wasn't caught earlier because we don't normally run tests on debug builds without ASM optimizations. Since this is a gap in our testing, I also added a new pipeline which will be run nightly and include more thorough testing.

Tested: symcryptunittest, CI
diff --git a/.pipelines/OneBranch.Official.yml b/.pipelines/OneBranch.Official.yml
@@ -0,0 +1,211 @@
+#################################################################################
+#                               OneBranch Pipelines                             #
+# This pipeline was created by EasyStart from a sample located at:              #
+#   https://aka.ms/obpipelines/easystart/samples                                #
+# Documentation:  https://aka.ms/obpipelines                                    #
+# Yaml Schema:    https://aka.ms/obpipelines/yaml/schema                        #
+# Retail Tasks:   https://aka.ms/obpipelines/tasks                              #
+# Support:        https://aka.ms/onebranchsup                                   #
+#################################################################################
+
+trigger: none
+
+variables:
+  CDP_DEFINITION_BUILD_COUNT: $[counter('', 0)] # needed for onebranch.pipeline.version task https://aka.ms/obpipelines/versioning
+  LinuxContainerImage: 'onebranch.azurecr.io/linux/ubuntu-2004:latest' # Docker image which is used to build the project https://aka.ms/obpipelines/containers
+  WindowsContainerImage: 'onebranch.azurecr.io/windows/ltsc2019/vse2022@sha256:a3083215a4675bad774ec88005dcf57436b3423584d0db3e82b6de3f780213ba'
+
+resources:
+  repositories:
+    - repository: templates
+      type: git
+      name: OneBranch.Pipelines/GovernedTemplates
+      ref: refs/heads/main
+
+extends:
+  template: v2/OneBranch.Official.CrossPlat.yml@templates # https://aka.ms/obpipelines/templates
+  parameters:
+    featureFlags:
+      needExceptionForUbuntuUsage: true # Mariner does not currently support all of our cross-compilation targets
+      LinuxHostVersion:
+        Distribution: Ubuntu # Mariner kernel is incompatible with Ubuntu container; causes infinite loop in ASAN https://github.com/actions/runner-images/issues/9524
+    globalSdl: # https://aka.ms/obpipelines/sdl
+      tsa:
+        enabled: false # Disable TSA to force build breaks
+      policheck:
+        break: true # always break the build on policheck issues.
+      binskim:
+        enabled: false # Temporarily disable binskim until we sort out BA2018 errors
+
+    stages:
+    - stage: Set_Version
+      jobs:
+      - job: set_version
+        displayName: Set Version
+        pool:
+          type: linux
+        variables:
+          ob_outputDirectory: $(Build.SourcesDirectory)/bin
+        steps:
+        - task: PythonScript@0
+          displayName: 'Set version number'
+          name: verStep
+          inputs:
+            scriptSource: 'filePath'
+            scriptPath: scripts/version.py
+            arguments: '--devops'
+            workingDirectory: $(Build.SourcesDirectory)
+        - task: onebranch.pipeline.version@1
+          inputs:
+            system: 'Custom'
+            customVersion: '$(verStep.VER_MAJOR).$(verStep.VER_MINOR).$(verStep.VER_PATCH)-$(Build.BuildId)'
+
+    - stage: Build_Windows
+      displayName: Build Windows
+      jobs:
+      - template: .pipelines/templates/build-windows-cmake.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Debug'
+          sign: true
+          additionalArgs: '--test-legacy-impl'
+      - template: .pipelines/templates/build-windows-cmake.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Release'
+          sign: true
+          additionalArgs: '--test-legacy-impl'
+      - template: .pipelines/templates/build-windows-cmake.yml@self
+        parameters:
+          arch: 'X86'
+          config: 'Debug'
+          sign: true
+          additionalArgs: '--test-legacy-impl'
+      - template: .pipelines/templates/build-windows-cmake.yml@self
+        parameters:
+          arch: 'X86'
+          config: 'Release'
+          sign: true
+          additionalArgs: '--test-legacy-impl'
+      - template: .pipelines/templates/build-windows-cmake.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Release'
+          sign: true
+          additionalArgs: '--no-asm'
+          identifier: 'NoAsm'
+      - template: .pipelines/templates/build-windows-cmake.yml@self
+        parameters:
+          arch: 'X86'
+          config: 'Release'
+          sign: true
+          additionalArgs: '--no-asm'
+          identifier: 'NoAsm'
+
+    - stage: Build_Linux
+      displayName: Build Linux
+      jobs:
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Debug'
+          cc: 'gcc'
+          cxx: 'g++'
+          openssl: true
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Sanitize'
+          cc: 'gcc'
+          cxx: 'g++'
+          openssl: true
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Release'
+          cc: 'gcc'
+          cxx: 'g++'
+          openssl: true
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Debug'
+          cc: 'clang'
+          cxx: 'clang++'
+          openssl: true
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Sanitize'
+          cc: 'clang'
+          cxx: 'clang++'
+          openssl: true
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Release'
+          cc: 'clang'
+          cxx: 'clang++'
+          openssl: true
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Release'
+          cc: 'gcc'
+          cxx: 'g++'
+          additionalArgs: '--no-asm'
+          openssl: true
+          identifier: 'NoAsm'
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'AMD64'
+          config: 'Release'
+          cc: 'clang'
+          cxx: 'clang++'
+          additionalArgs: '--no-asm'
+          openssl: true
+          identifier: 'NoAsm'
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'X86'
+          config: 'Release'
+          cc: 'gcc'
+          cxx: 'g++'
+          additionalArgs: '--no-asm --no-fips'
+          identifier: 'NoAsm'
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'X86'
+          config: 'Release'
+          cc: 'clang'
+          cxx: 'clang++'
+          additionalArgs: '--no-asm --no-fips'
+          identifier: 'NoAsm'
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'ARM64'
+          config: 'Debug'
+          cc: 'clang'
+          cxx: 'clang++'
+          additionalArgs: '--toolchain=cmake-configs/Toolchain-Clang-ARM64.cmake'
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'ARM64'
+          config: 'Release'
+          cc: 'clang'
+          cxx: 'clang++'
+          additionalArgs: '--toolchain=cmake-configs/Toolchain-Clang-ARM64.cmake'
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'ARM'
+          config: 'Debug'
+          cc: 'gcc'
+          cxx: 'g++'
+          additionalArgs: '--toolchain=cmake-configs/Toolchain-GCC-ARM.cmake'
+      - template: .pipelines/templates/build-linux.yml@self
+        parameters:
+          arch: 'ARM'
+          config: 'Release'
+          cc: 'gcc'
+          cxx: 'g++'
+          additionalArgs: '--toolchain=cmake-configs/Toolchain-GCC-ARM.cmake'
diff --git a/.pipelines/OneBranch.PullRequest.yml b/.pipelines/OneBranch.PullRequest.yml
@@ -30,6 +30,8 @@ extends:
       LinuxHostVersion:
         Distribution: Ubuntu # Mariner kernel is incompatible with Ubuntu container; causes infinite loop in ASAN https://github.com/actions/runner-images/issues/9524
     globalSdl: # https://aka.ms/obpipelines/sdl
+      tsa:
+        enabled: false # Disable TSA to force build breaks
       policheck:
         break: true # always break the build on policheck issues.
       binskim:
diff --git a/.pipelines/OneBranch.WindowsUndocked.Official.yml b/.pipelines/OneBranch.WindowsUndocked.Official.yml
@@ -69,9 +69,8 @@ extends:
     platform:
       name: 'windows_undocked'
     globalSdl:
-      # Disable TSA to force build breaks
       tsa:
-        enabled: false
+        enabled: false # Disable TSA to force build breaks
     featureFlags:
       WindowsHostVersion: '1ESWindows2022'
     stages:
diff --git a/.pipelines/OneBranch.WindowsUndocked.PullRequest.yml b/.pipelines/OneBranch.WindowsUndocked.PullRequest.yml
@@ -35,9 +35,8 @@ extends:
     platform:
       name: 'windows_undocked'
     globalSdl:
-      # Disable TSA to force build breaks
       tsa:
-        enabled: false
+        enabled: false # Disable TSA to force build breaks
     featureFlags:
       WindowsHostVersion: '1ESWindows2022'
     stages:
diff --git a/.pipelines/templates/build-linux.yml b/.pipelines/templates/build-linux.yml
@@ -178,9 +178,27 @@ jobs:
             workingDirectory: $(Build.SourcesDirectory)
 
     - task: PythonScript@0
-      displayName: 'Package build output'
+      displayName: 'Package generic Linux module'
       inputs:
         scriptSource: 'filePath'
         scriptPath: scripts/package.py
         arguments: 'bin ${{ parameters.arch }} ${{ parameters.config }} generic bin'
         workingDirectory: $(Build.SourcesDirectory)
+
+    - task: Bash@3
+      displayName: 'Check if OpenEnclave module exists'
+      inputs:
+        targetType: inline
+        script: |
+          if [ -d bin/module/oe_full ]; then
+            echo "##vso[task.setVariable variable=PACKAGE_OE]true"
+          fi
+
+    - task: PythonScript@0
+      displayName: 'Package OpenEnclave module'
+      condition: eq(variables.PACKAGE_OE, 'true')
+      inputs:
+        scriptSource: 'filePath'
+        scriptPath: scripts/package.py
+        arguments: 'bin ${{ parameters.arch }} ${{ parameters.config }} oe_full bin'
+        workingDirectory: $(Build.SourcesDirectory)
diff --git a/.pipelines/templates/build-windows-cmake.yml b/.pipelines/templates/build-windows-cmake.yml
@@ -20,6 +20,9 @@ parameters:
 - name: identifier # Additional identifier for job name
   type: string
   default: ''
+- name: sign # Sign user-mode binaries. Required by Guardian, even though we don't publish Windows binaries from CMake
+  type: boolean
+  default: false
 
 jobs:
 
@@ -71,3 +74,12 @@ jobs:
         scriptPath: scripts\test.py
         arguments: 'bin dynamic:bin\exe\symcrypttestmodule.dll noperftests'
         workingDirectory: $(Build.SourcesDirectory)
+
+    - task: onebranch.pipeline.signing@1 # https://aka.ms/obpipelines/signing
+      displayName: 'Sign Binaries'
+      condition: eq('${{ parameters.sign }}', true)
+      inputs:
+        command: 'sign'
+        signing_profile: 'external_distribution'
+        files_to_sign: '**\*.exe;**\*.dll' # Only supports user mode binaries
+        search_root: 'bin'
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ prior to the creation of a new release, based on the changes contained in that r
 - Fix tweak lower 64 bit overflow calculation in SYMCRYPT_XtsAesXxx
 - Add OpenSSL implementation for XtsAes to symcryptunittest
 - Add Windows user mode DLL
+- Fixed debug assertion failure in AES-GCM with nonce < 12 bytes when ASM is disabled
 
 # Version 103.4.1
 - Add retpoline guard flags for undocked Windows build
diff --git a/lib/gcm.c b/lib/gcm.c
@@ -353,8 +353,11 @@ SymCryptGcmSetNonce(
         BYTE buf[SYMCRYPT_GF128_BLOCK_SIZE];
         SIZE_T cbNonceRemainder = cbNonce & 0xf;
         
-        SymCryptGHashAppendData( &pState->pKey->ghashKey, &pState->ghashState, pbNonce,
-            cbNonce - cbNonceRemainder );
+        if(cbNonce >= SYMCRYPT_GF128_BLOCK_SIZE)
+        {
+            SymCryptGHashAppendData( &pState->pKey->ghashKey, &pState->ghashState, pbNonce,
+                cbNonce - cbNonceRemainder );
+        }
 
         // If the nonce length is not a multiple of 128 bits, it needs to be padded with zeros
         // until it is, as GHASH is only defined on multiples of 128 bits.
