Re: x509 parsing bug + fuzzing crypto in the userspace

[Eric Biggers]

+Cc keyri...@vger.kernel.org (for asymmetric_keys)

First of all, thanks for working on this!  A lot of this code really needs to be
better tested.

On Mon, Nov 20, 2017 at 03:10:55PM +0100, Alexander Potapenko wrote:
> Hi all,
> 
> TL;DR userspace fuzzing may be very effective for finding bugs in
> crypto code, but might require some kernel-side changes.
> 
> When the attached binary file,
> crash-bbeda07d4ce60cf3b7fc20c3af92297e19f6dbae, is passed as payload
> to add_key("asymmetric", "desc", payload, len), it triggers a
> slab-out-of-bounds KASAN report (see below). This can be reproduced
> with the attached program, add_key.c.

The bug is in asn1_ber_decoder(): the special length value of 0x80, which
indicates an "indefinite length", is being passed to one of the "action"
functions as the real length, causing the input buffer to be overread.  Off-hand
I don't know the best fix, but probably it would involve calling
asn1_find_indefinite_length() before the "action" rather than after...

> 
> We found this bug by fuzzing asn1_ber_decoder() in the userspace using
> libFuzzer (http://llvm.org/docs/LibFuzzer.html).
> The trick is to compile and link together several translation units
> that provide a transitive closure of asn1_ber_decoder() (some of the
> kernel functions need to be re-implemented to run in the userspace).
> I've attached the resulting fuzzer as well (asn1_fuzzer.tar.gz). It
> can operate on its own, although better results are achieved when
> using the fuzzing corpus from
> https://github.com/google/boringssl/tree/master/fuzz and
> https://github.com/openssl/openssl/tree/master/fuzz.

I had actually tried almost the same thing recently, but with afl-fuzz instead
of libFuzzer.  But I don't think it was any better than what you did, and I
didn't find the above bug, perhaps because the AFL_HARDEN compiler options don't
include AddressSanitizer.

I did use the original unpreprocessed .c files instead of preprocessed ones, but
it required stubbing out quite a few headers.  Probably using preprocessed files
is a bit easier.

For the fuzzing function I had it call x509_cert_parse() rather than
asn1_ber_decode() directly, but with x509_get_sig_params() and
x509_check_for_self_signed() stubbed out.  It would be nice to get coverage of
x509_check_for_self_signed() because it will call into crypto/rsa.c and, among
other things, run yet another ASN.1 decoder...  But that would have required
porting a lot of the crypto API to userspace.

> 
> Could it be possible to decouple the parsing algorithms from the rest
> of kernel-specific crypto code? Maybe someone has other ideas about
> how this code can be ran in the userspace?
> 

One idea is that the ASN.1 parsers could be run with no "actions", which would
minimize dependencies on other kernel code.  But unfortunately that would omit a
lot of the interesting code.  Many (or most?) bugs will be in how the parsed
data is used, rather than in the parsing itself.  I don't think there is an easy
solution.

Note that separate from asymmetric_keys (which you can think of as being
in-between the keyrings subsystem and the crypto subsystem) there is also the
userspace interface to cryptographic algorithms, AF_ALG.  It might be possible
to port a lot of the crypto API to userspace, but it would require a lot of work
to stub things out.  Maybe a simpler improvement would be to teach syzkaller to
more thoroughly test AF_ALG.  For example it could be made aware of algorithm
templates so that it could try combining them in unusual ways.  (Example:
https://marc.info/?l=linux-crypto-vger=148063683310477=2 was a NULL pointer
dereference bug that occurred if you asked to use the algorithm "mcryptd(md5)",
i.e. the mcryptd template wrapping md5.)  Also,
CONFIG_CRYPTO_MANAGER_DISABLE_TESTS should be unset, so that the crypto
self-tests are enabled.

Eric

Re: [PATCH 5/8] crypto: remove unused hardirq.h

[Yang Shi]

The email to Herbert is returned, resent it.

Yang


On 11/17/17 3:02 PM, Yang Shi wrote:

Preempt counter APIs have been split out, currently, hardirq.h just
includes irq_enter/exit APIs which are not used by crypto at all.

So, remove the unused hardirq.h.

Signed-off-by: Yang Shi 
Cc: Herbert Xu 
Cc: "David S. Miller" 
Cc: linux-crypto@vger.kernel.org
---
  crypto/ablk_helper.c | 1 -
  crypto/blkcipher.c   | 1 -
  crypto/mcryptd.c | 1 -
  3 files changed, 3 deletions(-)

diff --git a/crypto/ablk_helper.c b/crypto/ablk_helper.c
index 1441f07..ee52660 100644
--- a/crypto/ablk_helper.c
+++ b/crypto/ablk_helper.c
@@ -28,7 +28,6 @@
  #include 
  #include 
  #include 
-#include 
  #include 
  #include 
  #include 
diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index 6c43a0a..01c0d4a 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -18,7 +18,6 @@
  #include 
  #include 
  #include 
-#include 
  #include 
  #include 
  #include 
diff --git a/crypto/mcryptd.c b/crypto/mcryptd.c
index 4e64726..9fa362c 100644
--- a/crypto/mcryptd.c
+++ b/crypto/mcryptd.c
@@ -26,7 +26,6 @@
  #include 
  #include 
  #include 
-#include 
  
  #define MCRYPTD_MAX_CPU_QLEN 100

  #define MCRYPTD_BATCH 9

Re: [PATCH] crypto: hifn_795x - Fix a memory leak in the error handling path of 'hifn_probe()'

[PrasannaKumar Muralidharan]

Hi Christophe,

On 18 November 2017 at 19:15, Christophe JAILLET
 wrote:
> 'dev' is leaking in the error handling path of 'hifn_probe()'.
>
> Add a 'kfree(dev)' to match the code in 'hifn_remove()'
>
> Signed-off-by: Christophe JAILLET 
> ---
>  drivers/crypto/hifn_795x.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
> index e09d4055b19e..a5a36fe7bf2c 100644
> --- a/drivers/crypto/hifn_795x.c
> +++ b/drivers/crypto/hifn_795x.c
> @@ -2579,6 +2579,7 @@ static int hifn_probe(struct pci_dev *pdev, const 
> struct pci_device_id *id)
> for (i = 0; i < 3; ++i)
> if (dev->bar[i])
> iounmap(dev->bar[i]);
> +   kfree(dev);
>
>  err_out_free_regions:
> pci_release_regions(pdev);
> --
> 2.14.1
>

Nice catch.

Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH 3/8] fs: btrfs: remove unused hardirq.h

[David Sterba]

On Sat, Nov 18, 2017 at 07:02:16AM +0800, Yang Shi wrote:
> Preempt counter APIs have been split out, currently, hardirq.h just
> includes irq_enter/exit APIs which are not used by btrfs at all.
> 
> So, remove the unused hardirq.h.
> 
> Signed-off-by: Yang Shi 
> Cc: Chris Mason 
> Cc: Josef Bacik 
> Cc: David Sterba 
> Cc: linux-bt...@vger.kernel.org

Acked-by: David Sterba