Re: [PATCH 01/18] X.509: Parse RSASSA-PSS style certificates

[Varad Gautam <varad.gautam@suse.com>]

On 4/8/21 4:29 AM, hongbo li wrote:
> Hi Varad,
> 
> Varad Gautam <varad.gautam@suse.com <mailto:varad.gautam@suse.com>> 于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@gmail.com/
[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@gmail.com/ <https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-3-git-send-email-herbert.tencent@gmail.com/>
>> [2] https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-5-git-send-email-herbert.tencent@gmail.com/ <https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-5-git-send-email-herbert.tencent@gmail.com/>
>> [3] https://patchwork.kernel.org/project/linux-crypto/patch/20210330202829.4825-2-varad.gautam@suse.com/ <https://patchwork.kernel.org/project/linux-crypto/patch/20210330202829.4825-2-varad.gautam@suse.com/>
>> [4] https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-4-git-send-email-herbert.tencent@gmail.com/ <https://patchwork.kernel.org/project/linux-crypto/patch/1617802906-30513-4-git-send-email-herbert.tencent@gmail.com/>
>>
>> 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