Check error return from cms_sd_asn1_ctrl() correctly.

[paulidale]

Fixes #21986

• documentation is added or updated
• tests are added or updated

[botovq]

While this is a possible approach, I think it would make more sense to fix cms_generic_sign() to behave the same way as ossl_rsa_cms_sign() (i.e., return 0/1), or to do return cms_generic_sign(si, cmd) > 0; twice.

Otherwise the fixup of the return value after the ->pkey_ctrl() call is a bit strange.

openssl/crypto/cms/cms_sd.c

Lines 259 to 282 in 25b7025

	static int cms_sd_asn1_ctrl(CMS_SignerInfo *si, int cmd)
	{
	EVP_PKEY *pkey = si->pkey;
	int i;
	if (EVP_PKEY_is_a(pkey, "DSA") || EVP_PKEY_is_a(pkey, "EC"))
	return cms_generic_sign(si, cmd);
	else if (EVP_PKEY_is_a(pkey, "RSA") || EVP_PKEY_is_a(pkey, "RSA-PSS"))
	return ossl_cms_rsa_sign(si, cmd);
	/* Now give engines, providers, etc a chance to handle this */
	if (pkey->ameth == NULL || pkey->ameth->pkey_ctrl == NULL)
	return cms_generic_sign(si, cmd);
	i = pkey->ameth->pkey_ctrl(pkey, ASN1_PKEY_CTRL_CMS_SIGN, cmd, si);
	if (i == -2) {
	ERR_raise(ERR_LIB_CMS, CMS_R_NOT_SUPPORTED_FOR_THIS_KEY_TYPE);
	return 0;
	}
	if (i <= 0) {
	ERR_raise(ERR_LIB_CMS, CMS_R_CTRL_FAILURE);
	return 0;
	}
	return 1;
	}

[paulidale]

Yeah, that's a nicer approach I think.

[paulidale]

ossl_cms_rsa_sign() can also return -1 on error.

Anyway, updated.

[botovq]

I'm pretty sure it doesn't. I first thought so, too, but then noticed that here is a > 0 hiding at the end:

openssl/crypto/cms/cms_rsa.c

Line 246 in 025535e

return ossl_rsa_pss_to_ctx(NULL, pkctx, alg, NULL) > 0;

X509_ALGOR_set0 returns 0/1, so I think we're good.

[slontis]

Please add a testcase..

The following commandline returns -1 from cms_generic_sign..
It then segfaults inside the shake ctrl.
openssl cms -sign -in test/smcont.txt -signer test/smime-certs/smdsa1.pem -md SHAKE256

[pauli: edited to correct the test case]

[botovq]

It's your codebase and the diff fixes the bug. However, if I was to write the fix, I would change the -1 returns in cms_generic_sign() to 0 returns. Then making things complicated with the three added > 0 is unnecessary. We've seen that doing this is confusing in this very thread. I also do not know why you'd want two internal APIs that are supposed to do the same thing that have different API contracts. So I think aligning the APIs is cleaner, leads to simpler code and makes it easier to add new things. Consistency matters.

[slontis]

I dont mind either way it is done. Currently I cant see a path where ossl_cms_rsa_sign() could return -1. But the > check doesnt hurt anything..

[paulidale]

Thrice merged with conflicts resolved.

[bernd-edlinger]

the merge commit 41136a9 to 3.0 branch removed the semicolon:
plan tests => 17
therefore now the test_cms test case is faliing due to syntax.

[paulidale]

Oops. Thanks for pointhing this out. Fixed by #22045 already.

[bernd-edlinger]

Okay, but it still fails for no-des doesn't it?

[bernd-edlinger]

i meant no-dsa ....