Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAdES: implement CAdES-BES signed attributes validation #8098

Closed
wants to merge 6 commits into from

Conversation

@FdaSilvaYY
Copy link
Contributor

@FdaSilvaYY FdaSilvaYY commented Jan 27, 2019

CAdES-BES Signature Validation feature following #7893.

The core validation code comes from PR #771. this PR mostly shared and reuse its code.

Checklist
  • documentation is added or updated
  • tests are added or updated
@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch 2 times, most recently Jan 27, 2019
Copy link
Member

@levitte levitte left a comment

First quick read

crypto/cms/cms_ess.c Outdated Show resolved Hide resolved
crypto/cms/cms_ess.c Outdated Show resolved Hide resolved
@levitte
Copy link
Member

@levitte levitte commented Jan 28, 2019

Apart from my comment, can you have a look at Travis and figure out what's going on? Them are a few too many unresolved symbols for my comfort...

@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch 2 times, most recently Jan 28, 2019
@FdaSilvaYY
Copy link
Contributor Author

@FdaSilvaYY FdaSilvaYY commented Jan 28, 2019

Apart from my comment, can you have a look at Travis and figure out what's going on? Them are a few too many unresolved symbols for my comfort...

Fully fixed, I should not edit my code so late ;)
CI's are Green.

@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch Jan 28, 2019
@maxcuttins
Copy link

@maxcuttins maxcuttins commented Feb 17, 2019

any news about this?

@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch 2 times, most recently Feb 17, 2019
@FdaSilvaYY
Copy link
Contributor Author

@FdaSilvaYY FdaSilvaYY commented Feb 17, 2019

@maxcuttins : needs a bit of documents about the two added methods.
May be it is possible to get the signed attributes in a more fashion way .
Otherwise, the core changes are here.
I rebased this PR on top of #8117 to ease the merge.

@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch 2 times, most recently Feb 17, 2019
@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch Feb 27, 2019
@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch Mar 10, 2019
@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch 2 times, most recently Apr 4, 2019
@FdaSilvaYY FdaSilvaYY changed the title CAdes: implement CAdES-BES signed attributes validation CAdES: implement CAdES-BES signed attributes validation May 25, 2019
@FdaSilvaYY FdaSilvaYY mentioned this pull request May 25, 2019
0 of 2 tasks complete
@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch 3 times, most recently May 25, 2019
@FdaSilvaYY
Copy link
Contributor Author

@FdaSilvaYY FdaSilvaYY commented May 25, 2019

Ping @slontis : suggested tests added.

@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch May 25, 2019
@slontis
Copy link
Contributor

@slontis slontis commented May 27, 2019

Is this PR dependant on PR #8117 or should #8117 just be closed?

apps/cms.c Show resolved Hide resolved
@slontis
Copy link
Contributor

@slontis slontis commented May 27, 2019

Just a general comment about the generated files..
I have started keeping a seperate commit for the generated files (since they quite often need to be updated when other PR's are merged). That way you can then

  • rebase and then remove the generated files commit
  • make update and then add the generated files again and commit.
apps/cms.c Outdated Show resolved Hide resolved
@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch May 10, 2020
@FdaSilvaYY
Copy link
Contributor Author

@FdaSilvaYY FdaSilvaYY commented May 11, 2020

Fully rebased. CI are all green.
Just need to think a bit more about no-ts, no-cms interaction with these changes.

@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch May 16, 2020
@opensignature
Copy link
Contributor

@opensignature opensignature commented May 17, 2020

with make test TESTS=test_x509 V=1
there is this error:
crypto/pem/pvkfmt.c:42:18: runtime error: left shift of 137 by 24 places cannot be represented in type 'int'

@mspncp
Copy link
Contributor

@mspncp mspncp commented May 17, 2020

with make test TESTS=test_x509 V=1
there is this error:
crypto/pem/pvkfmt.c:42:18: runtime error: left shift of 137 by 24 places cannot be represented in type 'int'

Thanks @opensignature. @FdaSilvaYY already reported this issue in #11853.

Copy link
Member

@t8m t8m left a comment

Some minor things to resolve.

crypto/cms/cms_smime.c Outdated
X509 *signer;
int i, scount = 0, ret = 0;
BIO *cmsbio = NULL, *tmpin = NULL, *tmpout = NULL;
unsigned char cadesVerify = (flags & CMS_CADES) ? 1 : 0;

This comment has been minimized.

@t8m

t8m May 19, 2020
Member

Nit: It is weird to "optimize" boolean value into unsigned char. The int type is usually used in place of boolean throughout the OpenSSL code. Also I would slightly prefer using (flags & CMS_CADES) != 0 instead of the ? operator.

crypto/cms/cms_ess.c Show resolved Hide resolved
crypto/cms/cms_smime.c Outdated
CMSerr(CMS_F_CMS_VERIFY, ERR_R_MALLOC_FAILURE);
goto err;
}
}
cms_certs = CMS_get1_certs(cms);
if (!(flags & CMS_NOCRL))
crls = CMS_get1_crls(cms);
for (i = 0; i < sk_CMS_SignerInfo_num(sinfos); i++) {

This comment has been minimized.

@t8m

t8m May 19, 2020
Member

Optional: at this point scount == sk_CMS_SignerInfo_num(sinfos) so you could use scount here to optimize.

crypto/ess/build.info Show resolved Hide resolved
crypto/ess/ess_lib.c Outdated
|| X509_NAME_cmp(issuer->d.dirn, X509_get_issuer_name(cert)) != 0)
return -1;

if (ASN1_INTEGER_cmp(is->serial, X509_get_serialNumber((X509 *)cert)) != 0)

This comment has been minimized.

@t8m

t8m May 19, 2020
Member

Would it make sense to just return ASN1_INTEGER_cmp(is->serial, X509_get_serialNumber((X509 *)cert)); here?

crypto/ess/ess_lib.c Outdated
X509_digest(cert, EVP_sha1(), cert_sha1, NULL);

/* Recompute SHA1 hash of certificate if necessary (side effect). */
X509_check_purpose(cert, -1, 0);

This comment has been minimized.

@t8m

t8m May 19, 2020
Member

This was now changed. You can also use X509v3_cache_extensions() directly.

const ESS_CERT_ID_V2 *cid = sk_ESS_CERT_ID_V2_value(cert_ids, i);
const EVP_MD *md;

if (cid != NULL && cid->hash_alg != NULL)

This comment has been minimized.

@t8m

t8m May 19, 2020
Member

This will require change to use fetch. Perhaps just mark with TODO 3.0.

/* Recompute SHA1 hash of certificate if necessary (side effect). */
X509_check_purpose(cert, -1, 0);

if (!X509_digest(cert, EVP_sha1(), cert_sha1, NULL))

This comment has been minimized.

@t8m

t8m May 19, 2020
Member

This will have to be changed for 3.0 to use fetch. Please add TODO 3.0 comment.

This comment has been minimized.

@FdaSilvaYY

FdaSilvaYY May 22, 2020
Author Contributor

fixed !

@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch May 22, 2020
return -1;

/* Recompute SHA1 hash of certificate if necessary (side effect). */
if (!X509v3_cache_extensions(cert, NULL, NULL))

This comment has been minimized.

@bernd-edlinger

bernd-edlinger May 22, 2020
Member

I think that X509_digest should do this automatically.

FdaSilvaYY added 6 commits Jun 12, 2019
for signing certificate V2 and signing certificate extensions.

CAdES : lowercase name for now internal methods.
about the two new added methods and two new error codes.
[extended tests]
…ning certificate V2 and signing certificate extensions.
@FdaSilvaYY FdaSilvaYY force-pushed the FdaSilvaYY:add-cades-verify branch to b08c846 May 23, 2020
@FdaSilvaYY
Copy link
Contributor Author

@FdaSilvaYY FdaSilvaYY commented May 23, 2020

Travis builds are Green, except usual s390 "no space left" ...

@t8m
t8m approved these changes May 25, 2020
Copy link
Member

@t8m t8m left a comment

Good work!

@t8m
Copy link
Member

@t8m t8m commented May 25, 2020

@slontis Can you please re-approve?

Copy link
Contributor

@slontis slontis left a comment

Reapproved..

Thanks for your patience with this PR @FdaSilvaYY.

@mspncp
Copy link
Contributor

@mspncp mspncp commented May 26, 2020

This pull request is ready to merge.

openssl-machine pushed a commit that referenced this pull request May 27, 2020
for signing certificate V2 and signing certificate extensions.

CAdES: lowercase name for now internal methods.

crypto/cms: generated file changes.

Add some CHANGES entries.

[extended tests]

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #8098)
@t8m
Copy link
Member

@t8m t8m commented May 27, 2020

Squashed and merged as 9e3c510. Thank you very much for the PR and your patience when waiting for the reviews and merge.

@t8m t8m closed this May 27, 2020
@FdaSilvaYY FdaSilvaYY deleted the FdaSilvaYY:add-cades-verify branch May 27, 2020
@FdaSilvaYY
Copy link
Contributor Author

@FdaSilvaYY FdaSilvaYY commented May 27, 2020

Thanks for all your reviews and remarks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants