Flag modified when X509_req_info_st attribute updated by gibeom-gwon · Pull Request #18879 · openssl/openssl

gibeom-gwon · 2022-07-26T15:20:47Z

If X509_REQ is created by PEM_read_X509_REQ and only the attribute is changed, it is not exported as intended by PEM_write_X509_REQ Because the non-updated enc is used.
This patch turns on the modified flag when an attribute change occurs.

To reproduce:

Create a CSR file with extensions.

Certificate Request:
    Data:
        Version: 1 (0x0)
        Subject: C = KR, ST = dkfs, L = kdslf, O = klsdfjkldfjsd, OU = kdlfsjfdskl, CN = kfldsjdfs
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                RSA Public-Key: (2048 bit)
                Modulus:
                    00:c8:25:ae:fe:f2:a7:9a:54:c8:ce:6f:1f:d6:10:
                    0d:b7:7c:e2:0b:fe:c3:bc:68:f0:86:52:45:50:2b:
                    d5:f4:82:b8:3d:ab:85:14:0b:4e:6c:b7:00:12:e6:
                    9e:a8:98:29:32:b6:c2:a1:85:10:4c:93:68:d2:4e:
                    4e:04:df:e3:4f:53:da:ea:da:67:30:ee:d2:6b:15:
                    74:55:f2:7a:43:1c:1a:02:18:62:98:14:6f:b7:1d:
                    49:d4:e5:60:db:ca:80:43:34:28:c9:aa:79:fe:38:
                    5f:34:66:2f:53:d5:b5:6e:05:14:6e:97:c2:cb:d3:
                    1f:76:80:a0:bf:f9:86:96:02:00:4a:ac:6c:4d:ab:
                    1b:59:46:46:bd:46:53:9f:02:ca:45:86:6d:95:75:
                    ec:51:41:25:48:11:3d:72:b1:05:22:0c:61:b4:74:
                    2e:b0:09:1e:99:cb:2a:fd:e0:9e:3f:c4:96:85:c0:
                    24:0d:db:50:ca:74:74:5c:e2:b7:23:9f:12:e1:01:
                    24:39:6e:86:63:6b:1c:51:18:2d:ec:32:03:03:28:
                    85:b4:3e:56:42:fd:d0:2c:36:ab:f6:14:89:2b:8e:
                    55:a9:0a:63:29:3e:33:ce:a8:25:ca:55:8c:a6:6d:
                    56:47:f3:9d:88:31:82:60:fd:1e:5f:12:71:65:47:
                    1d:e1
                Exponent: 65537 (0x10001)
        Attributes:
        Requested Extensions:
            X509v3 Basic Constraints: critical
                CA:FALSE
            X509v3 Extended Key Usage: 
                TLS Web Server Authentication, TLS Web Client Authentication, Code Signing, E-mail Protection
            X509v3 Key Usage: 
                Digital Signature, Non Repudiation, Key Encipherment
    Signature Algorithm: sha256WithRSAEncryption
         06:85:a4:ff:19:a0:63:5e:ed:69:99:37:20:3d:ab:1f:c7:86:
         e2:d9:98:70:ca:0e:b3:21:52:66:99:f6:b1:06:22:a7:9d:40:
         7a:56:e4:1e:73:e8:e5:de:96:7f:e6:31:09:e3:5d:17:a5:4f:
         3d:ac:93:de:d7:4c:bc:1b:1c:db:d4:c9:2e:a9:f3:d1:81:8c:
         e1:60:97:af:51:59:d4:12:81:41:ba:b6:7b:c0:a1:f7:4d:87:
         b0:a5:12:aa:11:94:0b:5d:23:4a:b8:19:e8:ba:80:3e:c5:5c:
         59:b5:e7:41:21:b8:8a:2c:c4:39:5b:0c:29:92:62:cc:d6:8a:
         b8:11:27:9c:f8:09:4d:21:88:54:ea:ff:97:70:a3:43:57:06:
         a5:01:ed:64:d9:b5:7f:2d:ff:a2:05:e8:81:46:6d:7c:01:52:
         02:df:bb:04:91:e5:3f:2f:84:2f:71:b5:54:6d:0e:b2:f7:98:
         bf:c6:1e:b8:1f:03:71:7e:9f:1c:2d:b6:2e:5e:d1:e3:a8:27:
         71:de:ed:74:12:ce:44:b5:90:79:15:04:ff:a3:09:b7:8e:2d:
         6c:4f:e0:f8:62:56:a9:3b:f2:a6:12:60:84:95:58:2b:d6:c4:
         c5:48:63:f6:5c:4f:3c:e4:c0:b9:25:81:94:eb:1c:11:a8:22:
         84:37:d4:03
-----BEGIN CERTIFICATE REQUEST-----
MIIDEjCCAfoCAQAwbjELMAkGA1UEBhMCS1IxDTALBgNVBAgMBGRrZnMxDjAMBgNV
BAcMBWtkc2xmMRYwFAYDVQQKDA1rbHNkZmprbGRmanNkMRQwEgYDVQQLDAtrZGxm
c2pmZHNrbDESMBAGA1UEAwwJa2ZsZHNqZGZzMIIBIjANBgkqhkiG9w0BAQEFAAOC
AQ8AMIIBCgKCAQEAyCWu/vKnmlTIzm8f1hANt3ziC/7DvGjwhlJFUCvV9IK4PauF
FAtObLcAEuaeqJgpMrbCoYUQTJNo0k5OBN/jT1Pa6tpnMO7SaxV0VfJ6QxwaAhhi
mBRvtx1J1OVg28qAQzQoyap5/jhfNGYvU9W1bgUUbpfCy9MfdoCgv/mGlgIASqxs
TasbWUZGvUZTnwLKRYZtlXXsUUElSBE9crEFIgxhtHQusAkemcsq/eCeP8SWhcAk
DdtQynR0XOK3I58S4QEkOW6GY2scURgt7DIDAyiFtD5WQv3QLDar9hSJK45VqQpj
KT4zzqglylWMpm1WR/OdiDGCYP0eXxJxZUcd4QIDAQABoF8wXQYJKoZIhvcNAQkO
MVAwTjAMBgNVHRMBAf8EAjAAMDEGA1UdJQQqMCgGCCsGAQUFBwMBBggrBgEFBQcD
AgYIKwYBBQUHAwMGCCsGAQUFBwMEMAsGA1UdDwQEAwIF4DANBgkqhkiG9w0BAQsF
AAOCAQEABoWk/xmgY17taZk3ID2rH8eG4tmYcMoOsyFSZpn2sQYip51AelbkHnPo
5d6Wf+YxCeNdF6VPPayT3tdMvBsc29TJLqnz0YGM4WCXr1FZ1BKBQbq2e8Ch902H
sKUSqhGUC10jSrgZ6LqAPsVcWbXnQSG4iizEOVsMKZJizNaKuBEnnPgJTSGIVOr/
l3CjQ1cGpQHtZNm1fy3/ogXogUZtfAFSAt+7BJHlPy+EL3G1VG0OsveYv8YeuB8D
cX6fHC22Ll7R46gncd7tdBLORLWQeRUE/6MJt44tbE/g+GJWqTvyphJghJVYK9bE
xUhj9lxPPOTAuSWBlOscEagihDfUAw==
-----END CERTIFICATE REQUEST-----

Compile and run this C code. This code removes the requested extensions and save as edited.csr

#include <stdio.h>
#include <openssl/crypto.h>
#include <openssl/x509v3.h>
#include <openssl/pem.h>
#include <openssl/err.h>

int main()
{
        /*
        FILE *f = fopen("privekey.key","rb");
        EVP_PKEY *privkey = NULL;
        if(PEM_read_PrivateKey(f,&privkey,NULL,NULL) == NULL)
                printf("error\n");
        fclose(f);
        */

        FILE *f = fopen("test.csr","rb");
        X509_REQ *req = NULL;
        if(PEM_read_X509_REQ(f,&req,NULL,NULL) == NULL)
                printf("x509 req error\n");
        fclose(f);

        int attr_len = X509_REQ_get_attr_count(req);
        printf("count : %d\n",attr_len);

        X509_REQ_delete_attr(req,0);

        attr_len = X509_REQ_get_attr_count(req);
        printf("count : %d\n",attr_len);

        /*
        if(X509_REQ_sign(req,privkey,NULL) == 0)
                printf("sign error\n");
        */

        f = fopen("edited.csr","wb");
        PEM_write_X509_REQ(f,req);
        fclose(f);
        return 0;
}

edited.csr still has Requested Extensions

openssl-machine · 2022-08-27T00:08:45Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

DDvO

Thanks for your nice error report and fix.
I'm asking for further improvements and two small extensions of this PR.

crypto/x509/x509_req.c

gibeom-gwon · 2022-08-27T10:49:41Z

@DDvO Thank you for your review. Edited as requested. By the way, should X509_REQ_sign{,_ctx} also flag modified if ASN1_item_sign{_ctx} call was successful? Then X509_sign{,_ctx} also to be changed I think. Currently I edited same as X509_sign{,_ctx}.

DDvO

Very nice that you reacted that swiftly on the change requests I gave earlier today.
See my follow-up comment and my answer on your good question.

crypto/x509/x509_req.c

crypto/x509/x_all.c

gibeom-gwon · 2022-08-27T11:52:40Z

Edited as requested. I made one more commit for the functions in x_all.c.

DDvO

A further round of comments.

crypto/x509/x509_req.c

crypto/x509/x_all.c

crypto/x509/x509_req.c

…a updated We need to reencode X509_req_info_st if member data updated.

… successful

gibeom-gwon · 2022-08-27T13:59:41Z

Sorry for the multiple mistakes. There will be no more mistakes in this round. (maybe)

DDvO

All good now - thanks!

DDvO · 2022-08-29T07:07:30Z

@t8m could you please have a look again.

t8m · 2022-08-29T10:40:58Z

Can these commits be directly cherry-picked to master and 3.0 branches? If not, we'll need PRs against these branches as well before merging this.

DDvO · 2022-08-29T11:19:08Z

Can these commits be directly cherry-picked to master and 3.0 branches? If not, we'll need PRs against these branches as well before merging this.

Unfortunately not - I'm preparing a respective PR for 3.0+.

DDvO · 2022-08-29T12:05:24Z

Can these commits be directly cherry-picked to master and 3.0 branches?
If not, we'll need PRs against these branches as well before merging this.

Unfortunately not - I'm preparing a respective PR for 3.0+.

Done in #19090 and restricted this PR here to 1.1.1, so it can be fully approved now.

DDvO · 2022-08-29T17:25:32Z

@t8m please re-approve (for 1.1.1).

t8m · 2022-08-29T17:57:42Z

Please merge only along the other PR.

openssl-machine · 2022-08-30T18:00:15Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

gibeom-gwon · 2022-09-23T05:23:24Z

Since #19090 has been merged, it seems that this PR can also be merged.

…a updated We need to reencode X509_req_info_st if member data updated. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #18879)

… successful Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #18879)

DDvO · 2022-09-24T14:55:23Z

Merged - thanks @t8m for the OTC approval
and @gibeom-gwon for the fix and the reminder that this can be merged now.

davidben · 2022-10-11T16:15:54Z

FYI, this seems to have broken a couple of things: #19299 and now #19388

…a updated We need to reencode X509_req_info_st if member data updated. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from openssl#18879)

… successful Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from openssl#18879)

This is a variant of openssl/openssl#18879 for 3.0 and master, rebasing that PR from 1.1.1. On this occasion, I added a further commit that to add some NULL parameter checks and improve coding style in crypto/x509/{x509_req,x_all}.c Include 3 commits: X509 x509_req.c: Set 'modified' flag when X509_req_info_st member data updated X509 x_all.c: Set 'modified' flag when ASN1_item_sign{,_ctx} call was successful crypto/x509/{x509_req,x_all}.c: add some NULL parameter checks, improve coding style (Merged from openssl/openssl#19090)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jul 26, 2022

gibeom-gwon changed the base branch from OpenSSL_1_1_1-stable to master July 26, 2022 23:47

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jul 26, 2022

gibeom-gwon changed the base branch from master to OpenSSL_1_1_1-stable July 26, 2022 23:47

openssl-machine added hold: cla required The contributor needs to submit a license agreement and removed hold: cla required The contributor needs to submit a license agreement labels Jul 26, 2022

t8m approved these changes Jul 27, 2022

View reviewed changes

t8m removed the approval: otc review pending label Jul 27, 2022

DDvO requested changes Aug 27, 2022

View reviewed changes

crypto/x509/x509_req.c Outdated Show resolved Hide resolved

crypto/x509/x509_req.c Show resolved Hide resolved

gibeom-gwon force-pushed the fix_x509_req branch from 71b22d6 to 5fceabf Compare August 27, 2022 10:49

DDvO added the approval: otc review pending label Aug 27, 2022

DDvO requested changes Aug 27, 2022

View reviewed changes

crypto/x509/x509_req.c Outdated Show resolved Hide resolved

crypto/x509/x_all.c Outdated Show resolved Hide resolved

gibeom-gwon force-pushed the fix_x509_req branch from 5fceabf to 0755c9f Compare August 27, 2022 11:52

DDvO requested changes Aug 27, 2022

View reviewed changes

crypto/x509/x509_req.c Outdated Show resolved Hide resolved

crypto/x509/x509_req.c Outdated Show resolved Hide resolved

crypto/x509/x_all.c Outdated Show resolved Hide resolved

crypto/x509/x_all.c Show resolved Hide resolved

DDvO reviewed Aug 27, 2022

View reviewed changes

crypto/x509/x509_req.c Outdated Show resolved Hide resolved

gibeom-gwon added 2 commits August 27, 2022 22:27

X509 x509_req.c: Set 'modified' flag when X509_req_info_st member dat…

b8050b4

…a updated We need to reencode X509_req_info_st if member data updated.

X509 x_all.c: Set 'modified' flag when ASN1_item_sign{,_ctx} call was…

c917e26

… successful

gibeom-gwon force-pushed the fix_x509_req branch from 0755c9f to c917e26 Compare August 27, 2022 13:59

DDvO approved these changes Aug 27, 2022

View reviewed changes

DDvO removed the approval: review pending This pull request needs review by a committer label Aug 27, 2022

DDvO requested a review from t8m August 27, 2022 14:07

DDvO mentioned this pull request Aug 29, 2022

X509: Flag modified when X509_req_info_st updated - follow-up for 3.0+ #19090

Closed

DDvO removed branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch labels Aug 29, 2022

t8m approved these changes Aug 29, 2022

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending labels Aug 29, 2022

openssl-machine pushed a commit that referenced this pull request Sep 24, 2022

X509 x_all.c: Set 'modified' flag when ASN1_item_sign{,_ctx} call was…

748df18

… successful Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #18879)

DDvO closed this Sep 24, 2022

davidben mentioned this pull request Oct 11, 2022

Regression: X509_sign, etc., no longer implicitly refresh the cached TBSCertificate #19388

Closed

Uh oh!

Comments

Conversation

gibeom-gwon commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openssl-machine commented Aug 27, 2022

Uh oh!

DDvO left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gibeom-gwon commented Aug 27, 2022

Uh oh!

DDvO left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gibeom-gwon commented Aug 27, 2022

Uh oh!

DDvO left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gibeom-gwon commented Aug 27, 2022

Uh oh!

DDvO left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO commented Aug 29, 2022

Uh oh!

t8m commented Aug 29, 2022

Uh oh!

DDvO commented Aug 29, 2022

Uh oh!

DDvO commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Aug 29, 2022

Uh oh!

t8m commented Aug 29, 2022

Uh oh!

openssl-machine commented Aug 30, 2022

Uh oh!

gibeom-gwon commented Sep 23, 2022

Uh oh!

DDvO commented Sep 24, 2022

Uh oh!

davidben commented Oct 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gibeom-gwon commented Jul 26, 2022 •

edited

Loading

DDvO commented Aug 29, 2022 •

edited

Loading