* [PATCH] MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE @ 2018-10-16 2:41 Star Zeng 2018-10-16 7:03 ` Yao, Jiewen 2018-10-17 13:10 ` CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] Laszlo Ersek 0 siblings, 2 replies; 12+ messages in thread From: Star Zeng @ 2018-10-16 2:41 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Chao Zhang, Jian J Wang 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)); } } } -- 2.7.0.windows.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE 2018-10-16 2:41 [PATCH] MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE Star Zeng @ 2018-10-16 7:03 ` Yao, Jiewen 2018-10-17 13:10 ` CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] Laszlo Ersek 1 sibling, 0 replies; 12+ messages in thread From: Yao, Jiewen @ 2018-10-16 7:03 UTC (permalink / raw) To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Zhang, Chao B, Wang, Jian J Reviewed-by: Jiewen.yao@intel.com > -----Original Message----- > From: Zeng, Star > Sent: Tuesday, October 16, 2018 10:41 AM > To: edk2-devel@lists.01.org > Cc: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; > Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian J > <jian.j.wang@intel.com> > Subject: [PATCH] MdeModulePkg Variable: Fix Timestamp zeroing issue on > APPEND_WRITE > > 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)); > } > } > } > -- > 2.7.0.windows.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-16 2:41 [PATCH] MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE Star Zeng 2018-10-16 7:03 ` Yao, Jiewen @ 2018-10-17 13:10 ` Laszlo Ersek 2018-10-17 14:58 ` Zeng, Star 2018-10-18 21:45 ` Laszlo Ersek 1 sibling, 2 replies; 12+ messages in thread From: Laszlo Ersek @ 2018-10-17 13:10 UTC (permalink / raw) To: Star Zeng, Prasad Pandit Cc: edk2-devel, Jiewen Yao, Chao Zhang, Leif Lindholm (Linaro address), Vincent Zimmer, Michael Kinney, Gary Lin, Ard Biesheuvel, Steve McIntyre, Peter Jones Hi Star, 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)); > } > } > } > thank you for the BZ reference in the commit message. The commit message is very good, and from it, I suspected this was a security bug -- it makes "dbx" rollbacks possible, correct? --, and I was wondering if it should have received a CVE. Indeed, checking the TianoCore BZ, I can see that this patch mitigates CVE-2018-3613. I have requested earlier [1], and now I'm doing so again, that CVE fixes please all mention the CVE number in the *subject line*. When people look at the commit log, or even just patch traffic on this list, CVE numbers should *jump* at them. http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com Because you pushed this patch in ~25 hours after posting it to the public list, and because TianoCore BZ#415 used to be a security bug (restricted from mirroring to the bugzilla list, and opened up likely most recently only), I couldn't comment on the subject line (I was on PTO yesterday), and now we have another patch in the git history that is a CVE fix, but states that fact nowhere at all. To be clear, my complaint is not that the patch was pushed too quickly (one day should be fine for CVEs after coordinated disclosure); my point is that the patch was pushed quickly *and* it never mentioned it was a CVE fix (in the subject line specifically). In addition, while the bugzilla states: > The issue is there since the auth variable driver was created in > SecurityPkg, and it is inherited to current variable driver in > MdeModulePkg after the auth variable driver in SecurityPkg was merged > to variable driver in MdeModulePkg. some specific commit references in the fix's commit message would have helped, so that everyone could evaluate whether they were affected. --*-- Process-wise, I'm sad that Red Hat -- and likely many other organizations shipping edk2-based firmware -- have not been involved in a coordinated disclosure around this issue. The timeline in https://bugzilla.tianocore.org/show_bug.cgi?id=415 suggests that there would have been a lot of time for this (and apperently there was *intent* too). But here we are, caught with our pants around our ankles. Prasad, to my understanding, you are Red Hat's representative on the TianoCore Bugzilla security group. I've now searched the RH Bugzilla for "CVE-2018-3613", and there are no hits. Can you please confirm whether this BZ was made available to us (and we missed it, and/or failed to act upon it otherwise)? Either way, please: - Create the appropriate tracker in the Red Hat Bugzilla. (The patch has been picked to UDK as far back as UDK2015; we obviously need to fix this yesterday.) - Forward the issue to <https://seclists.org/oss-sec/>, so that other organizations that distribute OVMF learn of this. (I'm adding a few direct CC's now, but that list shouldn't be limited by my imagination. I've briefly searched the oss-sec archive as well: also no hits.) Thank you, Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-17 13:10 ` CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] Laszlo Ersek @ 2018-10-17 14:58 ` Zeng, Star 2018-10-17 18:27 ` Laszlo Ersek 2018-10-18 21:45 ` Laszlo Ersek 1 sibling, 1 reply; 12+ messages in thread From: Zeng, Star @ 2018-10-17 14:58 UTC (permalink / raw) To: Laszlo Ersek, Prasad Pandit Cc: Vincent Zimmer, edk2-devel, Steve McIntyre, Peter Jones, Jiewen Yao, Michael Kinney, Gary Lin, Chao Zhang, star.zeng Hi Laszlo, On 2018/10/17 21:10, Laszlo Ersek wrote: > Hi Star, > > 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)); >> } >> } >> } >> > > thank you for the BZ reference in the commit message. > > The commit message is very good, and from it, I suspected this was a > security bug -- it makes "dbx" rollbacks possible, correct? --, and I > was wondering if it should have received a CVE. Yes, your are right. You have known there is a CVE for it. > > Indeed, checking the TianoCore BZ, I can see that this patch mitigates > CVE-2018-3613. > > I have requested earlier [1], and now I'm doing so again, that CVE fixes > please all mention the CVE number in the *subject line*. When people > look at the commit log, or even just patch traffic on this list, CVE > numbers should *jump* at them. Good request. How about we document it as requirement at somewhere (Contributions.txt?)? Then people can easily find the requirement and follow it. > > http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com Sorry, I could not access it. > > Because you pushed this patch in ~25 hours after posting it to the > public list, and because TianoCore BZ#415 used to be a security bug > (restricted from mirroring to the bugzilla list, and opened up likely > most recently only), I couldn't comment on the subject line (I was on > PTO yesterday), and now we have another patch in the git history that is > a CVE fix, but states that fact nowhere at all. I was unlucky and I am sad that I could not receive your feedback/comment before I pushed the patch. :( From TianoCore BZ#415, we can see the original embargoed data was "10/26/17". For some reason, it was extended to "July 10, 2018". I supposed some coordinator(s) should have coordinated with organizations for this CVE before its disclosure. I was just aware that Security Advisory including this CVE at https://edk2-docs.gitbooks.io/security-advisory/ was released at "Oct 12, 2018" and TianoCore BZ#415 link was just made public before I posted the patch. I thought I should make the patch into the code as quick as possible (after following the community code review process) after this CVE's disclosure. > > To be clear, my complaint is not that the patch was pushed too quickly > (one day should be fine for CVEs after coordinated disclosure); my point > is that the patch was pushed quickly *and* it never mentioned it was a > CVE fix (in the subject line specifically). Got it. I should have done like this if I was aware the request.:( > > In addition, while the bugzilla states: > >> The issue is there since the auth variable driver was created in >> SecurityPkg, and it is inherited to current variable driver in >> MdeModulePkg after the auth variable driver in SecurityPkg was merged >> to variable driver in MdeModulePkg. > > some specific commit references in the fix's commit message would have > helped, so that everyone could evaluate whether they were affected. We can see the attachment for reference in TianoCore BZ#415 link only updates the variable driver in MdeModulePkg and we just synced the patch to UDK2018/UDK2017/UDK2015 which all have SecurityPkg variable driver merged into MdeModulePkg variable driver. SecurityPkg variable driver is just present in very old UDK branches. And people is not hard to know the history of MdeModulePkg and SecurityPkg variable driver by GIT log. So I left the statement in TianoCore BZ#415 link. Yes, I admit it would be better also including some statement/reference in commit log. :) Really thanks very much for the comments. Star > > --*-- > > Process-wise, I'm sad that Red Hat -- and likely many other > organizations shipping edk2-based firmware -- have not been involved in > a coordinated disclosure around this issue. The timeline in > > https://bugzilla.tianocore.org/show_bug.cgi?id=415 > > suggests that there would have been a lot of time for this (and > apperently there was *intent* too). But here we are, caught with our > pants around our ankles. > > Prasad, to my understanding, you are Red Hat's representative on the > TianoCore Bugzilla security group. I've now searched the RH Bugzilla for > "CVE-2018-3613", and there are no hits. Can you please confirm whether > this BZ was made available to us (and we missed it, and/or failed to act > upon it otherwise)? > > Either way, please: > > - Create the appropriate tracker in the Red Hat Bugzilla. (The patch has > been picked to UDK as far back as UDK2015; we obviously need to fix > this yesterday.) > > - Forward the issue to <https://seclists.org/oss-sec/>, so that other > organizations that distribute OVMF learn of this. > > (I'm adding a few direct CC's now, but that list shouldn't be limited by > my imagination. I've briefly searched the oss-sec archive as well: also > no hits.) > > Thank you, > Laszlo > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-17 14:58 ` Zeng, Star @ 2018-10-17 18:27 ` Laszlo Ersek 2018-10-18 2:45 ` Zeng, Star 0 siblings, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2018-10-17 18:27 UTC (permalink / raw) To: Zeng, Star, Prasad Pandit Cc: Vincent Zimmer, edk2-devel, Steve McIntyre, Peter Jones, Jiewen Yao, Michael Kinney, Gary Lin, Chao Zhang, Cetola, Stephano +Stephano On 10/17/18 16:58, Zeng, Star wrote: > On 2018/10/17 21:10, Laszlo Ersek wrote: >> I have requested earlier [1], and now I'm doing so again, that CVE fixes >> please all mention the CVE number in the *subject line*. When people >> look at the commit log, or even just patch traffic on this list, CVE >> numbers should *jump* at them. > > Good request. How about we document it as requirement at somewhere > (Contributions.txt?)? Then people can easily find the requirement and > follow it. I agree, we should have documented it somewhere explicitly. Stephano, can you please add a note to the "well-formed commit messages" topic that CVE number should be documented in the subject lines? My apologies for not thinking about this earlier. >> http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com >> > > Sorry, I could not access it. I'm unsure if you mean that you didn't see that message when I posted it, or else that you've now tried to follow the link, but it doesn't work for you. Does the official edk2-devel archive work perhaps? Here's a link within that, to the same message: https://lists.01.org/pipermail/edk2-devel/2018-August/028700.html Please see my request (1). Either way -- I totally agree this hasn't been documented appropriately before. Thanks Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-17 18:27 ` Laszlo Ersek @ 2018-10-18 2:45 ` Zeng, Star 2018-10-18 13:09 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Zeng, Star @ 2018-10-18 2:45 UTC (permalink / raw) To: Laszlo Ersek, Prasad Pandit Cc: Vincent Zimmer, edk2-devel, Cetola, Stephano, Steve McIntyre, Peter Jones, Jiewen Yao, Michael Kinney, Gary Lin, Chao Zhang, star.zeng Hi Laszlo, On 2018/10/18 2:27, Laszlo Ersek wrote: > +Stephano > > On 10/17/18 16:58, Zeng, Star wrote: >> On 2018/10/17 21:10, Laszlo Ersek wrote: > >>> I have requested earlier [1], and now I'm doing so again, that CVE fixes >>> please all mention the CVE number in the *subject line*. When people >>> look at the commit log, or even just patch traffic on this list, CVE >>> numbers should *jump* at them. >> >> Good request. How about we document it as requirement at somewhere >> (Contributions.txt?)? Then people can easily find the requirement and >> follow it. > > I agree, we should have documented it somewhere explicitly. > > Stephano, can you please add a note to the "well-formed commit messages" > topic that CVE number should be documented in the subject lines? My > apologies for not thinking about this earlier. I will be glad to help broadcast this request and direct people to that document. :) > >>> http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com >>> >> >> Sorry, I could not access it. > > I'm unsure if you mean that you didn't see that message when I posted > it, or else that you've now tried to follow the link, but it doesn't > work for you. Does the official edk2-devel archive work perhaps? Here's > a link within that, to the same message: > > https://lists.01.org/pipermail/edk2-devel/2018-August/028700.html The edk2-devel archive link works for me. But I did not review this thread and did not see the request. :( FYI, I could not access the redhat archive link http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com, I just heard some other people also could not access it. Thanks, Star > > Please see my request (1). > > Either way -- I totally agree this hasn't been documented appropriately > before. > > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-18 2:45 ` Zeng, Star @ 2018-10-18 13:09 ` Laszlo Ersek 2018-10-18 13:43 ` Zeng, Star 0 siblings, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2018-10-18 13:09 UTC (permalink / raw) To: Zeng, Star, Prasad Pandit Cc: Vincent Zimmer, edk2-devel, Cetola, Stephano, Steve McIntyre, Peter Jones, Jiewen Yao, Michael Kinney, Gary Lin, Chao Zhang On a tangent: On 10/18/18 04:45, Zeng, Star wrote: > On 2018/10/18 2:27, Laszlo Ersek wrote: >>>> http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com >>> Sorry, I could not access it. >> >> I'm unsure if you mean that you didn't see that message when I posted >> it, or else that you've now tried to follow the link, but it doesn't >> work for you. Does the official edk2-devel archive work perhaps? Here's >> a link within that, to the same message: >> >> https://lists.01.org/pipermail/edk2-devel/2018-August/028700.html > > The edk2-devel archive link works for me. But I did not review this > thread and did not see the request. :( OK, understood. > FYI, I could not access the redhat archive link > http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com, > I just heard some other people also could not access it. That link isn't a "Red Hat" link. It is a link that points to mid.mail-archive.com The site "mid.mail-archive.com" is a search service from mail-archive.com. The URL is composed as follows: mid.mail-archive.com + "/" + <Message-Id> In the <Message-Id> part, the user can place the message-id header of the email that they are looking for. In the current case, the Message-Id of the email that I wanted to direct you to was: e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com It ends with "@redhat.com" only because the message-id was originally generated by a RH SMTP server (because the message was sent by me). So, the complete link is not a "Red Hat" link; it is a mail-archive.com link that happens to end with "@redhat.com" -- because the message ID that I put in the URL, for the search service, ends with "@redhat.com". ... Anyway, I understand now that mail-archive.com may not be accessible from behind the Great Firewall. I'll try to keep in mind to provide both edk2-devel archive links, and mail-archive.com links. (Normally I prefer mail-archive.com because it shows the thread structure much better.) Thanks! Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-18 13:09 ` Laszlo Ersek @ 2018-10-18 13:43 ` Zeng, Star 2018-10-18 16:04 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Zeng, Star @ 2018-10-18 13:43 UTC (permalink / raw) To: Laszlo Ersek, Prasad Pandit Cc: Vincent Zimmer, edk2-devel, Cetola, Stephano, Steve McIntyre, Peter Jones, Jiewen Yao, Michael Kinney, Gary Lin, Chao Zhang, star.zeng Hi Laszlo, On 2018/10/18 21:09, Laszlo Ersek wrote: > On a tangent: > > On 10/18/18 04:45, Zeng, Star wrote: >> On 2018/10/18 2:27, Laszlo Ersek wrote: > >>>>> http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com > >>>> Sorry, I could not access it. >>> >>> I'm unsure if you mean that you didn't see that message when I posted >>> it, or else that you've now tried to follow the link, but it doesn't >>> work for you. Does the official edk2-devel archive work perhaps? Here's >>> a link within that, to the same message: >>> >>> https://lists.01.org/pipermail/edk2-devel/2018-August/028700.html >> >> The edk2-devel archive link works for me. But I did not review this >> thread and did not see the request. :( > > OK, understood. > >> FYI, I could not access the redhat archive link >> http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com, >> I just heard some other people also could not access it. > > That link isn't a "Red Hat" link. It is a link that points to > > mid.mail-archive.com > > The site "mid.mail-archive.com" is a search service from > mail-archive.com. The URL is composed as follows: > > mid.mail-archive.com + "/" + <Message-Id> > > In the <Message-Id> part, the user can place the message-id header of > the email that they are looking for. > > In the current case, the Message-Id of the email that I wanted to direct > you to was: > > e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com > > It ends with "@redhat.com" only because the message-id was originally > generated by a RH SMTP server (because the message was sent by me). So, > the complete link is not a "Red Hat" link; it is a mail-archive.com link > that happens to end with "@redhat.com" -- because the message ID that I > put in the URL, for the search service, ends with "@redhat.com". I am not familiar with using mail-archive.com. I just did some check and found it is interesting that I could access https://www.mail-archive.com/edk2-devel@lists.01.org/msg43513.html, it is because it has "https"? Not sure. Thanks, Star > > .... Anyway, I understand now that mail-archive.com may not be accessible > from behind the Great Firewall. I'll try to keep in mind to provide both > edk2-devel archive links, and mail-archive.com links. (Normally I prefer > mail-archive.com because it shows the thread structure much better.) > > Thanks! > Laszlo > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-18 13:43 ` Zeng, Star @ 2018-10-18 16:04 ` Laszlo Ersek 0 siblings, 0 replies; 12+ messages in thread From: Laszlo Ersek @ 2018-10-18 16:04 UTC (permalink / raw) To: Zeng, Star, Prasad Pandit Cc: Vincent Zimmer, edk2-devel, Cetola, Stephano, Steve McIntyre, Peter Jones, Jiewen Yao, Michael Kinney, Gary Lin, Chao Zhang On 10/18/18 15:43, Zeng, Star wrote: > Hi Laszlo, > > On 2018/10/18 21:09, Laszlo Ersek wrote: >> On a tangent: >> >> On 10/18/18 04:45, Zeng, Star wrote: >>> On 2018/10/18 2:27, Laszlo Ersek wrote: >> >>>>>> http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com >>>>>> >> >>>>> Sorry, I could not access it. >>>> >>>> I'm unsure if you mean that you didn't see that message when I posted >>>> it, or else that you've now tried to follow the link, but it doesn't >>>> work for you. Does the official edk2-devel archive work perhaps? Here's >>>> a link within that, to the same message: >>>> >>>> https://lists.01.org/pipermail/edk2-devel/2018-August/028700.html >>> >>> The edk2-devel archive link works for me. But I did not review this >>> thread and did not see the request. :( >> >> OK, understood. >> >>> FYI, I could not access the redhat archive link >>> http://mid.mail-archive.com/e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com, >>> >>> I just heard some other people also could not access it. >> >> That link isn't a "Red Hat" link. It is a link that points to >> >> mid.mail-archive.com >> >> The site "mid.mail-archive.com" is a search service from >> mail-archive.com. The URL is composed as follows: >> >> mid.mail-archive.com + "/" + <Message-Id> >> >> In the <Message-Id> part, the user can place the message-id header of >> the email that they are looking for. >> >> In the current case, the Message-Id of the email that I wanted to direct >> you to was: >> >> e62f7104-e341-6c7f-1af5-2130f161f111@redhat.com >> >> It ends with "@redhat.com" only because the message-id was originally >> generated by a RH SMTP server (because the message was sent by me). So, >> the complete link is not a "Red Hat" link; it is a mail-archive.com link >> that happens to end with "@redhat.com" -- because the message ID that I >> put in the URL, for the search service, ends with "@redhat.com". > > I am not familiar with using mail-archive.com. > I just did some check and found it is interesting that I could access > https://www.mail-archive.com/edk2-devel@lists.01.org/msg43513.html, it > is because it has "https"? Not sure. That's a good point; I think it could very well be the reason. Unfortunately, the message-id based search at mail-archive.com (that is, the domain "mid.mail-archive.com") does *not* work with HTTPS. It is regrettable, it has always annoyed me. I've tried the same URLs via the HTTPS scheme in the past, and they've never worked. (They aren't working right now either.) Otherwise I'd have posted HTTPS links on every occasion. Thanks for raising this! Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-17 13:10 ` CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] Laszlo Ersek 2018-10-17 14:58 ` Zeng, Star @ 2018-10-18 21:45 ` Laszlo Ersek 2018-10-19 7:09 ` Zeng, Star 1 sibling, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2018-10-18 21:45 UTC (permalink / raw) To: edk2-devel Cc: Star Zeng, Prasad Pandit, Jiewen Yao, Chao Zhang, Leif Lindholm (Linaro address), Vincent Zimmer, Michael Kinney, Gary Lin, Ard Biesheuvel, Steve McIntyre, Peter Jones 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-18 21:45 ` Laszlo Ersek @ 2018-10-19 7:09 ` Zeng, Star 2018-10-19 12:35 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Zeng, Star @ 2018-10-19 7:09 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: Vincent Zimmer, Prasad Pandit, Steve McIntyre, Peter Jones, Jiewen Yao, Michael Kinney, Gary Lin, Chao Zhang, qin.long 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] 2018-10-19 7:09 ` Zeng, Star @ 2018-10-19 12:35 ` Laszlo Ersek 0 siblings, 0 replies; 12+ messages in thread From: Laszlo Ersek @ 2018-10-19 12:35 UTC (permalink / raw) To: Zeng, Star, edk2-devel Cc: Vincent Zimmer, Prasad Pandit, Steve McIntyre, Peter Jones, Jiewen Yao, Michael Kinney, Gary Lin, Chao Zhang, qin.long On 10/19/18 09:09, Zeng, Star wrote: > 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). Makes sense, thanks. Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-10-19 12:35 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-16 2:41 [PATCH] MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE Star Zeng 2018-10-16 7:03 ` Yao, Jiewen 2018-10-17 13:10 ` CVE-2018-3613 [was: MdeModulePkg Variable: Fix Timestamp zeroing issue on APPEND_WRITE] Laszlo Ersek 2018-10-17 14:58 ` Zeng, Star 2018-10-17 18:27 ` Laszlo Ersek 2018-10-18 2:45 ` Zeng, Star 2018-10-18 13:09 ` Laszlo Ersek 2018-10-18 13:43 ` Zeng, Star 2018-10-18 16:04 ` Laszlo Ersek 2018-10-18 21:45 ` Laszlo Ersek 2018-10-19 7:09 ` Zeng, Star 2018-10-19 12:35 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox