>From: owner-openssl-us...@openssl.org On Behalf Of Sanjay Kumar (sanjaku5)
>Sent: Friday, 28 June, 2013 06:58
>I have a requirement to get unique certificate for each user.
>To achieve that I am modifying the CN field of CERT subject name
>by appending the user index to CN field.
>Eg. If CN=sanjay [with] userIndex 1 [to] CN=sanjay000001
Is that required or a choice? There are other DN (subject) fields
that could be used without mangling CN. SerialNumber (2.5.4 .5)
is the most obvious, but some others could be bent to fit.
If any of your certs are (intended to be) for HTTPS servers,
and probably any other SSL servers, changing CN that way
will make them unacceptable to any standard-compliant peer(s).
>I have the below code to achieve the above requirement.
>But I am memory dump in below line:
>if (!X509_NAME_add_entry(target_sub_name, target_entry, -1, 0))
*In* that line is impossible; in the routine it directly calls
(X509_NAME_add_entry) unlikely. Some *other* routines called
from there more possibly, particularly if it needs to allocate
and your heap has been corrupted. Do you have or can you get a
stacktrace? Did/can you run with valgrind or similar?
>Seems it this doesn't allow to increment the length of CN field
>(look like array overflow).
You can certainly use a new and longer value as you are doing.
If you exceed the compiled-in ub-xx limit for a field, which your
code won't given the correct snprintf limit, openssl gives error
return (null with error-queue set) but does not core.
Your code with a trivial wrapper and simplified output works
for me for the apparently intended case. But this code displays
enough misunderstandings that your other code could well have
a bug (or several) that causes failure here. Basically you're
failing to handle quite a few problems that could happen,
while wasting code on things that are impossible:
>int Certificateclass::generate_cert(X509 *x509, uint32_t user_id,
>uint8_t **user_cert, EVP_PKEY *cakey, uint32_t usr_cert_len)
>{
> unsigned char *ptr = NULL, *temp = NULL,
target_cn_value[EAY_MAX_CN_LEN] = {'\0'};
Even assuming that constant is 64, most DN fields including CN can be
BMPString or UniversalString which are 2x or 4x longer in bytes --
unless your CA prevents them (maybe even change from the CSR).
Even if you don't care about i18n, you should at least give an error
rather than crash or wrong output. And to treat singlebyte as a C string
as your code does in general you need one more byte for terminator.
> int len = 0, nid = 0;
> uint8_t entry_count = 0, i = 0;
It's unlikely for DN to have >255 T&V but not impossible and
there's no good reason to fail catastrophically if it does.
> char sub_name_str[EAY_MAX_CN_LEN] = {'\0'}; /*used for logging
purpose*/
First that isn't necessarily large enough even for CN as above, plus
DN can and usually does contain other fields, and xn_print_(,,,oneline)
adds more e.g. "/CN=". But you probably don't need at all, see below.
> X509_NAME *base_sub_name = NULL, *target_sub_name = NULL;
> X509_NAME_ENTRY *entry = NULL, *target_entry = NULL;
> ASN1_OBJECT *entry_obj = NULL;
> ASN1_STRING *entry_string = NULL;
> char *dataStart = NULL;
> long nameLength = 0;
> BIO *subjectBio = BIO_new(BIO_s_mem());
> char temp_cn[EAY_MAX_CN_LEN]= {'\0'};
As above.
> base_sub_name = X509_get_subject_name(x509);
> entry_count = X509_NAME_entry_count(base_sub_name);
> target_sub_name = X509_NAME_new();
> X509_NAME_print_ex(subjectBio, base_sub_name, 0, XN_FLAG_ONELINE);
> nameLength = BIO_get_mem_data(subjectBio, &dataStart);
> memcpy(sub_name_str, dataStart, nameLength);
> sub_name_str[nameLength] = '\0';
As above to allocate safely you need to use the dynamic/computed size
from the BIO, which can be quite a bit more than 64. But you probably
don't need to copy and terminate, and thus allocate, at all, see below.
>for (i = 0; i < entry_count; i++)
> {
> entry = X509_NAME_get_entry(base_sub_name, i);
> /*Get all element from cert sub name*/
> if (entry)
> {
> entry_obj = X509_NAME_ENTRY_get_object(entry);
> if (entry_obj)
> {
Those if's are useless; xn_get_ (via sk_xne_value) can't fail for
argument in range, and xne_get_object from decoded can't fail.
> nid = OBJ_obj2nid(entry_obj);
> if (NID_commonName == nid)
> {
> /* if entry NID is CN then append user index,
> else simply add to target_sub_name */
> if( NULL == commonName)
> {
> if(NULL != sub_name_str){
> LOG_EVENT (LOG_LEVEL_INFO, FACILITY_IKEV2,
> "Certificate subject name received:%s",
sub_name_str);
> }
If that's printf-like, instead of copying -- with the allocation issue
noted above -- just use %.*s with (int)nameLength and dataStart.
> commonName = (uint8_t *)calloc(1, EAY_MAX_CN_LEN);
> X509_NAME_get_text_by_NID(base_sub_name, nid, (char
*)commonName, EAY_MAX_CN_LEN);
> }
There's no need to search for the entry (as get_by_NID does) when you
have it, and there may be no need to copy and terminate, see below.
If you do, you need 64+1 for singlebyte, more for BMP or Universal.
commonName is not local and so must be global or at least class;
if it is not reset between calls to this routine for different users,
you get user1's name in user2's cert. Is that a feature?
{c,m,re}alloc can return null, which you should check before using
-- unless you eliminate this allocation entirely per below, or
substitute C++ new (and delete) which throws an exception.
> {
> /*Modifying the certificate subject name */
> memcpy(temp_cn, commonName, strlen((char
*)commonName));
> snprintf((char *)target_cn_value, EAY_MAX_CN_LEN,
"%s%06d", temp_cn, user_id);
> }
Unless you need (original) commonName elsewhere, or add handling
of multibyte, you can directly snprintf %.*s using the length
and address from xne_get_data(entry) (or just entry->value,
but if you've stayed out of internals so far keep that up).
If the original CN is longer than 58, the added number is
truncated or eliminated. You might want to handle that case.
> /*adding the new subject to target Enrty*/
No, adding the modified (name_)entry to the new subject.
> target_entry = X509_NAME_ENTRY_create_by_NID
> (NULL, nid, MBSTRING_ASC, target_cn_value, -1);
> if(NULL == target_entry) <snip>
> if (!X509_NAME_add_entry(target_sub_name, target_entry,
-1, 0)) <snip>
FWIW xn_add_entry_by_NID can do both xne_create_by_NID plus xn_add_entry .
> }
> else
> {
> if (!X509_NAME_add_entry(target_sub_name, entry, -1,
0)) <snip>
> }
> }
> }
> }
Your logic will mangle any multi-valued RDN (i.e. SET size > 1) but
those are pretty rare -- and nonexistent if your CA prevents them.
> X509_set_subject_name(x509, target_sub_name);
> BIO_free(subjectBio);
> subjectBio = BIO_new(BIO_s_mem());
> X509_NAME_print_ex(subjectBio, target_sub_name, 0, XN_FLAG_ONELINE);
> nameLength = BIO_get_mem_data(subjectBio, &dataStart);
> memcpy(sub_name_str, dataStart, nameLength);
> sub_name_str[nameLength] = '\0';
> if(NULL != sub_name_str)
> {
> LOG_EVENT (LOG_LEVEL_INFO, FACILITY_IKEV2, "Certificate
> subject name updated to:%s for user_id:%d", sub_name_str,
user_id);
> }
As above. Plus the if is useless; sub_name_str can never be null.
If you checked for its contents being empty (a "null string")
that's different and sometimes useful but not really here.
> X509_NAME_free(target_sub_name);
> BIO_free(subjectBio);
> if (!(X509_sign(x509, cakey, EVP_sha1()))) <snip>
I hope the caller is very sure the supplied cakey is the same as
used to issue the input cert. If you re-sign with a different key
without changing issuer, and AKI if applicable, to match, you will
cause chaos to the point your users rightly won't trust you at all.
Personally I would probably verify the original cert (before any
modification) against the public-half of the key to be safe.
> *user_cert= (unsigned char *) calloc(1, usr_cert_len + 1);
> ptr = *user_cert;
> temp = ptr;
> i2d_X509(x509,(unsigned char **)&ptr);
I hope the caller supplies usr_cert_len correctly, otherwise
this clobbers memory. It's safer to use i2d_(,NULL) to compute
the length and use that -- and you don't need +1 for a null
because DER is not and must never be treated as a C string.
You don't need the cast, ptr is already unsigned char*.
If it was actually a wrong type of pointer, casting it might
NOT work in general -- C++ and C do NOT require all pointer
types to use the same representation -- but I think it
probably works on all platforms where openssl works.
> len = (ptr - temp);
> return len;
>}
And I think you have memory leaks, but I didn't test. If this
is in a program that just runs to do one cert or a few and
then stop, that's tolerable. If it needs to keep running and
serve a large number of requests, you need to fix leaks --
but without creating any other bugs. valgrind, or at least
a debugging allocator, can be helpful.
______________________________________________________________________
OpenSSL Project http://www.openssl.org
User Support Mailing List openssl-users@openssl.org
Automated List Manager majord...@openssl.org
