Fix ca crash (attempt 2) #5444
Fix ca crash (attempt 2) #5444
Conversation
|
Looks good, but I have a question/suggestion. Since an empty Subject DN is semantically "unspecified", as opposed to actually having an empty name, perhaps the "unique_subject" code should allow multiple empty values, and still be able to enforce uniqueness otherwise? That said, I personally tend to find "unique_subject" a nuisance, and turn it off, so I am not sure why people want "unique_subject". Perhaps those people would not want the multiple unspecified names? Finally how are such names recorded in the index file? As "/", or as an empty string? The latter might be better if not already the case. |
|
|
|
Yes, perhaps an unfortunate default, but what do you think about empty (unspecified) subject DNs vs. |
The ca man page actually recommends that you should change the default. I have created issue #5451 so we don't forget it when we get to 1.2.0.
I kind of think that behaviour might be quite surprising if you were expecting enforcement of no duplicate Subjects. |
|
Yeah, perhaps some might be surprised by multiple empty subjects, but they should quickly realize that these are not in fact "duplicates", as the subject name is unspecified, rather than being the unique subject whose name is empty. It's a judgement call, I am leaving it to the others to make the final decision. Personally, I would allow empty "duplicates" while enforcing uniqueness otherwise (when enabled/not disabled). |
|
@levitte, you have approved this - are you happy for me to continue to merge this in light of @vdukhovni's comments? |
|
Hmmmm... I'm thinking that allowing empty subjects is something new anyway, so we really should make an effort to treat them right from the start, even if that diverges from how we treat other subjects. |
Once the database has been created the index.txt.attr file gets created, populated with the value of unique_subject. From that point on it is impossible to know whether it was set explicitly or by default. |
|
Put differently, let's make "no" the default unique_subject for empty subjects now. |
|
In that case, I suggest unique_empty_subject with its own default. Or ignore unique_subject for empty subjects. |
|
Another way to justify "non-unique" empty subjects is that these are quite likely to actually be certificates for distinct subjects ("principals"), that are identified by the subjectAltName. It is just the the subjectDN is no longer adequate to distinguish the different principals to which the certificates are issued. So if Richard agrees, I think we have enough support for this reasonable interpretation, I don't think it requires a team vote, just more evidence of support than my solitary perspective. |
Ok. I can live with that. |
|
Having empty subjects with unique_empty_subject set to "yes" turns out to be a hard problem to solve. The problem is that we use libcrypto to manage the database. As part of that, if we are going to have unique empty subjects, then we create an index of those subjects. Under pinning the database is a hash, which needs to have a comparison function for elements in the hash. To avoid two different database rows coming back as the same thing if they both have empty subjects, then the comparison function must report them as different. That is easy enough to do. However this then causes the index to fail, because when you add a new entry to the index it immediately sanity checks it by trying to retrieve it again - which fails because the comparison function can't find the thing it just added. We can't really make changes in the database code itself because this is all in libcrypto and exposed via public API. We could just not create an index for the subject at all in the ca app. That would mean that every time we add an entry to the database we would have to scan each row in the database manually, checking to see if we have a duplicate. However that starts sounding like major heart surgery in a stable branch, which I'm a little reluctant to do. While researching all of this I discovered a 16-year old bug introduced by commit 87e8fec. I suspect (but haven't checked) that this is actually when the crashing bug which started this saga off was also first introduced. It turns out that we have code in CA which is supposed to complain with a nice user friendly message if you attempt to add a duplicate entry into the database. It is supposed to tell you details of the clashing entry in the database. This doesn't work and you're just told that the operation failed with database error 2. Fixing that bug, also fixes the crashing bug co-incidentally! So, this means I'm going to remove the commit in this PR that actually adds the fix, and use this alternative fix instead. |
480eb40 to
c4ba9c4
Compare
|
I've updated this PR with the new fix which also fixes the bug I described above. I've not done the unique subjects except when empty thing because of the problems also described above. |
|
I'll hold off updating the 1.0.2 version of this until we get some agreement that this is the way ahead. |
|
Given the hash table, if we were to allow non-unique subject names just for certs with an empty subject DN, the way to do that is to use the serial number as the hash key when the subject DN is empty. The serial number really does need to be unique. If that still too hard, I can live with the limitation that CAs that create empty subject DNs must set "unique_subject = no" (something that is generally sane anyway), but I if we can avoid the limitation with reasonable effort, then I'd prefer that. The above basically means that empty subject DNs get implicitly mapped to the serial number for indexing, comparing, sorting, ... Note that just arbitrarily comparing empty as non-equal can make some qsort implementations crash, since this can result in non-transitivity of the comparison function. Don;'t know whether that's an issue here... |
Hmmm. That's a good idea. I will investigate if that is viable. |
|
It works! Pushed a new commit to do that. I think this is going to need some documentation somewhere. I've not added that yet. Waiting until we're all in agreement to this as an approach. |
|
I'm a little lost... it sounds like |
apps/ca.c
Outdated
There was a problem hiding this comment.
Do you see a reason why we do this copy? I don't, so I think this is just a historical remnant... you might as well remove it and use row directly below.
No, the 16 year old bug I found is with "unique_subject = yes". If you attempt to add a duplicate subject you are supposed to get a nice error message. Instead you get "database error 2" (or something like that). |
|
You misunderstand... For empty subjects, you copy I'll follow up with an in diff comment... |
apps/ca.c
Outdated
There was a problem hiding this comment.
The block above is an elaborate way to assume unique_subject = no at all times, and I wonder if it wouldn't be simpler to just...
There was a problem hiding this comment.
The block above is an elaborate way to assume unique_subject = no at all times
No? This is to explicitly handle the unique_subject = yes case to make sure that we really do have a unique subject, by filling in the Subject with a serial number iff the Subject is empty.
apps/ca.c
Outdated
There was a problem hiding this comment.
... change this condition to (row[DB_name][0] != '\0' && db->attributes.unique_subject)
There was a problem hiding this comment.
This doesn't solve the problem. The call to TXT_DB_insert() will fail later if we don't do the above block due to the existence of the index.
|
I guess that in the end, I'm asking what would happen if we simply ignored |
No, the problem only occurs with unique_subject = yes, because in that case we create an index. If unique_subject = no then we do not create an index so there is no problem. Our issue was we wanted somewhere in the middle - a partial index - where most entries were unique, but some were not. Viktor's workaround of using serial number for those entries with an empty Subject works around that problem though. |
Ah, right, yeah I see the problem then. Now bear with me... what do you think would happen if we did a more radical change, such as throwing away the index on |
|
The problem is that users who rely on creation of duplicate certificates (by DN) to fail, will no longer have that happen. Why they would want this is less clear, but perhaps some do. I think that this PR should just address the issue, and removing "unique_subject" support could be a separate change? |
|
Good point. Sorry for derailing |
|
What's the status of this? |
This reverts commit 1e05c6d. Empty subjects should be permissible.
This reverts commit e505f1e. Empty Subjects should be permissible.
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
I ran into some problems with this PR. Needs some attention, but is currently on hold while I fight some bigger fires. I'll come back to it. |
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.
7123eba to
67ee0a7
Compare
|
Grrr....it no longer cherry-picks to 1.1.0. See #5627. |
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 <levitte@openssl.org> (Merged from #5444)
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 <levitte@openssl.org> (Merged from #5444)
Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #5444)
|
Pushed. Thanks! |
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
This PR is for master/1.1.0. A separate PR will fix 1.0.2.