CAdES: Fix SignerInfo attribute construction order.#8117
CAdES: Fix SignerInfo attribute construction order.#8117FdaSilvaYY wants to merge 2 commits intoopenssl:masterfrom
Conversation
FdaSilvaYY
force-pushed
the
cades/fixup
branch
4 times, most recently
from
dabcde4 to
30ae235
Compare
FdaSilvaYY
force-pushed
the
cades/fixup
branch
2 times, most recently
from
7ae8042 to
76561db
Compare
|
Ping @openssl: small fix quick to review. |
crypto/cms/cms_sd.c
Outdated
There was a problem hiding this comment.
This code-block was just move down.
FdaSilvaYY
force-pushed
the
cades/fixup
branch
2 times, most recently
from
b913df4 to
18428c5
Compare
FdaSilvaYY
force-pushed
the
cades/fixup
branch
from
18428c5 to
d2833e2
Compare
There was a problem hiding this comment.
I discovered this bug also while trying to fix another issue in cms related to signed attributes.
So your fix is good since adding extra signers with -cades enabled resulted in the attribute being added after the digest was calculated resulting in a incorrect signature.
Please add a test to test/recipes/80-test_cms.t (see #8944)
The test you need to add should be something like the following
./apps/openssl cms -sign -binary -nodetach -cades -nosmimecap -stream -in ./test/smcont.txt -outform DER -signer ./test/smime-certs/smrsa1.pem -md SHA256 -out test.cms
./apps/openssl cms -resign -binary -cades -nosmimecap -nodetach -in test.cms -inform DER -outform DER -signer ./test/smime-certs/smrsa2.pem -md SHA256 -out test2.cms
./apps/openssl cms -verify -in test2.cms -inform DER -CAfile ./test/smime-certs/smroot.pem -out smtst.txt
|
The travis failures are relevant. |
|
@FdaSilvaYY is this code in 1_1 also? |
|
The travis failure is related to the failing extended test (which is a known failure currently) |
No, #7893 was just merged into master |
mattcaswell
added
the
approval: done
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #8117)
Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #8117)
|
Merged. Thanks @FdaSilvaYY. You probably will want to rebase your other PR now that this one has gone in. |
3 participants
Small fixup on the recent CAdES contribution.
Can be trigger if someone is using CMS_REUSE_DIGEST flag value.
Not a big issue, just a lack of logic ;)
Checklist