From patchwork Thu Jul 1 12:41:05 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 1499522 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ozlabs.org (client-ip=112.213.38.117; helo=lists.ozlabs.org; envelope-from=skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org; receiver=) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.a=rsa-sha256 header.s=google header.b=LeI1ik/s; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4GFyWw6VMcz9t2g for ; Thu, 1 Jul 2021 22:41:48 +1000 (AEST) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4GFyWw4q2lz3065 for ; Thu, 1 Jul 2021 22:41:48 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.a=rsa-sha256 header.s=google header.b=LeI1ik/s; dkim-atps=neutral X-Original-To: skiboot@lists.ozlabs.org Delivered-To: skiboot@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=axtens.net (client-ip=2607:f8b0:4864:20::102e; helo=mail-pj1-x102e.google.com; envelope-from=dja@axtens.net; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.a=rsa-sha256 header.s=google header.b=LeI1ik/s; dkim-atps=neutral Received: from mail-pj1-x102e.google.com (mail-pj1-x102e.google.com [IPv6:2607:f8b0:4864:20::102e]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4GFyWp4Hqkz3bXK for ; Thu, 1 Jul 2021 22:41:42 +1000 (AEST) Received: by mail-pj1-x102e.google.com with SMTP id mn20-20020a17090b1894b02901707fc074e8so5817954pjb.0 for ; Thu, 01 Jul 2021 05:41:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=k6YtTVBl3bvB6OoC64HRqCmY8sNCcMRRo478dFu3sEE=; b=LeI1ik/s12kqHSSMZFWY3W9Ol66fAfaET+uJFGkyQ40L+e09BkAI0v1fFXKcEBPo/J JrPeMWNAsEkgodd3OwSP79RMG3XxPYldSxBLmLBlrDWTIaKke1wA1H9QDNrELhgK9L8m h+u0VZP1dhU82wbbY8NpIO3Zyx0+2EmrPPol4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=k6YtTVBl3bvB6OoC64HRqCmY8sNCcMRRo478dFu3sEE=; b=ZelJXptPtuEFiS9oZdiXPCTfn08SGLQONhgAdpyG58OcGTjZ9HqFHGHnfSN564//Sg ph6nEgaAAcGEe20M44b4dMcI/0ueQ532ZPM28FF+VFuMVj0VB/Jp/QQT1nNWZUu2rP1f QSoz5+VJwuVMGy22Wx8iB2gHuzuwj8iKvE28N1BcBkLt5P0EupPA2avB0JssC5Sr5Qla lQdOGHdriL980OANqV+ZvvwkHKD0mdKdLPoB0VmK/0QKG/PLSeI73xT8Sp9tXFABdCiK ruxIou1+vCOv3006sHNdhs7cZuc43xXBJ3u9LnpuXcLFuGDoh7pZWvisrlaSQbqhCjUf za5A== X-Gm-Message-State: AOAM531L3UCuab3Fskt/Vlb6cwRlxyqY/5sD2aIlAfhDGc4M43pf0jvF q/W3mS/0C0guW5Z1cCAyWAtN368eyeGwkg== X-Google-Smtp-Source: ABdhPJxi+aCLnce9JD3ydNJt09NEuNA/hGt3h5XfLlPuRxE/vBsf6XoihbFslmaI5S+OD1yF9YQw9A== X-Received: by 2002:a17:90a:e09:: with SMTP id v9mr8435302pje.41.1625143299840; Thu, 01 Jul 2021 05:41:39 -0700 (PDT) Received: from localhost ([203.206.29.204]) by smtp.gmail.com with ESMTPSA id la17sm14908887pjb.34.2021.07.01.05.41.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Jul 2021 05:41:39 -0700 (PDT) From: Daniel Axtens To: skiboot@lists.ozlabs.org Date: Thu, 1 Jul 2021 22:41:05 +1000 Message-Id: <20210701124106.2784003-7-dja@axtens.net> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20210701124106.2784003-1-dja@axtens.net> References: <20210701124106.2784003-1-dja@axtens.net> MIME-Version: 1.0 Subject: [Skiboot] [PATCH 6/7] secvar/backend: get_pkcs7_len should return a signed type X-BeenThere: skiboot@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list for skiboot development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: nick.child@ibm.com, nayna@linux.ibm.com Errors-To: skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Skiboot" get_pkcs7_len parses a efi_variable_authentication2 and tells us how long the embedded PKCS#7 message is. This process involves reading a number out of the auth header and subtracting a number of constants. As such, this process can fail: if the resultant value is less than 0, this is an error: a pkcs7 message cannot have negative size! Currently, we don't catch this: we calculate with an unsigned type, so it experiences integer overflow and becomes enormous. The field we're reading from the header is 32 bit, so we don't need all our 64 bits as data bits. Instead, change the type to be ssize_t, and if the result of the subtraction is less than 0, return OPAL_PARAMETER. Adjust call-sites to use the different return type and specifically catch return values that are errors. Signed-off-by: Daniel Axtens --- I am fairly sure this cannot cause a bug. If you could get to get_pkcs7 then you'd get to mbedtls_pkcs7_parse_der with a huge len, but the only caller is verify_signature(), which is only called by process_update(). However process_update() first calls get_auth_descriptor2(), which will helpfully call get_pkcs7_len() and compare the result with buf_len. The underflowed negative number will be bigger than buf_len - so the caller (get_auth_descriptor2()) will fail, and process_update() will then bail before calling into verify_signature(). So this patch isn't as urgent as the others. --- libstb/secvar/backend/edk2-compat-process.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c index f6831f1f7ccf..580bd4ebdcc8 100644 --- a/libstb/secvar/backend/edk2-compat-process.c +++ b/libstb/secvar/backend/edk2-compat-process.c @@ -162,11 +162,13 @@ static int get_esl_cert(const char *buf, const size_t buflen, char **cert) /* * Extracts size of the PKCS7 signed data embedded in the * struct Authentication 2 Descriptor Header. + * + * Returns a ssize_t: positive values are OK, negative values are error. */ -static size_t get_pkcs7_len(const struct efi_variable_authentication_2 *auth) +static ssize_t get_pkcs7_len(const struct efi_variable_authentication_2 *auth) { uint32_t dw_length; - size_t size; + ssize_t size; assert(auth != NULL); @@ -176,14 +178,17 @@ static size_t get_pkcs7_len(const struct efi_variable_authentication_2 *auth) + sizeof(auth->auth_info.hdr.w_certificate_type) + sizeof(auth->auth_info.cert_type)); + if (size < 0) + return OPAL_PARAMETER; + return size; } int get_auth_descriptor2(const void *buf, const size_t buflen, void **auth_buffer) { const struct efi_variable_authentication_2 *auth = buf; - int auth_buffer_size; - size_t len; + ssize_t auth_buffer_size; + ssize_t len; assert(auth_buffer != NULL); if (buflen < sizeof(struct efi_variable_authentication_2) @@ -192,7 +197,7 @@ int get_auth_descriptor2(const void *buf, const size_t buflen, void **auth_buffe len = get_pkcs7_len(auth); /* pkcs7 content length cannot be greater than buflen */ - if (len > buflen) + if (len < 0 || len > buflen) return OPAL_PARAMETER; auth_buffer_size = sizeof(auth->timestamp) + sizeof(auth->auth_info.hdr) @@ -417,11 +422,13 @@ int check_timestamp(const char *key, const struct efi_time *timestamp, static mbedtls_pkcs7* get_pkcs7(const struct efi_variable_authentication_2 *auth) { char *checkpkcs7cert = NULL; - size_t len; + ssize_t len; mbedtls_pkcs7 *pkcs7 = NULL; int rc; len = get_pkcs7_len(auth); + if (len < 0) + return NULL; pkcs7 = malloc(sizeof(struct mbedtls_pkcs7)); if (!pkcs7)