| Message ID | E1ZkdZl-0005IT-IU@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On Fri, 09 Oct 2015 20:43:33 +0100 Russell King <rmk+kernel@arm.linux.org.uk> wrote: > If the algorithm passed a zero statesize, do not pass a valid pointer > into the export/import functions. Passing a valid pointer covers up > bugs in driver code which then go on to smash the kernel stack. > Instead, pass NULL, which will cause any attempt to write to the > pointer to fail. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > crypto/ahash.c | 3 ++- > crypto/shash.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/crypto/ahash.c b/crypto/ahash.c > index 8acb886032ae..9c1dc8d6106a 100644 > --- a/crypto/ahash.c > +++ b/crypto/ahash.c > @@ -544,7 +544,8 @@ static int ahash_prepare_alg(struct ahash_alg *alg) > struct crypto_alg *base = &alg->halg.base; > > if (alg->halg.digestsize > PAGE_SIZE / 8 || > - alg->halg.statesize > PAGE_SIZE / 8) > + alg->halg.statesize > PAGE_SIZE / 8 || > + alg->halg.statesize == 0) Just read Russel's answer to the cover letter, and I wonder if the following test wouldn't fix the problem: (alg->halg.statesize == 0 && (alg->import || alg->export)) I mean, the only valid case where statesize can be zero is when you don't have any state associated to the crypto algorithm, and if that's the case, ->import() and ->export() functions are useless, isn't ? Best Regards, Boris > return -EINVAL; > > base->cra_type = &crypto_ahash_type; > diff --git a/crypto/shash.c b/crypto/shash.c > index ecb1e3d39bf0..ab3384b38542 100644 > --- a/crypto/shash.c > +++ b/crypto/shash.c > @@ -585,7 +585,8 @@ static int shash_prepare_alg(struct shash_alg *alg) > > if (alg->digestsize > PAGE_SIZE / 8 || > alg->descsize > PAGE_SIZE / 8 || > - alg->statesize > PAGE_SIZE / 8) > + alg->statesize > PAGE_SIZE / 8 || > + alg->statesize == 0) > return -EINVAL; > > base->cra_type = &crypto_shash_type;
On Sat, Oct 10, 2015 at 06:46:07PM +0200, Boris Brezillon wrote: > On Fri, 09 Oct 2015 20:43:33 +0100 > Russell King <rmk+kernel@arm.linux.org.uk> wrote: > > > If the algorithm passed a zero statesize, do not pass a valid pointer > > into the export/import functions. Passing a valid pointer covers up > > bugs in driver code which then go on to smash the kernel stack. > > Instead, pass NULL, which will cause any attempt to write to the > > pointer to fail. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > --- > > crypto/ahash.c | 3 ++- > > crypto/shash.c | 3 ++- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/crypto/ahash.c b/crypto/ahash.c > > index 8acb886032ae..9c1dc8d6106a 100644 > > --- a/crypto/ahash.c > > +++ b/crypto/ahash.c > > @@ -544,7 +544,8 @@ static int ahash_prepare_alg(struct ahash_alg *alg) > > struct crypto_alg *base = &alg->halg.base; > > > > if (alg->halg.digestsize > PAGE_SIZE / 8 || > > - alg->halg.statesize > PAGE_SIZE / 8) > > + alg->halg.statesize > PAGE_SIZE / 8 || > > + alg->halg.statesize == 0) > > Just read Russel's answer to the cover letter, and I wonder if the > following test wouldn't fix the problem: > > (alg->halg.statesize == 0 && (alg->import || alg->export)) > > I mean, the only valid case where statesize can be zero is when you > don't have any state associated to the crypto algorithm, and if that's > the case, ->import() and ->export() functions are useless, isn't ? However, that brings up a question. If you're using AF_ALG, and you attach to (say) the ARM Neon SHA512 implementation through it, and then use accept() to duplicate it's state, what prevents the kernel from oopsing when hash_accept() calls crypto_ahash_export(), which then dereferences the NULL alg->export function pointer?
On Sat, Oct 10, 2015 at 06:46:07PM +0200, Boris Brezillon wrote: > > I mean, the only valid case where statesize can be zero is when you > don't have any state associated to the crypto algorithm, and if that's > the case, ->import() and ->export() functions are useless, isn't ? The import/export functions are used by algif_hash and must be implemented by the driver. Cheers,
On Sat, Oct 10, 2015 at 05:52:29PM +0100, Russell King - ARM Linux wrote: > > If you're using AF_ALG, and you attach to (say) the ARM Neon SHA512 > implementation through it, and then use accept() to duplicate it's > state, what prevents the kernel from oopsing when hash_accept() calls > crypto_ahash_export(), which then dereferences the NULL alg->export > function pointer? After reading the code I don't think you can actually trigger a NULL dereference since the crypto API will provide a default import and export function that just returns ENOSYS. Having said that, not having an import/export function means that algif_hash may not work correctly so they should be provided by the driver. Cheers,
On Fri, Oct 09, 2015 at 08:43:33PM +0100, Russell King wrote: > If the algorithm passed a zero statesize, do not pass a valid pointer > into the export/import functions. Passing a valid pointer covers up > bugs in driver code which then go on to smash the kernel stack. > Instead, pass NULL, which will cause any attempt to write to the > pointer to fail. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> Patch applied without the shash hunk. I also replaced your commit message as it no longer makes any sense: crypto: ahash - ensure statesize is non-zero Unlike shash algorithms, ahash drivers must implement export and import as their descriptors may contain hardware state and cannot be exported as is. Unfortunately some ahash drivers did not provide them and end up causing crashes with algif_hash. This patch adds a check to prevent these drivers from registering ahash algorithms until they are fixed. Thanks,
On Tue, Oct 13, 2015 at 10:33:12PM +0800, Herbert Xu wrote: > On Fri, Oct 09, 2015 at 08:43:33PM +0100, Russell King wrote: > > If the algorithm passed a zero statesize, do not pass a valid pointer > > into the export/import functions. Passing a valid pointer covers up > > bugs in driver code which then go on to smash the kernel stack. > > Instead, pass NULL, which will cause any attempt to write to the > > pointer to fail. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > Patch applied without the shash hunk. I also replaced your commit > message as it no longer makes any sense: > > crypto: ahash - ensure statesize is non-zero > > Unlike shash algorithms, ahash drivers must implement export > and import as their descriptors may contain hardware state and > cannot be exported as is. Unfortunately some ahash drivers did > not provide them and end up causing crashes with algif_hash. > > This patch adds a check to prevent these drivers from registering > ahash algorithms until they are fixed. > > Thanks, There will be fallout from this. The CAAM driver is similarly buggy - it has export/import functions in its ahash drivers, but zero statesize. User exploitable kernel stack smashing... I'd suggest putting this patch into stable kernels as high priority, as I'm pretty sure this could be used to gain privileges via carefully crafted md5 hashes. I've not proven it, but given that the md5 hash and state data get copied over the kernel stack, it's highly likely that it _is_ exploitable from any user that can create an AF_ALG socket. Yes, it means regressions in the form of various hw crypto no longer being loadable, but I think that's preferable to the security hole here.
On Thu, Oct 15, 2015 at 10:39:30AM +0100, Russell King - ARM Linux wrote: > > The CAAM driver is similarly buggy - it has export/import functions in > its ahash drivers, but zero statesize. > > User exploitable kernel stack smashing... I'd suggest putting this patch > into stable kernels as high priority, as I'm pretty sure this could be I agree. It should already be on its way to stable as Linus has pulled it into his tree and it carries a stable cc. Thanks,
On Thu, Oct 15, 2015 at 05:41:47PM +0800, Herbert Xu wrote: > On Thu, Oct 15, 2015 at 10:39:30AM +0100, Russell King - ARM Linux wrote: > > > > The CAAM driver is similarly buggy - it has export/import functions in > > its ahash drivers, but zero statesize. > > > > User exploitable kernel stack smashing... I'd suggest putting this patch > > into stable kernels as high priority, as I'm pretty sure this could be > > I agree. It should already be on its way to stable as Linus has > pulled it into his tree and it carries a stable cc. Thanks. I think the CAAM driver is pretty unfixable from a trivial point of view. This driver exports a huge amount of state - it contains both a struct caam_hash_ctx and a struct caam_hash_state, which totals up to 1600 bytes. This fails the: alg->halg.statesize > PAGE_SIZE / 8 in ahash_prepare_alg() if we set .statesize. For ARM, this places a limit of 512 bytes on the state size. The CAAM authors need to come up with a better solution (and quickly, as caamhash is going to fail in all kernels soon), or we need to support larger exported states. BTW, I can't find a MAINTAINERS entry for CAAM, so I've just grabbed a couple of addresses from recent git history in the hope they'll know who's responsible.
On Thu, Oct 15, 2015 at 01:59:44PM +0100, Russell King - ARM Linux wrote: > > I think the CAAM driver is pretty unfixable from a trivial point of > view. This driver exports a huge amount of state - it contains both a > struct caam_hash_ctx and a struct caam_hash_state, which totals up to > 1600 bytes. This fails the: Right just dumping the state out as is not going to work. This is not supposed to be how export works anyway. But it doesn't look too bad as most of that 1600 is consumed by the hardware program descriptor which can easily be regenerated upon import. The only things that need to be exported AFAICS are key and buf_X. Cheers,
On Thu, 15 Oct 2015 21:13:38 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Thu, Oct 15, 2015 at 01:59:44PM +0100, Russell King - ARM Linux wrote: > > > > I think the CAAM driver is pretty unfixable from a trivial point of > > view. This driver exports a huge amount of state - it contains both a > > struct caam_hash_ctx and a struct caam_hash_state, which totals up to > > 1600 bytes. This fails the: > > Right just dumping the state out as is not going to work. This > is not supposed to be how export works anyway. But it doesn't > look too bad as most of that 1600 is consumed by the hardware > program descriptor which can easily be regenerated upon import. > > The only things that need to be exported AFAICS are key and buf_X. I just pushed out a patch for export/import functions in the CAAM driver. The patch has been through testing with OpenSSL and the AF_ALG plugin on the MX6. > > Cheers, > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 16, 2015 at 04:24:54PM -0700, Victoria Milhoan wrote: > On Thu, 15 Oct 2015 21:13:38 +0800 > Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > On Thu, Oct 15, 2015 at 01:59:44PM +0100, Russell King - ARM Linux wrote: > > > > > > I think the CAAM driver is pretty unfixable from a trivial point of > > > view. This driver exports a huge amount of state - it contains both a > > > struct caam_hash_ctx and a struct caam_hash_state, which totals up to > > > 1600 bytes. This fails the: > > > > Right just dumping the state out as is not going to work. This > > is not supposed to be how export works anyway. But it doesn't > > look too bad as most of that 1600 is consumed by the hardware > > program descriptor which can easily be regenerated upon import. > > > > The only things that need to be exported AFAICS are key and buf_X. > > I just pushed out a patch for export/import functions in the CAAM driver. The > patch has been through testing with OpenSSL and the AF_ALG plugin on the MX6. Be careful with that. There's two ways to test: 1. Checking hash output. Preparation - copy openssl.cnf and add this to openssl.cnf: openssl_conf = openssl_def [openssl_def] engines = engine_section [engine_section] af_alg = af_alg_engine [af_alg_engine] CIPHERS=aes-128-cbc aes-192-cbc aes-256-cbc des-cbc des-ede3-cbc DIGESTS=md5 sha1 sha256 sha512 # Putting this last means we register the above as the default algorithms default_algorithms = ALL Then: #!/bin/sh for type in md5 sha1 sha256 sha512; do echo -n "Checking $type hash:" for file in /bin/*; do echo -n " $file" if ! OPENSSL_CONF=./openssl.cnf openssl dgst -$type < $file | sed "s,(stdin)= ,,;s,\$,\t$file," | ${type}sum -c > /dev/null; then echo " ... failed" echo -n "Openssl says: " >&2 OPENSSL_CONF=./openssl.cnf openssl dgst -$type < $file | sed "s,(stdin)= ,,;s,\$,\t$file," >&2 echo -n "${type}sum says: " >&2 ${type}sum $file >&2 exit 1 fi done echo " ... ok" done echo "All hashes passed" The above will verify that the hashes are producing the correct answers for a range of files. This does _not_ test the export/import paths. 2. Backup the existing openssl.cnf and replace it with the above modified version. Then try to ssh into the platform. This will verify the export/import side of things. If ssh fails to connect to the target, you know that the crypto drivers for SHA1 are broken, probably due to export/import.
diff --git a/crypto/ahash.c b/crypto/ahash.c index 8acb886032ae..9c1dc8d6106a 100644 --- a/crypto/ahash.c +++ b/crypto/ahash.c @@ -544,7 +544,8 @@ static int ahash_prepare_alg(struct ahash_alg *alg) struct crypto_alg *base = &alg->halg.base; if (alg->halg.digestsize > PAGE_SIZE / 8 || - alg->halg.statesize > PAGE_SIZE / 8) + alg->halg.statesize > PAGE_SIZE / 8 || + alg->halg.statesize == 0) return -EINVAL; base->cra_type = &crypto_ahash_type; diff --git a/crypto/shash.c b/crypto/shash.c index ecb1e3d39bf0..ab3384b38542 100644 --- a/crypto/shash.c +++ b/crypto/shash.c @@ -585,7 +585,8 @@ static int shash_prepare_alg(struct shash_alg *alg) if (alg->digestsize > PAGE_SIZE / 8 || alg->descsize > PAGE_SIZE / 8 || - alg->statesize > PAGE_SIZE / 8) + alg->statesize > PAGE_SIZE / 8 || + alg->statesize == 0) return -EINVAL; base->cra_type = &crypto_shash_type;
If the algorithm passed a zero statesize, do not pass a valid pointer into the export/import functions. Passing a valid pointer covers up bugs in driver code which then go on to smash the kernel stack. Instead, pass NULL, which will cause any attempt to write to the pointer to fail. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- crypto/ahash.c | 3 ++- crypto/shash.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)