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

["Zeng, Star" <star.zeng@intel.com>]

Hi Laszlo,

Cc Qin also. Qin and Chao are secure boot experts, I also had some talk 
with them.

On 2018/10/19 5:45, Laszlo Ersek wrote:
> 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.

Very good analysis, I totally agree. :)

Yes, if the dbx signature(includes the "attribute" information) was 
generated with "APPEND" attribute (that is the case you are seeing), 
it's infeasible to apply the downgrade write since the signature 
includes the "attribute" information, the PKCS7 verification will block 
the direct write without "APPEND" attribute.

But there may be some initial dbx signature was generated without 
"APPEND" attribute. E.g. OEM may have some this kind of dbx. It should 
be rarely case, but I am not sure about that.

Another, similar situation is also for other authenticated variables 
(not PK/KEK/DB/DBX/DBT).


Thanks,
Star

> 
> (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
>