Skip to content

Comments

Fix regression in i2d_re_X509_REQ_tbs()#19299

Closed
t8m wants to merge 2 commits intoopenssl:masterfrom
t8m:fix-i2d-re-x509-req-tbs
Closed

Fix regression in i2d_re_X509_REQ_tbs()#19299
t8m wants to merge 2 commits intoopenssl:masterfrom
t8m:fix-i2d-re-x509-req-tbs

Conversation

@t8m
Copy link
Member

@t8m t8m commented Sep 29, 2022

This fixes regression from commit 8e39049.

Fixes #19297

@t8m t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: bug The issue/pr is/fixes a bug branch: 3.0 Applies to openssl-3.0 branch severity: regression The issue/pr is a regression from previous released version labels Sep 29, 2022
@t8m t8m requested a review from DDvO September 29, 2022 11:01
@t8m t8m mentioned this pull request Sep 29, 2022
Closed
@t8m
Fix regression in i2d_re_X509_REQ_tbs()
881e094 
This fixes regression from commit 8e39049. There is also no point
in setting the modified flag after just calling i2d.

Fixes openssl#19297
@t8m t8m force-pushed the fix-i2d-re-x509-req-tbs branch from 81de79e to 881e094 Compare September 29, 2022 11:05
DDvO
DDvO approved these changes Sep 29, 2022
View reviewed changes
Copy link
Contributor

@DDvO DDvO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DDvO
Copy link
Contributor

DDvO commented Sep 29, 2022

Just found in crypto/x509/x509cset.c:

int i2d_re_X509_CRL_tbs(X509_CRL *crl, unsigned char **pp)
{
    crl->crl.enc.modified = 1;
    return i2d_X509_CRL_INFO(&crl->crl, pp);
}

that at least for some related data types the modified flag is set before calling i2d_*, apparently to be on the safe side.
Maybe such apparent workarounds are no more needed after the adaptations I added yesterday to #19271 ?

@davidben
Copy link
Contributor

davidben commented Sep 29, 2022

Should this have a unit test?

@t8m
Copy link
Member Author

t8m commented Sep 29, 2022

Should this have a unit test?

It should. However IMO this falls under much bigger effort to write the test than the actual fix. We do not have any unit test for i2d_re_* functions nor for the i2d_X509_REQ() function. I could squeeze it in x509_check_cert_pkey_test.c easily, on the other hand it is a little bit weird to piggy-back on quite unrelated test.

@DDvO DDvO removed the approval: review pending This pull request needs review by a committer label Sep 30, 2022
@no6v
Copy link
Contributor

no6v commented Sep 30, 2022

Thanks for your quick works.

Just found in crypto/x509/x509cset.c:

int i2d_re_X509_CRL_tbs(X509_CRL *crl, unsigned char **pp)
{
    crl->crl.enc.modified = 1;
    return i2d_X509_CRL_INFO(&crl->crl, pp);
}

that at least for some related data types the modified flag is set before calling i2d_*, apparently to be on the safe side.

The "re" in i2d_re_X509_tbs stands for "re-encode", and ensures that a
fresh encoding is generated in case the object has been modified after
creation (..snip..)

If, after modification, the X509 object is re-signed with X509_sign(),
the encoding is automatically renewed. Otherwise, the encoding of the
TBSCertificate portion of the X509 can be manually renewed by calling
i2d_re_X509_tbs().

My understanding from reading the above part of manpage is that setting the modified flag before calling relevant functions tells them to invalidate the internal cache, so the renewed TBS portion will be encoded.

Maybe such apparent workarounds are no more needed after the adaptations I added yesterday to #19271 ?

If your work resolves this situation, that will be great, thanks.

@davidben
Copy link
Contributor

davidben commented Sep 30, 2022

More reason to write a test, since it seems 8e39049 regressed more than just the return code! :-) (It used to be in the right order, but then 8e39049 broke it.)

@t8m
Copy link
Member Author

t8m commented Oct 1, 2022

More reason to write a test, since it seems 8e39049 regressed more than just the return code! :-) (It used to be in the right order, but then 8e39049 broke it.)

Feel free to write one.

@no6v
Copy link
Contributor

no6v commented Oct 4, 2022

de54030 looks good to me, thanks!

@t8m
Copy link
Member Author

t8m commented Oct 4, 2022

@DDvO still OK?
ping @openssl/otc for otc review.

@hlandau hlandau added approval: done This pull request has the required number of approvals and removed approval: otc review pending labels Oct 4, 2022
@DDvO
Copy link
Contributor

DDvO commented Oct 4, 2022

@DDvO still OK?

Yep

@openssl-machine
Copy link
Collaborator

openssl-machine commented Oct 5, 2022

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.

@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 5, 2022
@t8m
Copy link
Member Author

t8m commented Oct 5, 2022

Merged to master and 3.0 branches. Thank you for the reviews.

@t8m t8m closed this Oct 5, 2022
openssl-machine pushed a commit that referenced this pull request Oct 5, 2022
@t8m
Fix regression in i2d_re_X509_REQ_tbs()
928f15e 
This fixes regression from commit 8e39049. There is also no point
in setting the modified flag after just calling i2d.

Fixes #19297

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from #19299)
openssl-machine pushed a commit that referenced this pull request Oct 5, 2022
@t8m
Fix regression in i2d_re_X509_REQ_tbs()
6b9d582 
This fixes regression from commit 8e39049. There is also no point
in setting the modified flag after just calling i2d.

Fixes #19297

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from #19299)

(cherry picked from commit 928f15e)
@openssl openssl deleted a comment Oct 8, 2022
This was referenced Oct 11, 2022
Closed
Closed
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@t8m @beldmit
Fix regression in i2d_re_X509_REQ_tbs()
feaa39b 
This fixes regression from commit 8e39049. There is also no point
in setting the modified flag after just calling i2d.

Fixes openssl#19297

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from openssl#19299)
kafei-cy added a commit to kafei-cy/Tongsuo that referenced this pull request Jan 11, 2026
@kafei-cy
Fix regression in i2d_re_X509_REQ_tbs()
5f85cd1 
This fixes regression from commit 8e39049. There is also no point
in setting the modified flag after just calling i2d.

Fixes openssl/openssl#19297

(Merged from openssl/openssl#19299)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DDvO DDvO DDvO approved these changes

+1 more reviewer

@hlandau hlandau hlandau approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch severity: regression The issue/pr is a regression from previous released version triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

return value of i2d_re_X509_REQ_tbs()

6 participants

@t8m @DDvO @davidben @no6v @openssl-machine @hlandau