| Message ID | E1bFfDK-0007yq-FV@gondolin.me.apana.org.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
Hi Herbert, On 22 June 2016 at 12:16, Herbert Xu <herbert@gondor.apana.org.au> wrote: > The only user of rsa-pkcs1pad always uses the hash so there is > no reason to support the case of not having a hash. We use pkcs1pad with AF_ALG to implement lightweight TLS. TLS versions < 1.2 use a non-standard hash so we'd have to move the PKCS#1 padding back to userspace if this is changed. The remaining patches make sense, I think there was some reason there were those PAGE_SIZE checks in every operation but it's probably no longer valid. Best regards -- 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 Wed, Jun 22, 2016 at 03:20:51PM +0200, Andrzej Zaborowski wrote: > > We use pkcs1pad with AF_ALG to implement lightweight TLS. TLS > versions < 1.2 use a non-standard hash so we'd have to move the PKCS#1 > padding back to userspace if this is changed. When this is submitted for upstream inclusion we can add support for it. Cheers,
Hi Herbert, On 06/22/2016 09:02 AM, Herbert Xu wrote: > On Wed, Jun 22, 2016 at 03:20:51PM +0200, Andrzej Zaborowski wrote: >> >> We use pkcs1pad with AF_ALG to implement lightweight TLS. TLS >> versions < 1.2 use a non-standard hash so we'd have to move the PKCS#1 >> padding back to userspace if this is changed. > > When this is submitted for upstream inclusion we can add support > for it. > Just to clarify, we use this from userspace. So we _already_ depend on this functionality. Please keep the hash and non-hash versions of pkcs1pad available. Regards, -Denis -- 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 Wed, Jun 22, 2016 at 09:19:16AM -0500, Denis Kenzior wrote: > Just to clarify, we use this from userspace. So we _already_ depend > on this functionality. Please keep the hash and non-hash versions > of pkcs1pad available. How can you be depending on this in userspace when we haven't even exported akcipher to userspace? Colour me confused. Cheers,
Hi Herbert, On 06/22/2016 09:20 AM, Herbert Xu wrote: > On Wed, Jun 22, 2016 at 09:19:16AM -0500, Denis Kenzior wrote: >> Just to clarify, we use this from userspace. So we _already_ depend >> on this functionality. Please keep the hash and non-hash versions >> of pkcs1pad available. > > How can you be depending on this in userspace when we haven't > even exported akcipher to userspace? Colour me confused. > We live on the bleeding edge :) I realize that these features are not upstream yet, but that doesn't mean that we can't influence / see the direction that the kernel is taking and act accordingly. We'd like to have both pkcs1pad + hash, and simple pkcs1pad go upstream. That will make our job in userspace much easier. Andrew submitted pkcs1pad transform to the kernel specifically so we could get rid of this logic in our userspace code. So please consider leaving both versions for upstream inclusion. Regards, -Denis -- 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 Wed, Jun 22, 2016 at 09:30:12AM -0500, Denis Kenzior wrote: > > We live on the bleeding edge :) > > I realize that these features are not upstream yet, but that doesn't > mean that we can't influence / see the direction that the kernel is > taking and act accordingly. > > We'd like to have both pkcs1pad + hash, and simple pkcs1pad go > upstream. That will make our job in userspace much easier. Andrew > submitted pkcs1pad transform to the kernel specifically so we could > get rid of this logic in our userspace code. So please consider > leaving both versions for upstream inclusion. Sorry but the crypto API isn't a repository for general algorithms. It's first and foremost a place for algorithms that we use in the kernel. The user-space interface (if we ever add one for akcipher, right now there are strong objections against it) is mainly there to allow access to hardware accelerators. So I'm afraid I cannot keep the hashless pkcs1pad until such a time that either we have a kernel user for it or there is a piece of hardware implementing it. Cheers,
Herbert, On Wed, 22 Jun 2016, Herbert Xu wrote: > On Wed, Jun 22, 2016 at 09:30:12AM -0500, Denis Kenzior wrote: >> >> We live on the bleeding edge :) >> >> I realize that these features are not upstream yet, but that doesn't >> mean that we can't influence / see the direction that the kernel is >> taking and act accordingly. >> >> We'd like to have both pkcs1pad + hash, and simple pkcs1pad go >> upstream. That will make our job in userspace much easier. Andrew >> submitted pkcs1pad transform to the kernel specifically so we could >> get rid of this logic in our userspace code. So please consider >> leaving both versions for upstream inclusion. > > Sorry but the crypto API isn't a repository for general algorithms. > It's first and foremost a place for algorithms that we use in the > kernel. > > The user-space interface (if we ever add one for akcipher, right now > there are strong objections against it) is mainly there to allow > access to hardware accelerators. So I'm afraid I cannot keep the > hashless pkcs1pad until such a time that either we have a kernel > user for it or there is a piece of hardware implementing it. David Howells has a keyctl patch set in progress that makes use of pkcs1pad, with or without a hash: https://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-asym-keyctl&id=6fe3b4aa7df524f4867868b01d4cb4345b1bf2de Please leave the non-hash code in, or consider deferring this patch until we can also discuss the issue at the upcoming security summit. We've been having a lot of trouble getting agreement on userspace access to asymmetric ciphers and I think we could make some progress with in-person discussion. (Mailing list discussion is also important because not everyone concerned can attend the summit) Thanks, -- Mat Martineau Intel OTC -- 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 Wed, Jun 22, 2016 at 08:39:09AM -0700, Mat Martineau wrote: > > David Howells has a keyctl patch set in progress that makes use of > pkcs1pad, with or without a hash: > > https://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-asym-keyctl&id=6fe3b4aa7df524f4867868b01d4cb4345b1bf2de I'm sorry but I'm not going to allow arbitrary software algorithms to be exported to userspace like this. As I said, if we have a legitimate kernel user or a real hardware implementation I'll reconsider this. Cheers,
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c index ead8dc0..5c1c78e 100644 --- a/crypto/rsa-pkcs1pad.c +++ b/crypto/rsa-pkcs1pad.c @@ -92,13 +92,12 @@ static const struct rsa_asn1_template *rsa_lookup_asn1(const char *name) struct pkcs1pad_ctx { struct crypto_akcipher *child; - const char *hash_name; unsigned int key_size; }; struct pkcs1pad_inst_ctx { struct crypto_akcipher_spawn spawn; - const char *hash_name; + const struct rsa_asn1_template *digest_info; }; struct pkcs1pad_request { @@ -416,20 +415,16 @@ static int pkcs1pad_sign(struct akcipher_request *req) struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req); struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm); struct pkcs1pad_request *req_ctx = akcipher_request_ctx(req); - const struct rsa_asn1_template *digest_info = NULL; + struct akcipher_instance *inst = akcipher_alg_instance(tfm); + struct pkcs1pad_inst_ctx *ictx = akcipher_instance_ctx(inst); + const struct rsa_asn1_template *digest_info = ictx->digest_info; int err; unsigned int ps_end, digest_size = 0; if (!ctx->key_size) return -EINVAL; - if (ctx->hash_name) { - digest_info = rsa_lookup_asn1(ctx->hash_name); - if (!digest_info) - return -EINVAL; - - digest_size = digest_info->size; - } + digest_size = digest_info->size; if (req->src_len + digest_size > ctx->key_size - 11) return -EOVERFLOW; @@ -462,10 +457,8 @@ static int pkcs1pad_sign(struct akcipher_request *req) memset(req_ctx->in_buf + 1, 0xff, ps_end - 1); req_ctx->in_buf[ps_end] = 0x00; - if (digest_info) { - memcpy(req_ctx->in_buf + ps_end + 1, digest_info->data, - digest_info->size); - } + memcpy(req_ctx->in_buf + ps_end + 1, digest_info->data, + digest_info->size); pkcs1pad_sg_set_buf(req_ctx->in_sg, req_ctx->in_buf, ctx->key_size - 1 - req->src_len, req->src); @@ -499,7 +492,9 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err) struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req); struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm); struct pkcs1pad_request *req_ctx = akcipher_request_ctx(req); - const struct rsa_asn1_template *digest_info; + struct akcipher_instance *inst = akcipher_alg_instance(tfm); + struct pkcs1pad_inst_ctx *ictx = akcipher_instance_ctx(inst); + const struct rsa_asn1_template *digest_info = ictx->digest_info; unsigned int pos; if (err == -EOVERFLOW) @@ -527,17 +522,11 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err) goto done; pos++; - if (ctx->hash_name) { - digest_info = rsa_lookup_asn1(ctx->hash_name); - if (!digest_info) - goto done; + if (memcmp(req_ctx->out_buf + pos, digest_info->data, + digest_info->size)) + goto done; - if (memcmp(req_ctx->out_buf + pos, digest_info->data, - digest_info->size)) - goto done; - - pos += digest_info->size; - } + pos += digest_info->size; err = 0; @@ -626,12 +615,11 @@ static int pkcs1pad_init_tfm(struct crypto_akcipher *tfm) struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm); struct crypto_akcipher *child_tfm; - child_tfm = crypto_spawn_akcipher(akcipher_instance_ctx(inst)); + child_tfm = crypto_spawn_akcipher(&ictx->spawn); if (IS_ERR(child_tfm)) return PTR_ERR(child_tfm); ctx->child = child_tfm; - ctx->hash_name = ictx->hash_name; return 0; } @@ -648,12 +636,12 @@ static void pkcs1pad_free(struct akcipher_instance *inst) struct crypto_akcipher_spawn *spawn = &ctx->spawn; crypto_drop_akcipher(spawn); - kfree(ctx->hash_name); kfree(inst); } static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb) { + const struct rsa_asn1_template *digest_info; struct crypto_attr_type *algt; struct akcipher_instance *inst; struct pkcs1pad_inst_ctx *ctx; @@ -676,7 +664,11 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb) hash_name = crypto_attr_alg_name(tb[2]); if (IS_ERR(hash_name)) - hash_name = NULL; + return PTR_ERR(hash_name); + + digest_info = rsa_lookup_asn1(hash_name); + if (!digest_info) + return -EINVAL; inst = kzalloc(sizeof(*inst) + sizeof(*ctx), GFP_KERNEL); if (!inst) @@ -684,7 +676,7 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb) ctx = akcipher_instance_ctx(inst); spawn = &ctx->spawn; - ctx->hash_name = hash_name ? kstrdup(hash_name, GFP_KERNEL) : NULL; + ctx->digest_info = digest_info; crypto_set_spawn(&spawn->base, akcipher_crypto_instance(inst)); err = crypto_grab_akcipher(spawn, rsa_alg_name, 0, @@ -696,27 +688,14 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb) err = -ENAMETOOLONG; - if (!hash_name) { - if (snprintf(inst->alg.base.cra_name, - CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s)", - rsa_alg->base.cra_name) >= - CRYPTO_MAX_ALG_NAME || - snprintf(inst->alg.base.cra_driver_name, - CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s)", - rsa_alg->base.cra_driver_name) >= - CRYPTO_MAX_ALG_NAME) + if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME, + "pkcs1pad(%s,%s)", rsa_alg->base.cra_name, hash_name) >= + CRYPTO_MAX_ALG_NAME || + snprintf(inst->alg.base.cra_driver_name, CRYPTO_MAX_ALG_NAME, + "pkcs1pad(%s,%s)", + rsa_alg->base.cra_driver_name, hash_name) >= + CRYPTO_MAX_ALG_NAME) goto out_drop_alg; - } else { - if (snprintf(inst->alg.base.cra_name, - CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s,%s)", - rsa_alg->base.cra_name, hash_name) >= - CRYPTO_MAX_ALG_NAME || - snprintf(inst->alg.base.cra_driver_name, - CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s,%s)", - rsa_alg->base.cra_driver_name, hash_name) >= - CRYPTO_MAX_ALG_NAME) - goto out_free_hash; - } inst->alg.base.cra_flags = rsa_alg->base.cra_flags & CRYPTO_ALG_ASYNC; inst->alg.base.cra_priority = rsa_alg->base.cra_priority; @@ -738,12 +717,10 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb) err = akcipher_register_instance(tmpl, inst); if (err) - goto out_free_hash; + goto out_drop_alg; return 0; -out_free_hash: - kfree(ctx->hash_name); out_drop_alg: crypto_drop_akcipher(spawn); out_free_inst:
The only user of rsa-pkcs1pad always uses the hash so there is no reason to support the case of not having a hash. This patch also changes the digest info lookup so that it is only done once during template instantiation rather than on each operation. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/rsa-pkcs1pad.c | 83 ++++++++++++++++++-------------------------------- 1 file changed, 30 insertions(+), 53 deletions(-) -- 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