Re: AF_ALG broken?

[Herbert Xu]

On Tue, Aug 09, 2016 at 08:27:17AM +0100, Russell King - ARM Linux wrote:
>
> From: Russell King 
> Subject: [PATCH] crypto: caam - fix non-hmac hashes
> 
> Since 6de62f15b581 ("crypto: algif_hash - Require setkey before
> accept(2)"), the AF_ALG interface requires userspace to provide a key
> to any algorithm that has a setkey method.  However, the non-HMAC
> algorithms are not keyed, so setting a key is unnecessary.
> 
> Fix this by removing the setkey method from the non-keyed hash
> algorithms.
> 
> Fixes: 6de62f15b581 ("crypto: algif_hash - Require setkey before accept(2)")
> Cc: 
> Signed-off-by: Russell King 

Patch applied.  Thanks.
-- 
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

Re: AF_ALG broken?

[Russell King - ARM Linux]

On Tue, Aug 09, 2016 at 03:14:02PM +0800, Herbert Xu wrote:
> On Tue, Aug 09, 2016 at 08:08:59AM +0100, Russell King - ARM Linux wrote:
> > 
> > I thought I gave the commands and link to your example code.  The
> > openssl case is md5, though sha* also gives the same result.  Your
> > example code was sha1 iirc.  I guess none of these would be using
> > HMAC - the openssl cases used to give results compatible with the
> > md5sum/ sha1sum etc userspace commands.
> > 
> > /proc/crypto:
> > 
> > name : md5
> > driver   : md5-caam
> 
> Right, caam is providing a setkey function for md5, which leads the
> API to think that a key is required.  We should fix it so that setkey
> is only set for the HMAC-variant.

Thanks, that works nicely again, and passes my tests.

8<
From: Russell King 
Subject: [PATCH] crypto: caam - fix non-hmac hashes

Since 6de62f15b581 ("crypto: algif_hash - Require setkey before
accept(2)"), the AF_ALG interface requires userspace to provide a key
to any algorithm that has a setkey method.  However, the non-HMAC
algorithms are not keyed, so setting a key is unnecessary.

Fix this by removing the setkey method from the non-keyed hash
algorithms.

Fixes: 6de62f15b581 ("crypto: algif_hash - Require setkey before accept(2)")
Cc: 
Signed-off-by: Russell King 
---
 drivers/crypto/caam/caamhash.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index ea284e3909ef..9d7fc9ec0b7e 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -1950,6 +1950,7 @@ caam_hash_alloc(struct caam_hash_template *template,
 template->name);
snprintf(alg->cra_driver_name, CRYPTO_MAX_ALG_NAME, "%s",
 template->driver_name);
+   t_alg->ahash_alg.setkey = NULL;
}
alg->cra_module = THIS_MODULE;
alg->cra_init = caam_hash_cra_init;
-- 
2.1.0

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: AF_ALG broken?

[Herbert Xu]

On Tue, Aug 09, 2016 at 08:08:59AM +0100, Russell King - ARM Linux wrote:
> 
> I thought I gave the commands and link to your example code.  The
> openssl case is md5, though sha* also gives the same result.  Your
> example code was sha1 iirc.  I guess none of these would be using
> HMAC - the openssl cases used to give results compatible with the
> md5sum/ sha1sum etc userspace commands.
> 
> /proc/crypto:
> 
> name : md5
> driver   : md5-caam

Right, caam is providing a setkey function for md5, which leads the
API to think that a key is required.  We should fix it so that setkey
is only set for the HMAC-variant.

Thanks,
-- 
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

Re: AF_ALG broken?

[Herbert Xu]

Russell King - ARM Linux  wrote:
> Testing that code on 4.8-rc (and 4.7 fwiw) gives:
> 
> socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
> bind(3, {sa_family=AF_ALG, sa_data="hash\0\0\0\0\0\0\0\0\0\0"}, 88) = 0
> accept(3, 0, NULL)  = 4
> write(4, "abc", 3)  = -1 ENOKEY (Required key not 
> available)
> read(4, 0xbec50508, 20) = -1 ENOKEY (Required key not 
> available)
> 
> IOW, the same problem - and it seems not to be a recent regression.
> 
> Since the last time I tested CESA or CAAM was back in 4.4 times,
> it's got to be something between 4.4 and 4.7.
> 
> Looking at the history, my guess would be the setkey changes -
> crypto: algif_skcipher - Require setkey before accept(2)
> crypto: af_alg - Disallow bind/setkey/... after accept(2)
> crypto: af_alg - Add nokey compatibility path
> crypto: hash - Add crypto_ahash_has_setkey
> crypto: algif_hash - Require setkey before accept(2)

This is definitely supposed to work.  Basically if the algorithm
requires a key (e.g., HMAC) then you must set it.  Otherwise it
should never return ENOKEY.

Which algorithm were you testing and what does /proc/crypto say?

Thanks,
-- 
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

Re: AF_ALG broken?

[David Miller]

From: Russell King - ARM Linux 
Date: Mon, 8 Aug 2016 23:58:51 +0100

> I don't know, but this seems to go completely against Linus' no
> userspace regressions, which seems to be an absolute requirement of
> all kernel development... Linus flames people for arguing against
> that rule!

Reading the explanation for the change given to you, it's impossible
for some hash algorithms to function properly without being given the
key first, and would crash.

Therefore the requirement is reasonable, even though it is unfortunate
that a critical piece of userspace infrastructure was coded this way.
--
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: AF_ALG broken?

[Stephan Mueller]

Am Montag, 8. August 2016, 20:18:32 CEST schrieb Stephan Mueller:

Hi Stephan,

> Am Montag, 8. August 2016, 17:44:27 CEST schrieb Russell King - ARM Linux:
> 
> Hi Russell,
> 
> > Hi,
> > 
> > When trying to use the openssl AF_ALG module with 4.8-rc1 with imx
> > caam, I get this:
> > 
> > $ OPENSSL_CONF=/shared/crypto/openssl-imx.cnf strace openssl dgst -md5
> >  > socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
> > close(3)= 0
> > socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
> > bind(3, {sa_family=AF_ALG, sa_data="hash\0\0\0\0\0\0\0\0\0\0"}, 88) = 0
> > accept(3, 0, NULL)  = 4
> > fstat64(0, {st_mode=S_IFREG|0755, st_size=666864, ...}) = 0
> > mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0)
> > =
> > 0xb6fab000 read(0,
> > "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\2\0(\0\1\0\0\0\21'\2\0004\0\0\0"...,
> > 8192)
> > = 8192 send(4,
> > "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\2\0(\0\1\0\0\0\21'\2\0004\0\0\0"...,
> > 8192,
> > MSG_MORE) = -1 ENOKEY (Required key not available)
> > 
> > This used to work, so something in the kernel AF_ALG API has changed
> > which has broken userspace.  Any ideas what's up, or where to look?
> 
> This seems to be the the change added by Herbert to fix a security issue.
> This caused a similar stirr in the cryptsetup user space tool.
> 
> I guess you are affected by 6de62f15b581f920ade22d758f4c338311c2f0d4

Just to be clear: the settings on the tfmfd must be completed before an 
accept(). If make an operation on the tfmfd after the accept call, you get 
the ENOKEY.

This was my change to the issue:

https://github.com/smuellerDD/libkcapi/commit/
1d6c3b1b540caea784a95b1ca6e2cf38c174f585

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: AF_ALG broken?

[Stephan Mueller]

Am Montag, 8. August 2016, 17:44:27 CEST schrieb Russell King - ARM Linux:

Hi Russell,

> Hi,
> 
> When trying to use the openssl AF_ALG module with 4.8-rc1 with imx
> caam, I get this:
> 
> $ OPENSSL_CONF=/shared/crypto/openssl-imx.cnf strace openssl dgst -md5
>  socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
> close(3)= 0
> socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
> bind(3, {sa_family=AF_ALG, sa_data="hash\0\0\0\0\0\0\0\0\0\0"}, 88) = 0
> accept(3, 0, NULL)  = 4
> fstat64(0, {st_mode=S_IFREG|0755, st_size=666864, ...}) = 0
> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
> 0xb6fab000 read(0,
> "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\2\0(\0\1\0\0\0\21'\2\0004\0\0\0"..., 8192)
> = 8192 send(4,
> "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\2\0(\0\1\0\0\0\21'\2\0004\0\0\0"..., 8192,
> MSG_MORE) = -1 ENOKEY (Required key not available)
> 
> This used to work, so something in the kernel AF_ALG API has changed
> which has broken userspace.  Any ideas what's up, or where to look?

This seems to be the the change added by Herbert to fix a security issue. This 
caused a similar stirr in the cryptsetup user space tool.

I guess you are affected by 6de62f15b581f920ade22d758f4c338311c2f0d4


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: AF_ALG broken?

[Russell King - ARM Linux]

On Mon, Aug 08, 2016 at 01:47:33PM -0400, Jeffrey Walton wrote:
> > When trying to use the openssl AF_ALG module with 4.8-rc1 with imx
> > caam, I get this:
> >
> > $ OPENSSL_CONF=/shared/crypto/openssl-imx.cnf strace openssl dgst -md5 
> >  > ...
> > socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
> > close(3)= 0
> > socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
> > bind(3, {sa_family=AF_ALG, sa_data="hash\0\0\0\0\0\0\0\0\0\0"}, 88) = 0
> > accept(3, 0, NULL)  = 4
> > fstat64(0, {st_mode=S_IFREG|0755, st_size=666864, ...}) = 0
> > mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 
> > 0xb6fab000
> > read(0, 
> > "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\2\0(\0\1\0\0\0\21'\2\0004\0\0\0"..., 8192) 
> > = 8192
> > send(4, 
> > "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\2\0(\0\1\0\0\0\21'\2\0004\0\0\0"..., 8192, 
> > MSG_MORE) = -1 ENOKEY (Required key not available)
> 
> As far as I know from testing on x86, it has never worked as expected.
> I believe you have to use 'sendto' and 'recvfrom' because 'send' and
> 'recv' use default structures, and they configure the object
> incorrectly.

This used to work, because that's how I've tested my previous CAAM
and Marvell CESA patches.  So, this is not a case of "never worked"
but is definitely a regression caused by some kernel change.

There's also people's presentations that illustrate example code:

https://events.linuxfoundation.org/sites/events/files/slides/lcj-2014-crypto-user.pdf

which can also be found at:

https://lwn.net/Articles/410833/

and that example code comes from Herbert, so one would assume that
it's tested and was working.  Note that it doesn't use any send*,
but uses write().

Testing that code on 4.8-rc (and 4.7 fwiw) gives:

socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
bind(3, {sa_family=AF_ALG, sa_data="hash\0\0\0\0\0\0\0\0\0\0"}, 88) = 0
accept(3, 0, NULL)  = 4
write(4, "abc", 3)  = -1 ENOKEY (Required key not available)
read(4, 0xbec50508, 20) = -1 ENOKEY (Required key not available)

IOW, the same problem - and it seems not to be a recent regression.

Since the last time I tested CESA or CAAM was back in 4.4 times,
it's got to be something between 4.4 and 4.7.

Looking at the history, my guess would be the setkey changes -
crypto: algif_skcipher - Require setkey before accept(2)
crypto: af_alg - Disallow bind/setkey/... after accept(2)
crypto: af_alg - Add nokey compatibility path
crypto: hash - Add crypto_ahash_has_setkey
crypto: algif_hash - Require setkey before accept(2)

-- 
Rmk's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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: AF_ALG broken?

[Jeffrey Walton]

> When trying to use the openssl AF_ALG module with 4.8-rc1 with imx
> caam, I get this:
>
> $ OPENSSL_CONF=/shared/crypto/openssl-imx.cnf strace openssl dgst -md5 
>  ...
> socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
> close(3)= 0
> socket(PF_ALG, SOCK_SEQPACKET, 0)   = 3
> bind(3, {sa_family=AF_ALG, sa_data="hash\0\0\0\0\0\0\0\0\0\0"}, 88) = 0
> accept(3, 0, NULL)  = 4
> fstat64(0, {st_mode=S_IFREG|0755, st_size=666864, ...}) = 0
> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 
> 0xb6fab000
> read(0, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\2\0(\0\1\0\0\0\21'\2\0004\0\0\0"..., 
> 8192) = 8192
> send(4, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\2\0(\0\1\0\0\0\21'\2\0004\0\0\0"..., 
> 8192, MSG_MORE) = -1 ENOKEY (Required key not available)

As far as I know from testing on x86, it has never worked as expected.
I believe you have to use 'sendto' and 'recvfrom' because 'send' and
'recv' use default structures, and they configure the object
incorrectly.

Jeff
--
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