Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE]

[Laszlo Ersek <lersek@redhat.com>]

Hi All,

On 10/16/18 04:41, Star Zeng wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=415
>
> When SetVariable() to a time based auth variable with APPEND_WRITE
> attribute, and if the EFI_VARIABLE_AUTHENTICATION_2.TimeStamp in
> the input Data is earlier than current value, it will cause timestamp
> zeroing.
>
> This issue may bring time based auth variable downgrade problem.
> For example:
> A vendor released three certs at 2014, 2015, and 2016, and system
> integrated the 2016 cert. User can SetVariable() with 2015 cert and
> APPEND_WRITE attribute to cause timestamp zeroing first, then
> SetVariable() with 2014 cert to downgrade the cert.
>
> This patch fixes this issue.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index a2d61c8cd618..8e8db71bd201 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -2462,6 +2462,8 @@ UpdateVariable (
>          if (Variable->CurrPtr != NULL) {
>            if (VariableCompareTimeStampInternal (&(((AUTHENTICATED_VARIABLE_HEADER *) CacheVariable->CurrPtr)->TimeStamp), TimeStamp)) {
>              CopyMem (&AuthVariable->TimeStamp, TimeStamp, sizeof (EFI_TIME));
> +          } else {
> +            CopyMem (&AuthVariable->TimeStamp, &(((AUTHENTICATED_VARIABLE_HEADER *) CacheVariable->CurrPtr)->TimeStamp), sizeof (EFI_TIME));
>            }
>          }
>        }
>

I believe I found a significant mitigating factor for this
vulnerability.

(i) I tried to reproduce the issue (with this patch reverted). I indeed
managed to trigger the "timestamp zeroed" case, by *appending* a
hypothetical "2015" DBX update, to a virtual system that had the "2016"
DBX update installed first.

However, when I tried to replay the hypothetical "2014" DBX update on
top, by *writing* it (not appending it), I found that it wouldn't work:


(ii) I confirmed that the timestamp check was in fact circumvented, due
to the zeroing above. That is, the following code snippet from
VerifyTimeBasedPayload() would *not* fire:

[SecurityPkg/Library/AuthVariableLib/AuthService.c]

  1869    if ((OrgTimeStamp != NULL) && ((Attributes & EFI_VARIABLE_APPEND_WRITE) == 0)) {
  1870      if (AuthServiceInternalCompareTimeStamp (&CertData->TimeStamp, OrgTimeStamp)) {
  1871        //
  1872        // TimeStamp check fail, suspicious replay attack, return EFI_SECURITY_VIOLATION.
  1873        //
  1874        return EFI_SECURITY_VIOLATION;
  1875      }
  1876    }

(Line numbers correspond to commit 3a0329bed2a2).

Yet the replay attempt was rejected. Why?


(iii) It was rejected on the following call chain:

  VariableServiceSetVariable()           [MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c]
    AuthVariableLibProcessVariable()     [SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c]
      ProcessVarWithKek()                [SecurityPkg/Library/AuthVariableLib/AuthService.c]
       VerifyTimeBasedPayloadAndUpdate() [SecurityPkg/Library/AuthVariableLib/AuthService.c]
         VerifyTimeBasedPayload()        [SecurityPkg/Library/AuthVariableLib/AuthService.c]
           Pkcs7Verify()                 [CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c]

[SecurityPkg/Library/AuthVariableLib/AuthService.c]

  2032      //
  2033      // Ready to verify Pkcs7 SignedData. Go through KEK Signature Database to find out X.509 CertList.
  2034      //
  2035      KekDataSize      = (UINT32) DataSize;
  2036      CertList         = (EFI_SIGNATURE_LIST *) Data;
  2037      while ((KekDataSize > 0) && (KekDataSize >= CertList->SignatureListSize)) {
  2038        if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
  2039          Cert       = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
  2040          CertCount  = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize;
  2041          for (Index = 0; Index < CertCount; Index++) {
  2042            //
  2043            // Iterate each Signature Data Node within this CertList for a verify
  2044            //
  2045            TrustedCert      = Cert->SignatureData;
  2046            TrustedCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
  2047
  2048            //
  2049            // Verify Pkcs7 SignedData via Pkcs7Verify library.
  2050            //
  2051            VerifyStatus = Pkcs7Verify (
  2052                             SigData,
  2053                             SigDataSize,
  2054                             TrustedCert,
  2055                             TrustedCertSize,
  2056                             NewData,
  2057                             NewDataSize
  2058                             );
  2059            if (VerifyStatus) {
  2060              goto Exit;
  2061            }
  2062            Cert = (EFI_SIGNATURE_DATA *) ((UINT8 *) Cert + CertList->SignatureSize);
  2063          }
  2064        }
  2065        KekDataSize -= CertList->SignatureListSize;
  2066        CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList->SignatureListSize);
  2067      }

The Pkcs7Verify() call on line 2051 would never return TRUE, even though
the CA certificate against which the "2014" DBX update had been released
was in KEK.

So I hexdumped SigData, TrustedCert, and NewData, and compared them
against a (successful) *append* call.

The difference is a single bit in NewData, and Pkcs7Verify() is right to
reject it.


(iv) It is explained in the UEFI-2.7 spec, section "8.2.2 Using the
EFI_VARIABLE_AUTHENTICATION_2 descriptor":

> A caller that invokes the SetVariable() service with the
> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS attribute set shall
> do the following prior to invoking the service:
>
> [...]
>
> 2. Hash the serialization of the values of the VariableName,
>    VendorGuid and Attributes parameters of the SetVariable() call and
>    the TimeStamp component of the EFI_VARIABLE_AUTHENTICATION_2
>    descriptor followed by the variable's new value (i.e. the Data
>    parameter's new variable content). That is, digest = hash
>    (VariableName, VendorGuid, Attributes, TimeStamp,
>    DataNew_variable_content). [...]
>
> 3. Sign the resulting digest using a selected signature scheme (e.g.
>    PKCS #1 v1.5)
>
> 4. Construct a DER-encoded PKCS #7 version 1.5 SignedData (see
>    [RFC2315]) with the signed content as follows:
>
> [...]
>
> 5. Set AuthInfo.CertData to the DER-encoded PKCS #7 SignedData value.
>
> 6. Construct Data parameter: Construct the SetVariable()'s Data
>    parameter by concatenating the complete, serialized
>    EFI_VARIABLE_AUTHENTICATION_2 descriptor with the new value of the
>    variable (DataNew_variable_content).

Note that in step 2, the *Attributes* parameter of the SetVariable()
call is an input to the hash, and so it is an input to the signature
that the vendor places into the EFI_VARIABLE_AUTHENTICATION_2
descriptor's AuthInfo field in step 5.

In other words, when the vendor signs the DBX update, they sign the
expected Attributes value as well.


(v) Now compare the verification side:

> Firmware that implements the SetVariable() service and supports the
> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS attribute shall do
> the following in response to being called:
>
> [...]
>
> 4. Verify the signature by:
>    - extracting the EFI_VARIABLE_AUTHENTICATION_2 descriptor from the
>      Data buffer;
>    - using the descriptor contents and other parameters to (a)
>      construct the input to the digest algorithm; (b) computing the
>      digest; and (c) comparing the digest with the result of applying
>      the signer's public key to the signature.

That is, when recomputing the digest, the Attributes parameter of the
replay attempt is hashed *afresh* with the rest of the payload, and the
new digest is verified against the vendor's. Consider what the full
payload comprises, for the purpose of hashing:
- VariableName: fixed (otherwise the attack doesn't qualify as "replay")
- VendorGuid: fixed (ditto)
- Attributes: applied afresh, from the call parameters!
- TimeStamp: part of the vendor-released blob (from the auth descriptor)
- DataNew_variable_content: part of the vendor-released blob


         >------------->----------->------------->---------v---------------<
         |             |           |             |         |               |
         ^             ^           ^             ^         v               ^
 | VariableName | VendorGuid | Attributes | TimeStamp | Signature | New Var Content |
 |                                        |                       |                 |
 |                                        +--- auth descriptor ---+                 |
 |                                        |                                         |
 |                                        +------------- vendor blob ---------------+
 |                                                                                  |
 +----------------------------------- call params ----------------------------------+


(vi) In practice, a vendor should always release a DBX update with
EFI_VARIABLE_APPEND_WRITE set, in Attributes. This is because Vendor1
should never intend the user to lose Vendor2's DBX entries, when the
user applies Vendor1's DBX update. This means that, in practice,
Vendor1's signature should always depend on Attributes having
EFI_VARIABLE_APPEND_WRITE set.

However, exploiting this vulnerability (= the replay of the "2014" DBX
update) depends on EFI_VARIABLE_APPEND_WRITE being *clear*. And when the
"2014" DBX update is replayed like that, then the vendor's signature on
the same "2014" update will no longer match.

This makes me wonder if this vulnerability is exploitable at all in
practice. Please share your thoughts.

Thanks
Laszlo