[PATCH] crypto: fix potential NULL pointer dereference in skcipher_alloc_sgl()

[Jeff Liu]

From: Jie Liu 

In skcipher_alloc_sgl(), there is a potential null pointer dereference
issue to retrieve the last item from ctx->tsgl list if the list is empty.

This patch fix it by checking if the list is empty or not at first.

Signed-off-by: Jie Liu 
---
 crypto/algif_skcipher.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index a1c4f0a..bfa702e 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -73,9 +73,10 @@ static int skcipher_alloc_sgl(struct sock *sk)
struct skcipher_sg_list *sgl;
struct scatterlist *sg = NULL;
 
-   sgl = list_entry(ctx->tsgl.prev, struct skcipher_sg_list, list);
-   if (!list_empty(&ctx->tsgl))
+   if (!list_empty(&ctx->tsgl)) {
+   sgl = list_entry(ctx->tsgl.prev, struct skcipher_sg_list, list);
sg = sgl->sg;
+   }
 
if (!sg || sgl->cur >= MAX_SGL_ENTS) {
sgl = sock_kmalloc(sk, sizeof(*sgl) +
-- 
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] CPU Jitter RNG: inclusion into kernel crypto API and /dev/random

[Stephan Mueller]

Am Donnerstag, 14. November 2013, 19:30:22 schrieb Clemens Ladisch:

Hi Clemens,

>Stephan Mueller wrote:
>> Am Donnerstag, 14. November 2013, 11:51:03 schrieb Clemens Ladisch:
>>> An attacker would not try to detect patterns; he would apply
>>> knowledge
>>> of the internals.
>> 
>> I do not buy that argument, because if an attacker can detect or
>> deduce the internals of the CPU, he surely can detect the state of
>> the input_pool or the other entropy pools behind /dev/random.
>
>With "internals", I do not mean the actual state of the CPU, but the
>behaviour of all the CPU's execution engines.
>
>An Intel engineer might know how to affect the CPU so that the CPU
>jitter code measures a deterministic pattern, but he will not know the
>contents of my memory.

Here I agree fully.
>
>>> Statistical tests are useful only for detecting the absence of
>>> entropy, not for the opposite.
>> 
>> Again, I fully agree. But it is equally important to understand that
>> entropy is relative.
>
>In cryptography, we care about absolute entropy, i.e., _nobody_ must be
>able to predict the RNG output, not even any CPU engineer.

With your clarification above, I agree here fully.

And now my task is to verify the root cause which I seem to have found.

Let me do my homework.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] CPU Jitter RNG: inclusion into kernel crypto API and /dev/random

[Clemens Ladisch]

Stephan Mueller wrote:
> Am Donnerstag, 14. November 2013, 11:51:03 schrieb Clemens Ladisch:
>> An attacker would not try to detect patterns; he would apply knowledge
>> of the internals.
>
> I do not buy that argument, because if an attacker can detect or deduce
> the internals of the CPU, he surely can detect the state of the
> input_pool or the other entropy pools behind /dev/random.

With "internals", I do not mean the actual state of the CPU, but the
behaviour of all the CPU's execution engines.

An Intel engineer might know how to affect the CPU so that the CPU
jitter code measures a deterministic pattern, but he will not know the
contents of my memory.

>> Statistical tests are useful only for detecting the absence of entropy,
>> not for the opposite.
>
> Again, I fully agree. But it is equally important to understand that
> entropy is relative.

In cryptography, we care about absolute entropy, i.e., _nobody_ must be
able to predict the RNG output, not even any CPU engineer.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] CPU Jitter RNG: inclusion into kernel crypto API and /dev/random

[Stephan Mueller]

Am Donnerstag, 14. November 2013, 11:51:03 schrieb Clemens Ladisch:

Hi Clemens,

>Stephan Mueller wrote:
>> Am Mittwoch, 13. November 2013, 12:51:44 schrieb Clemens Ladisch:
>>> (And any setting that increases accesses to main memory is likey to
>>> introduce more entropy due to clock drift between the processor and
>>> the memory bus.  Or where do you assume the entropy comes from?)
>> 
>> You nailed it. When I disable the caches using the CR0 setting, I get
>> a massive increase in variations and thus entropy.
>
>Now this would be an opportunity to use the number of main memory
>accesses to estimate entropy.  (And when you're running out of the
>cache, i.e., deterministically, is there any entropy?)
>

I seem to have found the root cause with my bare metal tester, but I am 
yet unable to make sense of it.

I will report back with more analyses.


>An attacker would not try to detect patterns; he would apply knowledge
>of the internals.

I do not buy that argument, because if an attacker can detect or deduce 
the internals of the CPU, he surely can detect the state of the 
input_pool or the other entropy pools behind /dev/random. And then, 
/dev/random is not so entropic any more for that attacker.
>
>Statistical tests are useful only for detecting the absence of entropy,
>not for the opposite.

Again, I fully agree. But it is equally important to understand that 
entropy is relative. And all I want and all I care about is that an 
attacker has only the knowledge and ability to make measurements that 
are less precise than 1 bit.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: crypto: s390 - Fix aes-cbc IV corruption

[Jan Glauber]

On Thu, Oct 31, 2013 at 11:25:47AM +0800, Herbert Xu wrote:
> Hi:

Hi Herbert,

just seen this as my old email address is dead... Your patch looks
fine as it keeps the iv and the key together as required by the instruction.

However, I'm curious how this could be racy with threads. The encryption must
be serialized because of the chaining. The decryption could in theory happen
in parallel, but is this the case here?

--Jan

> The cbc-aes-s390 algorithm incorrectly places the IV in the tfm
> data structure.  As the tfm is shared between multiple threads,
> this introduces a possibility of data corruption.
> 
> This patch fixes this by moving the parameter block containing
> the IV and key onto the stack (the block is 48 bytes long).
> 
> The same bug exists elsewhere in the s390 crypto system and they
> will be fixed in subsequent patches.
> 
> Signed-off-by: Herbert Xu 
> 
> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index b4dbade..2e4b5be 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c
> @@ -35,7 +35,6 @@ static u8 *ctrblk;
>  static char keylen_flag;
>  
>  struct s390_aes_ctx {
> - u8 iv[AES_BLOCK_SIZE];
>   u8 key[AES_MAX_KEY_SIZE];
>   long enc;
>   long dec;
> @@ -441,30 +440,36 @@ static int cbc_aes_set_key(struct crypto_tfm *tfm, 
> const u8 *in_key,
>   return aes_set_key(tfm, in_key, key_len);
>  }
>  
> -static int cbc_aes_crypt(struct blkcipher_desc *desc, long func, void *param,
> +static int cbc_aes_crypt(struct blkcipher_desc *desc, long func,
>struct blkcipher_walk *walk)
>  {
> + struct s390_aes_ctx *sctx = crypto_blkcipher_ctx(desc->tfm);
>   int ret = blkcipher_walk_virt(desc, walk);
>   unsigned int nbytes = walk->nbytes;
> + struct {
> + u8 iv[AES_BLOCK_SIZE];
> + u8 key[AES_MAX_KEY_SIZE];
> + } param;
>  
>   if (!nbytes)
>   goto out;
>  
> - memcpy(param, walk->iv, AES_BLOCK_SIZE);
> + memcpy(param.iv, walk->iv, AES_BLOCK_SIZE);
> + memcpy(param.key, sctx->key, sctx->key_len);
>   do {
>   /* only use complete blocks */
>   unsigned int n = nbytes & ~(AES_BLOCK_SIZE - 1);
>   u8 *out = walk->dst.virt.addr;
>   u8 *in = walk->src.virt.addr;
>  
> - ret = crypt_s390_kmc(func, param, out, in, n);
> + ret = crypt_s390_kmc(func, ¶m, out, in, n);
>   if (ret < 0 || ret != n)
>   return -EIO;
>  
>   nbytes &= AES_BLOCK_SIZE - 1;
>   ret = blkcipher_walk_done(desc, walk, nbytes);
>   } while ((nbytes = walk->nbytes));
> - memcpy(walk->iv, param, AES_BLOCK_SIZE);
> + memcpy(walk->iv, param.iv, AES_BLOCK_SIZE);
>  
>  out:
>   return ret;
> @@ -481,7 +486,7 @@ static int cbc_aes_encrypt(struct blkcipher_desc *desc,
>   return fallback_blk_enc(desc, dst, src, nbytes);
>  
>   blkcipher_walk_init(&walk, dst, src, nbytes);
> - return cbc_aes_crypt(desc, sctx->enc, sctx->iv, &walk);
> + return cbc_aes_crypt(desc, sctx->enc, &walk);
>  }
>  
>  static int cbc_aes_decrypt(struct blkcipher_desc *desc,
> @@ -495,7 +500,7 @@ static int cbc_aes_decrypt(struct blkcipher_desc *desc,
>   return fallback_blk_dec(desc, dst, src, nbytes);
>  
>   blkcipher_walk_init(&walk, dst, src, nbytes);
> - return cbc_aes_crypt(desc, sctx->dec, sctx->iv, &walk);
> + return cbc_aes_crypt(desc, sctx->dec, &walk);
>  }
>  
>  static struct crypto_alg cbc_aes_alg = {
> 
> Cheers,
> -- 
> Email: Herbert Xu 
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] CPU Jitter RNG: inclusion into kernel crypto API and /dev/random

[Clemens Ladisch]

Stephan Mueller wrote:
> Am Mittwoch, 13. November 2013, 12:51:44 schrieb Clemens Ladisch:
>> (And any setting that increases accesses to main memory is likey to
>> introduce more entropy due to clock drift between the processor and the
>> memory bus.  Or where do you assume the entropy comes from?)
>
> You nailed it. When I disable the caches using the CR0 setting, I get a
> massive increase in variations and thus entropy.

Now this would be an opportunity to use the number of main memory
accesses to estimate entropy.  (And when you're running out of the
cache, i.e., deterministically, is there any entropy?)

>>> XOR is a bijective operation.
>>
>> Only XOR with a constant, which is not what you're doing.
>
> If you want to regain the full 64 bit input bit stream, then you are
> right.
>
> But still, XOR is bijective with respect to the two bits that go into
> the operation.

Please look up what "bijective" actually means:


> folding based on XOR is an appropriate way to collapse the string and
> yet retain entropy.

Correct, but the reason for that is not being bijective.

>> If the observations are not independent, you cannot just assume that
>> the estimate is off by a certain factor.  It means that the estimate
>> is not applicable _at all_.
>
> Well, I am not so sure here.

So, what is the factor that you are saying is large enough?

>>> The RNG has the following safety margins where it is more conservative than
>>> measurements indicate:
>>
>> You _cannot_ measure entropy by looking at the output.  How would you
>> differentiate between /dev/random and /dev/urandom?
>
> I think regarding the independence you can very well draw the conclusion
> based on the output, because any dependencies will ultimately form a
> pattern.

The output of a pure PRNG (such as /dev/urandom without entropy) is
dependent on the internal state, but there are no patterns detectable
from the output alone.

> In addition, we need to keep in mind that the independence claim is
> relative

No, independence is a property of the process that generates the
samples.

> If you have an output string that has no detectable patterns, i.e. looks
> like white noise, you can only assume that the observations are
> independent of each other. If there are patterns, you know that there
> are dependencies.
>
> That statement may *not* apply any more if you look at the internals of
> the RNG. In such a case, you may have even strong dependencies.
>
> The best example on this matter are deterministic RNGs with a
> cryptographic output function. When you see the output string, you only
> see white noise. As you cannot detect a pattern, you have to assume that
> the string provides independent observations. At least for you who looks
> from the outside cannot draw any conclusions from observing some output
> bit stream which would be the future bit stream. Yet, when you know the
> state of the RNG, you entire future output bit stream has no
> independence.

You cannot restrict the analysis to observations from the outside;
there are many people who can know about the CPU's internal structure.

> Coming back to the jitter RNG: I applied a large array of statistical
> tests and all of them concluded that the output is white noise, as
> explained in the documentation. That means when looking from the
> outside, there are no dependencies visible. Now you may say that this
> conclusion does not mean that there are no dependencies -- but I reply
> again, if there would be any, none of the array of statistical tests can
> detect it. So, how would an attacker detect patterns?

An attacker would not try to detect patterns; he would apply knowledge
of the internals.

Statistical tests are useful only for detecting the absence of entropy,
not for the opposite.


All your arguments about the output of the CPU jitter RNG also apply to
the output of /dev/urandom.  So are you saying that it would be a good
idea to loop the output of /dev/urandom back into /dev/random?


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html