Fix ca crash (attempt 2) 1.0.2 #5445
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
levitte
approved these changes
Feb 23, 2018
richsalz
added
the
approval: done
Commit 87e8fec (16 years ago!) introduced a bug where if we are attempting to insert a cert with a duplicate subject name, and duplicate subject names are not allowed (which is the default), then we get an unhelpful error message back (error number 2). Prior to that commit we got a helpful error message which displayed details of the conflicting entry in the database. That commit was itself attempting to fix a bug with the noemailDN option where we were setting the subject field in the database too early (before extensions had made any amendments to it). This PR moves the check for a conflicting Subject name until after all changes to the Subject have been made by extensions etc. This also, co-incidentally Fixes the ca crashing bug described in issue 5109. Fixes openssl#5109
It is quite likely for there to be multiple certificates with empty subjects, which are still distinct because of subjectAltName. Therefore we allow multiple certificates with an empty Subject even if unique_subject is set to yes.
mattcaswell
force-pushed
the
fix-ca-crash2-102
branch
from
38b2dea to
bf84f42
Compare
levitte
approved these changes
Mar 15, 2018
levitte
pushed a commit
that referenced
this pull request
Mar 15, 2018
This reverts commit dd37f6f. Empty Subjects are permissible. Reviewed-by: Richard Levitte <[email protected]> (Merged from #5445)
levitte
pushed a commit
that referenced
this pull request
Mar 15, 2018
This reverts commit a3d684f. Empty Subjects are permissible Reviewed-by: Richard Levitte <[email protected]> (Merged from #5445)
levitte
pushed a commit
that referenced
this pull request
Mar 15, 2018
Commit 87e8fec (16 years ago!) introduced a bug where if we are attempting to insert a cert with a duplicate subject name, and duplicate subject names are not allowed (which is the default), then we get an unhelpful error message back (error number 2). Prior to that commit we got a helpful error message which displayed details of the conflicting entry in the database. That commit was itself attempting to fix a bug with the noemailDN option where we were setting the subject field in the database too early (before extensions had made any amendments to it). This PR moves the check for a conflicting Subject name until after all changes to the Subject have been made by extensions etc. This also, co-incidentally Fixes the ca crashing bug described in issue 5109. Fixes #5109 Reviewed-by: Richard Levitte <[email protected]> (Merged from #5445)
levitte
pushed a commit
that referenced
this pull request
Mar 15, 2018
It is quite likely for there to be multiple certificates with empty subjects, which are still distinct because of subjectAltName. Therefore we allow multiple certificates with an empty Subject even if unique_subject is set to yes. Reviewed-by: Richard Levitte <[email protected]> (Merged from #5445)
levitte
pushed a commit
that referenced
this pull request
Mar 15, 2018
Reviewed-by: Richard Levitte <[email protected]> (Merged from #5445)
Member
Author
|
Pushed. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(This is the 1.0.2 version of #5444)
This is another attempt at fixing the issue originally described in #5109. A fix for that was committed as a result of #5114 (#5115 for 1.0.2). Unfortunately that fix prevented empty Subjects from being used completely. As pointed out by @vdukhovni in #5115, an empty Subject is actually permissible in some situations.
This PR reverts the original commits and implements a new fix. The core issue is the name comparison function which cannot handle NULL as an input parameter. This PR updates that function to be able to correctly process that.
Note: that the default database configuration setting is to disallow multiple subjects with the same value. If two certs are issued without a subject then this counts as two with the same value. Therefore to allow this the database should be configured with the "unique_subject = no" setting (see the ca man page for details).
Fixes #5109