On 4/8/21 4:29 AM, hongbo li wrote: > Hi Varad, > > Varad Gautam <varad.gautam@xxxxxxxx <mailto:varad.gautam@xxxxxxxx>> 于2021年4月8日周四 上午5:20写道： >> >> Hi Hongbo, >> >> On 4/7/21 10:27 AM, hongbo li wrote: >> > Hello Varad， >> > >> > I also made an implementation of rsa pss: "[PATCH v3 0/4] crypto: add >> > rsa pss support for x509". >> > I notice your patches and did some review, find the following >> > differences between our patches: >> > 1. You rework the rsa pad framework. This is reasonable. >> > 2. You did some changes on the keyctl and asymmetric struct. I don't >> > see the reason. >> > Because for x509 layer, it only need to know the hash param, and >> > could ignore other params(salt len, mgfhash). >> > Let rsa-pss itself parse the pss related params. So it seems we >> > don't need to change asymmetric's >> > common struct. >> >> A signature might be generated with a different set of params than those >> used for signing the x509 certificate that wraps the corresponding pubkey. >> In this case, using the params that came in when the pubkey was loaded, >> instead of params for the actual signature would be incorrect. I see >> struct public_key_signature as the right place to store such state, >> regardless of where the signature came from (detached or selfsigned). >> > > As what the comments in x509_note_params() say: > In crypto/asymmetric_keys/x509.asn1, AlgorithmIdentifier is used three times : > 1. The signature AlgorithmIdentifier in TBSCertificate. > 2. The algorithm in SubjectPublicKeyInfo > 3. The signatureAlgorithm after tbsCertificate. > When the pubkey was loaded, it is the third one. According to rfc5280 [1], > the third has the same value as the first one. Your patch use the first, and I > use the third, I think both are fine. > Consider the following to illustrate my point: # Generate a key pair: slen=20 mgfhash=sha384 openssl req -x509 -newkey rsa:4096 -nodes -keyout private.pem -out pss-0.der \ -days 100 -outform der -config x509.genkey -sha384 -sigopt rsa_padding_mode:pss \ -sigopt rsa_pss_saltlen:$slen -sigopt rsa_mgf1_md:$mgfhash openssl x509 -in pss-0.der -inform der -pubkey -noout > public.pem # Sign some data: echo data > data.txt slen=30 mgfhash=sha256 openssl dgst -sha384 -sign private.pem -sigopt rsa_padding_mode:pss \ -sigopt rsa_pss_saltlen:$slen -sigopt rsa_mgf1_md:$mgfhash \ -out sig.bin data.txt sig.bin has a different slen and mgfhash vs the signature stored in pss-0.der. Since psspad_set_pub_key() here [1] will unpack the params that correspond to pss-0.der, verifying sig.bin (eg via keyctl pkey_verify) would fail. >> For the same reason, I also prefer the parsing machinery for signature >> params be kept in x509_cert_parser instead of unpacking a buffer in the >> PSS akcipher's set_pub_key implementation [1]. Going that way, we also end >> up parsing these params twice, since x509 needs to unpack the hash >> algorithm in a pss-specific way anyway. >> > > Yes, my patch needs to parse the params twice, my purpose is to make small > change to x509 layer. > >> For the IMA usecase, since x509_key_preparse() would have already filled >> in the params in public_key_signature, asymmetric_verify should be able >> to find and set these from key->payload before calling verify_signature(). >> >> > 3. Why reject the cert whose MGF is different from the hash function >> > used for signature generation? >> > My implementation could support different hashes, so don't get your point. >> >> The verify operation (psspad_verify_complete [3]) in theory supports it, >> which I've tested against such certificates crafted via openssl. >> >> I chose to reject such certificates early on during x509 parsing since, >> - these are not a common occurence in practice, and >> - testing (besides via openssl) without a set of reference vectors to harden >> the verification against seemed insufficient. >> >> I've had some more test runs complete in the meantime, and I'll drop that >> check in the next round. >> >> > 4. I add a test vector and a patch to support using rsa-pss for iam. >> > 5. Other implementation difference, i.e. the mgf and verify functions. >> > >> > Maybe we could merge our patches, what's your opinion? >> > >> >> Sounds good. I'll send out a v2 soon, and if you agree, the test vector [4] >> and IMA [5] can go on top of it? > > Sure, Thank you. I've posted a v2 at [2]. [1] https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-3-git-send-email-herbert.tencent@xxxxxxxxx/ [2] https://lkml.org/lkml/2021/4/8/775 Regards, Varad >> >> [1] https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-3-git-send-email-herbert.tencent@xxxxxxxxx/ <https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-3-git-send-email-herbert.tencent@xxxxxxxxx/> >> [2] https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-5-git-send-email-herbert.tencent@xxxxxxxxx/ <https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-5-git-send-email-herbert.tencent@xxxxxxxxx/> >> [3] https://patchwork.kernel.org/project/linux-crypto/patch/20210330202829.4825-2-varad.gautam@xxxxxxxx/ <https://patchwork.kernel.org/project/linux-crypto/patch/20210330202829.4825-2-varad.gautam@xxxxxxxx/> >> [4] https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-4-git-send-email-herbert.tencent@xxxxxxxxx/ <https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-4-git-send-email-herbert.tencent@xxxxxxxxx/> >> >> Regards, >> Varad >> >> > Best regards >> > > > [1] https://tools.ietf.org/html/rfc5280#section-4.1.1.2 <https://tools.ietf.org/html/rfc5280#section-4.1.1.2> > > Best Regards > Hongbo > -- SUSE Software Solutions Germany GmbH Maxfeldstr. 5 90409 Nürnberg Germany HRB 36809, AG Nürnberg Geschäftsführer: Felix Imendörffer
