Clear the extension list when removing the last extension by davidben · Pull Request #28398 · openssl/openssl

davidben · 2025-08-31T21:57:03Z

The extensions list in a certificate, CRL, and CRL entry is defined as:

... extensions      [3]  EXPLICIT Extensions OPTIONAL ...
... crlEntryExtensions      Extensions OPTIONAL ...
... crlExtensions           [0]  EXPLICIT Extensions OPTIONAL ...

Extensions  ::=  SEQUENCE SIZE (1..MAX) OF Extension

This means that a present but empty extensions list is actually invalid. Rather, if you have no extensions to encode, you are meant to omit the list altogether. Fix the delete_ext functions to handle this correctly.

This would mostly be moot, as an application adding extensions only to delete them all would be unusual. However, #13658 implemented a slightly roundabout design where, to omit SKID/AKID, the library first puts them in and then the command-line tool detects some placeholder values and deletes the extensino again.

Fixes #28397

Checklist

tests are added or updated

paulidale

Just some formatting nits. Approved once these are addressed.

Is it worthwhile factoring the repeated code into a function?
Or even defining a new delete function that does the additional work?

crypto/x509/x509_ext.c

paulidale · 2025-08-31T22:05:19Z

Happy for this to be back–ported to any relevant branches too.

The extensions list in a certificate, CRL, and CRL entry is defined as: ... extensions [3] EXPLICIT Extensions OPTIONAL ... ... crlEntryExtensions Extensions OPTIONAL ... ... crlExtensions [0] EXPLICIT Extensions OPTIONAL ... Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension This means that a present but empty extensions list is actually invalid. Rather, if you have no extensions to encode, you are meant to omit the list altogether. Fix the delete_ext functions to handle this correctly. This would mostly be moot, as an application adding extensions only to delete them all would be unusual. However, openssl#13658 implemented a slightly roundabout design where, to omit SKID/AKID, the library first puts them in and then the command-line tool detects some placeholder values and deletes the extension again. Fixes openssl#28397

davidben · 2025-08-31T22:11:19Z

(Alright, hopefully that version works? Embarrassing type errors. 😳 In my defense, it looks like the default macOS build doesn't check them, which is a little odd.)

Is it worthwhile factoring the repeated code into a function?

Ah sure, done.

Or even defining a new delete function that does the additional work?

You mean whether to export the helper publicly? I figure that can probably be left to when/if someone needs it, but no strong feelings either way. 🤷

paulidale

Much neater, thanks.

Yes, I did mean exporting it but to the rest of libcrypto rather than publicly.
I agree, that can happen when someone needs it.

t-j-h

Nice fix. Thanks.

Ok for backfit.

The extensions list in a certificate, CRL, and CRL entry is defined as: ... extensions [3] EXPLICIT Extensions OPTIONAL ... ... crlEntryExtensions Extensions OPTIONAL ... ... crlExtensions [0] EXPLICIT Extensions OPTIONAL ... Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension This means that a present but empty extensions list is actually invalid. Rather, if you have no extensions to encode, you are meant to omit the list altogether. Fix the delete_ext functions to handle this correctly. This would mostly be moot, as an application adding extensions only to delete them all would be unusual. However, #13658 implemented a slightly roundabout design where, to omit SKID/AKID, the library first puts them in and then the command-line tool detects some placeholder values and deletes the extension again. Fixes #28397 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28398) (cherry picked from commit 9a8d7dc)

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28398) (cherry picked from commit 9e8898b)

t8m · 2025-09-09T09:17:48Z

Merged to all the active branches. Thank you for your contribution.

The extensions list in a certificate, CRL, and CRL entry is defined as: ... extensions [3] EXPLICIT Extensions OPTIONAL ... ... crlEntryExtensions Extensions OPTIONAL ... ... crlExtensions [0] EXPLICIT Extensions OPTIONAL ... Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension This means that a present but empty extensions list is actually invalid. Rather, if you have no extensions to encode, you are meant to omit the list altogether. Fix the delete_ext functions to handle this correctly. This would mostly be moot, as an application adding extensions only to delete them all would be unusual. However, #13658 implemented a slightly roundabout design where, to omit SKID/AKID, the library first puts them in and then the command-line tool detects some placeholder values and deletes the extension again. Fixes #28397 Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28398) (cherry picked from commit 9a8d7dc)

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tim Hudson <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28398) (cherry picked from commit 9e8898b)

CHANGES.md: * openssl#28398 * openssl#28411 * openssl#28447 * openssl#28449 NEWS.md: * openssl#28447 Release: yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

CHANGES.md: * #28398 * #28411 * #28447 * #28449 NEWS.md: * #28447 Release: yes Signed-off-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #28521)

CHANGES.md: * openssl#28398 * openssl#28411 * openssl#28447 * openssl#28449 NEWS.md: * openssl#28447 Release: yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

CHANGES.md: * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28447 * openssl#28449 NEWS.md: * openssl#28447 Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

CHANGES.md: * #28198 * #28398 * #28411 * #28447 * #28449 NEWS.md: * #28447 Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #28558)

CHANGES.md: * #28398 * #28411 * #28447 * #28449 NEWS.md: * #28447 Release: yes Signed-off-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28547)

3.0.18 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28624 Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

3.2.6 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.2.6 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

3.3.5 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.3.5 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

3.4.3 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28415 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.4.3 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

3.3.5 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.3.5 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

3.4.3 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28415 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.4.3 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

3.2.6 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.2.6 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

3.0.18 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28624 Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]>

3.4.3 CHANGES.md includes the following: * #28198 * #28398 * #28411 * #28415 * #28449 Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]>

3.3.5 CHANGES.md includes the following: * #28198 * #28398 * #28411 * #28449 Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]>

3.2.6 CHANGES.md includes the following: * #28198 * #28398 * #28411 * #28449 Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]>

3.0.18 CHANGES.md includes the following: * #28198 * #28398 * #28411 * #28449 Release: Yes Signed-off-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]>

paulidale previously approved these changes Aug 31, 2025

View reviewed changes

crypto/x509/x509_ext.c Show resolved Hide resolved

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

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

davidben dismissed paulidale’s stale review via 6a94c58 August 31, 2025 22:04

davidben force-pushed the delete-ext branch from 5120e72 to 6a94c58 Compare August 31, 2025 22:04

paulidale added branch: master Applies to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug tests: present The PR has suitable tests present labels Aug 31, 2025

davidben force-pushed the delete-ext branch from 6a94c58 to cb9a4eb Compare August 31, 2025 22:06

paulidale previously approved these changes Aug 31, 2025

View reviewed changes

Add a helper function to delete the extension list

e305a92

davidben dismissed paulidale’s stale review via e305a92 August 31, 2025 22:10

paulidale approved these changes Aug 31, 2025

View reviewed changes

t-j-h approved these changes Aug 31, 2025

View reviewed changes

t-j-h added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Aug 31, 2025

mattcaswell approved these changes Sep 1, 2025

View reviewed changes

t8m added branch: 3.0 Applies to openssl-3.0 branch branch: 3.2 Applies to openssl-3.2 (EOL) branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 labels Sep 1, 2025

t8m closed this Sep 1, 2025

t8m reopened this Sep 1, 2025

t8m removed the branch: 3.0 Applies to openssl-3.0 branch label Sep 1, 2025

t8m approved these changes Sep 1, 2025

View reviewed changes

openssl-machine 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 Sep 1, 2025

t8m closed this Sep 9, 2025

esyr mentioned this pull request Sep 11, 2025

[3.6] CHANGES.md, NEWS.md: update for 3.6.0-beta1 #28521

Closed

esyr mentioned this pull request Sep 15, 2025

[3.5] CHANGES.md, NEWS.md: update for 3.5.3 #28558

Closed

esyr mentioned this pull request Sep 30, 2025

[3.0] CHANGES.md: update for 3.0.18 #28697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Clear the extension list when removing the last extension#28398

Clear the extension list when removing the last extension#28398
davidben wants to merge 2 commits intoopenssl:masterfrom
davidben:delete-ext

davidben commented Aug 31, 2025

Uh oh!

paulidale left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulidale commented Aug 31, 2025

Uh oh!

davidben commented Aug 31, 2025

Uh oh!

paulidale left a comment •

edited

Loading

Uh oh!

t-j-h left a comment

Uh oh!

t8m commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Comments

Conversation

davidben commented Aug 31, 2025

Checklist

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulidale commented Aug 31, 2025

Uh oh!

davidben commented Aug 31, 2025

Uh oh!

paulidale left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-j-h left a comment

Choose a reason for hiding this comment

Uh oh!

t8m commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

paulidale left a comment •

edited

Loading