From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.1305.1580458384114735819 for ; Fri, 31 Jan 2020 00:13:04 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VASw3K1G; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580458383; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=BFbbqK9Gix/OeaozKT8lXcevjmeMigzoiwaFVxskCa8=; b=VASw3K1GfS4EmYFOPtM9bzuB1wiN37KsTg6AUBScuh65jf1gO3mkOWdvwxm4xGNJF3T1oX VXd/sLJbYyqrOt0ZJ1k2DxSEfYkpLbOyaSWrL/cTyDRGpihCv7TqugzPgC5VEmbFrJ+38/ +HUIW3y0ZsXg1UhFjJGCGypAszqfAc8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-43-RwKOVolkMnqG9kTLT7oOSw-1; Fri, 31 Jan 2020 03:12:59 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0858E1005513; Fri, 31 Jan 2020 08:12:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-175.ams2.redhat.com [10.36.116.175]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7A4345ED21; Fri, 31 Jan 2020 08:12:55 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy To: "Kinney, Michael D" , "devel@edk2.groups.io" Cc: "Zhang, Chao B" , "Wang, Jian J" , "Yao, Jiewen" References: <20200116190705.18816-1-lersek@redhat.com> From: "Laszlo Ersek" Message-ID: <45017d12-10e1-8a9b-2997-c8fa42fc1049@redhat.com> Date: Fri, 31 Jan 2020 09:12:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-MC-Unique: RwKOVolkMnqG9kTLT7oOSw-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 01/31/20 03:59, Kinney, Michael D wrote: > Hi Laszlo, > > I have reviewed this patch series. All the patches look good. The > detailed description of each change made it easy to review. > > Series Reviewed-by: Michael D Kinney Thank you very much! (more below) > > I have one question about that is not directly related to this logic > change. > > I see this logic that checks for invalid policy settings and ASSERT()s > and halts in a deadloop if either of 2 specific values are used that > have been removed from the UEFI Spec. > > // > // The policy QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECURITY_VIOLATION > // violates the UEFI spec and has been removed. > // > ASSERT (Policy != QUERY_USER_ON_SECURITY_VIOLATION && Policy != ALLOW_EXECUTE_ON_SECURITY_VIOLATION); > if (Policy == QUERY_USER_ON_SECURITY_VIOLATION || Policy == ALLOW_EXECUTE_ON_SECURITY_VIOLATION) { > CpuDeadLoop (); > } > > There are 3 conditions where the Policy comes from a PCD. > > // > // Check the image type and get policy setting. > // > switch (GetImageType (File)) { > > case IMAGE_FROM_FV: > Policy = ALWAYS_EXECUTE; > break; > > case IMAGE_FROM_OPTION_ROM: > Policy = PcdGet32 (PcdOptionRomImageVerificationPolicy); > break; > > case IMAGE_FROM_REMOVABLE_MEDIA: > Policy = PcdGet32 (PcdRemovableMediaImageVerificationPolicy); > break; > > case IMAGE_FROM_FIXED_MEDIA: > Policy = PcdGet32 (PcdFixedMediaImageVerificationPolicy); > break; > > default: > Policy = DENY_EXECUTE_ON_SECURITY_VIOLATION; > break; > } > > However, there are no checks in this function to verify that Policy is > set to one of the allowed values. That's right. I seem to recall that I noticed it too, and I wrote it off with "you can do damage with a bunch of other PCDs as well, if you misconfigure them". > This means an invalid PCD value will fall through to the > EFI_ACCESS_DENIED case. Yes. I find that the safest (silent) fallback for an undefined (per DEC) PCD value. > Is this the expected behavior for an invalid PCD setting? If so, then > why is there a check for the retired values from the UEFI Spec and a > deadloop performed. That seems inconsistent and not a good idea to > deadloop if we do not have to. Would it be better to return > EFI_ACCESS_DENIED for these 2 retired Policy values as well? Hmm, good point. I think these scenarios are different, historically. In one case we have a plain misconfigured platform. In the other case we have a platform that used to have correct configuration (considering edk2 in itself), but then for spec conformance reasons, it got invalidated *retroactively*. And I guess we wanted to be as loud as possible about the second case. There's a difference between "you didn't do your homework" and "you did your homework but we've changed the curriculum meanwhile". So the main question is if we want to remain "silent" about "plain misconfig", vs. "loud" about "obsolete config". We could unify the handling of both cases ("loudly") like this, for example: > diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > index ff79e30ef83e..1d41161abedc 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > @@ -1617,23 +1617,32 @@ DxeImageVerificationHandler ( > Policy = DENY_EXECUTE_ON_SECURITY_VIOLATION; > break; > } > + > + switch (Policy) { > // > // If policy is always/never execute, return directly. > // > - if (Policy == ALWAYS_EXECUTE) { > + case ALWAYS_EXECUTE: > return EFI_SUCCESS; > - } > - if (Policy == NEVER_EXECUTE) { > + case NEVER_EXECUTE: > return EFI_ACCESS_DENIED; > - } > > // > - // The policy QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECURITY_VIOLATION > - // violates the UEFI spec and has been removed. > + // "Defer" and "deny" are valid policies that require actual verification. > // > - ASSERT (Policy != QUERY_USER_ON_SECURITY_VIOLATION && Policy != ALLOW_EXECUTE_ON_SECURITY_VIOLATION); > - if (Policy == QUERY_USER_ON_SECURITY_VIOLATION || Policy == ALLOW_EXECUTE_ON_SECURITY_VIOLATION) { > + case DEFER_EXECUTE_ON_SECURITY_VIOLATION: > + case DENY_EXECUTE_ON_SECURITY_VIOLATION: > + break; > + > + // > + // QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECURITY_VIOLATION > + // violate the UEFI spec; they now indicate platform misconfig. Hang here if > + // we find those policies. Handle undefined policy values the same way. > + // > + default: > + ASSERT (FALSE); > CpuDeadLoop (); > + return EFI_ACCESS_DENIED; > } > > GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID**)&SecureBoot, NULL); Continuing: On 01/31/20 03:59, Kinney, Michael D wrote: > I am ok if you consider this to be a different issue. Yes, TianoCore#2129 is about mistaking DENY for DEFER, while this other topic is "complain loudly, rather than silently, about invalid PCD settings". So let me push this series as-is for TianoCore#2129, with your R-b applied. Then I will submit the above patch for separate review -- I'll put something like "hang on undefined image verification policy PCDs" in the subject. Would you like me to file a separate BZ too, for that patch? Thanks! Laszlo