solutions

[Simon Josefsson]

Simon Josefsson <simon at josefsson.org> writes:

> Nikos Mavrogiannopoulos <nmav at gnutls.org> writes:
>
>> diff --git a/lib/x509/common.c b/lib/x509/common.c
>> index 51da7b1..71a4114 100644
>> --- a/lib/x509/common.c
>> +++ b/lib/x509/common.c
>> @@ -181,7 +181,7 @@ _gnutls_x509_oid_data2string (const char *oid, void *value,
>>  {
>>    char str[MAX_STRING_LEN], tmpname[128];
>>    const char *ANAME = NULL;
>> -  int CHOICE = -1, len = -1, result;
>> +  int CHOICE = -1, len = -1, result, i;
>>    ASN1_TYPE tmpasn = ASN1_TYPE_EMPTY;
>>    char asn1_err[ASN1_MAX_ERROR_DESCRIPTION_SIZE] = "";
>>  
>> @@ -309,6 +309,12 @@ _gnutls_x509_oid_data2string (const char *oid, void *value,
>>  	    }
>>  	}
>>      }
>> +  
>> +  /* Convert null char in the name to '?'
>> +   * to protect applications */
>> +  for (i=0;i<*res_size;i++) {
>> +      if (res[i] == 0) res[i]='?';
>> +  }
>>  
>>    return 0;
>>  }
>
> Hi Nikos -- this code crashed the self-tests, but I fixed that.
>
> However, isn't this the wrong way to address the real problem?  It seems
> callers of the function should be fixed to be careful not to assume
> decoded data does not contain NULs?

Indeed the code doesn't seem to be right, since it uses _gnutls_str_cpy
which uses strlen earlier to get the length of the string.  So instead
of truncating on the \0 it will add a ? and truncate after the \0.

Fixing the callers should be the right solution.  Let's hope our API
isn't broken and uses zero terminated strings for these strings...

/Simon