From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1VOiI8-00050e-TT for mharc-grub-devel@gnu.org; Wed, 25 Sep 2013 02:09:40 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39747) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOiHy-0004zU-CQ for grub-devel@gnu.org; Wed, 25 Sep 2013 02:09:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VOiHp-0008SL-Hd for grub-devel@gnu.org; Wed, 25 Sep 2013 02:09:30 -0400 Received: from mail-lb0-x232.google.com ([2a00:1450:4010:c04::232]:40507) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOiHp-0008SC-9s for grub-devel@gnu.org; Wed, 25 Sep 2013 02:09:21 -0400 Received: by mail-lb0-f178.google.com with SMTP id z5so4593714lbh.23 for ; Tue, 24 Sep 2013 23:09:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=yZumTQULxeiJDSRqBdyOCxXr6Ejcnt7JqMOMNYt2a8M=; b=U2KtPDwkBnoA1iV7GBwEGQ9qa2/HDtHx59CWgSBqB7kAV81uohZd3aKU1LFb53NF7D eO43WjE2q2OnnwzQ1F2U+X80hogufkZ6Dvo+BEnBDRc24b+jAoji8c6bWgffhZYieRlt vaDlHcGUXulEdIOQXmgEQFAnUHauVusL/NOFhPnaAPhHcz/PcrvT7oxmoMo187HnZCVa n/CQzt8pOlFkrDdw/YaAOjMpizSNHjOZ5obbtDWQgRaIMpLJjNSH1K5PTb5vQz8sDONo FEdNv/l0hH9TzeiXdh4AZg/udqlqNe8sN0dHZ7l1s1sPylD8dwpc9ozI3T6aGAR7Qfa7 V9sw== X-Received: by 10.112.198.39 with SMTP id iz7mr14113072lbc.24.1380089359832; Tue, 24 Sep 2013 23:09:19 -0700 (PDT) Received: from opensuse.site (ppp91-76-150-246.pppoe.mtu-net.ru. [91.76.150.246]) by mx.google.com with ESMTPSA id l10sm17737178lbh.13.1969.12.31.16.00.00 (version=SSLv3 cipher=RC4-SHA bits=128/128); Tue, 24 Sep 2013 23:09:19 -0700 (PDT) Date: Wed, 25 Sep 2013 10:09:18 +0400 From: Andrey Borzenkov To: The development of GNU GRUB Subject: Re: [PATCH v4 2/4] load_env support for whitelisting which variables are read from an env file, even if check_signatures=enforce Message-ID: <20130925100918.2087bec5@opensuse.site> In-Reply-To: <1380074432-27299-3-git-send-email-jonmccune@google.com> References: <1380074432-27299-1-git-send-email-jonmccune@google.com> <1380074432-27299-3-git-send-email-jonmccune@google.com> X-Mailer: Claws Mail 3.9.0 (GTK+ 2.24.18; x86_64-suse-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c04::232 Cc: jonmccune@google.com X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Sep 2013 06:09:38 -0000 В Tue, 24 Sep 2013 19:00:30 -0700 Jon McCune пишет: > This version of the patch implements whitelisting in the envblk subsystem, > instead of in loadenv.c. It seems to be cleaner than the previous patches. > > This works by adding an open_envblk_file_untrusted() method that bypasses > signature checking, but only if the invocation of load_env includes a > whitelist of one or more environment variables that are to be read from the > file. I do not really like such silent assumptions. Being able to load only selected environment variables is useful by itself, but I still may want to ensure file was not tampered with. I suggest you simply add flag "--skip-signature-check" to load_env and add support for explicit variable list. Then it is up to user how and when to use it. And please update also documentation for command changes. > +static grub_file_t > +open_envblk_file_untrusted (char *filename) Why do you need extra function? Is not flag to open_envblk_file enough? > +{ > + grub_file_filter_t curfilt[GRUB_FILE_FILTER_MAX]; > + grub_file_t file; > + > + /* Temporarily disable all filters so as to read the untrusted file */ > + grub_memcpy (curfilt, grub_file_filters_enabled, sizeof (curfilt)); > + grub_file_filter_disable_all (); Why do you need to disable *all* filters? Assuming disabling compression was good enough, you just need to disable signature checking, right? > void > grub_envblk_iterate (grub_envblk_t envblk, > + const grub_env_whitelist_t* whitelist, > int hook (const char *name, const char *value)) Again, that's really too restrictive. Like with any other iterators, I'd make hook accept extra pointer for hook-specific data and pass this data to grub_envblk_iterate. This will let caller decide the policy.