Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3e2a42216662adb390831f98ea429a4245a4598a.camel@linux.ibm.com>
Subject: Re: [PATCH v4] KEYS: encrypted: fix key instantiation with
 user-provided data
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Nikolaus Voss <nv@vosn.de>
Cc:     David Howells <dhowells@redhat.com>,
        Jarkko Sakkinen <jarkko@kernel.org>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>, Yael Tzur <yaelt@google.com>,
        Cyril Hrubis <chrubis@suse.cz>, Petr Vorel <pvorel@suse.cz>,
        linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
        linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Tue, 18 Oct 2022 20:53:37 -0400
In-Reply-To: <1b621acf-a1f1-ec9b-21f6-d081d69ed74@vosn.de>
References: <20221013064308.857011E25@mail.steuer-voss.de>
          <924a29d81cc7e0d3e2f62f693a0d8fcef97b9779.camel@linux.ibm.com>
          <c620d6ed-d97f-b0c3-574-7b3cd63a7799@vosn.de>
         <16fe5265c49fcecdf613fe9dd660efe4ae8d452e.camel@linux.ibm.com>
         <1b621acf-a1f1-ec9b-21f6-d081d69ed74@vosn.de>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Fri, 2022-10-14 at 13:39 +0200, Nikolaus Voss wrote:
> On Fri, 14 Oct 2022, Mimi Zohar wrote:
> > On Fri, 2022-10-14 at 08:40 +0200, Nikolaus Voss wrote:
> >> On Thu, 13 Oct 2022, Mimi Zohar wrote:
> >>> On Thu, 2022-10-13 at 08:39 +0200, Nikolaus Voss wrote:
> >>>> Commit cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided
> >>>> decrypted data") added key instantiation with user provided decrypted data.
> >>>> The user data is hex-ascii-encoded but was just memcpy'ed to the binary buffer.
> >>>> Fix this to use hex2bin instead.
> >>>>
> >>>> Old keys created from user provided decrypted data saved with "keyctl pipe"
> >>>> are still valid, however if the key is recreated from decrypted data the
> >>>> old key must be converted to the correct format. This can be done with a
> >>>> small shell script, e.g.:
> >>>>
> >>>> BROKENKEY=abcdefABCDEF1234567890aaaaaaaaaa
> >>>> NEWKEY=$(echo -ne $BROKENKEY | xxd -p -c32)
> >>>> keyctl add user masterkey "$(cat masterkey.bin)" @u
> >>>> keyctl add encrypted testkey "new user:masterkey 32 $NEWKEY" @u
> >>>>
> >>>> It is encouraged to switch to a new key because the effective key size
> >>>> of the old keys is only half of the specified size.
> >>>
> >>> Both the old and new decrypted data size is 32 bytes.  Is the above
> >>> statement necessary, especially since the Documentation example does
> >>> the equivalent?
> >>
> >> The old key has the same byte size but all bytes must be within the
> >> hex-asc?i range of characters, otherwise it is refused by the kernel.
> >> So if you wanted a 32 bytes key you get 16 effective bytes for the key.
> >> In the above example the string size of the $BROKENKEY is 32, while
> >> the string size of the $NEWKEY is 64.
> >>
> >> If you do
> >>
> >> $ echo $NEWKEY
> >> 6162636465664142434445463132333435363738393061616161616161616161
> >>
> >> for the example, the range problem is obvious, so $NEWKEY is still broken.
> >> That's why it should only be used to recover data which should be
> >> reencypted with a new key. If you count exactly, the effective key size is
> >> _slightly_ longer than half of the specified size, but it is still a
> >> severe security problem.
> >
> > So the issue with NEWKEY isn't the "effective key size of the old keys
> > is only half of the specified size", but that the old key, itself, is
> > limited to the hex-ascii range of characters.
> 
> The latter resulting in the former. If for BROKENKEY 32 bytes were 
> specified, a brute force attacker knowing the key properties would only 
> need to try at most 2^(16*8) keys, as if the key was only 16 bytes long. 
> This is what I mean with "effective size" in contrast to the key's byte 
> size which is 32 in my example.
> 
> The security issue is a result of the combination of limiting the input 
> range to hex-ascii and using memcpy() instead of hex2bin(). It could have 
> been fixed either by allowing binary input or using hex2bin() (and 
> doubling the ascii input key length). I chose the latter.

Agreed the latter is better solution.  Please update/replace the
sentence "It is encouraged to switch to a new key because ..." based on
this more complete explanation.

thanks,

Mimi