solutions

[Nikos Mavrogiannopoulos]

Simon Josefsson wrote:
> Having had some time to read the code, here are some observations:
> 
> * The old _gnutls_x509_oid_data2string in lib/x509/common.c is buggy
>   since it truncates the string after a NUL (it copies the string using
>   strcpy instead of memcpy).  The RES_SIZE length output variable is
>   correct though, but output data beyond the NUL will be garbage.
> 
> * I can see a few ways to solve the problem:
> 
>   1) Make _gnutls_x509_oid_data2string escape NULs as \00 following RFC
>      2253.
> 
>   2) Use memcpy instead of strcpy and change the documentation of the
>      function to say that the returned string may contain embedded NULs,
>      and fix the callers of that function.
> 
>   3) Return a RFC 2253 #-style string for these strings.
> 
>   4) Return an error when a NUL is encountered.

4 is just ok. One could do 1 if he really bothers, but there is no
reason for that. Even though IA5 string allows for null character there
is no reason for us to allow it. It is the null terminator for C strings
thus allowing it can cause only problems and complicated code.

> * Solution 2) could work but require that we investigate and possibly
>   fix all callers.  The function is used in three places:
> 
>     A) By parse_attribute in lib/x509/crq.c which is used by two other
>        functions:
> 
>       A.1.) gnutls_x509_crq_get_challenge_password which is OK as per above.
> 
>       A.2) gnutls_x509_crq_get_attribute_by_oid.  That function calls
>       parse_attribute with RAW=1 which means that
>       _gnutls_x509_oid_data2string is never called.
> 
>     B) By _gnutls_x509_parse_dn in lib/x509/dn.c.  That function is
>        documented to return a LDAP string.  It performs RFC 2253
>        escaping by calling str_escape, which doesn't handle embedded
>        NULs.
> 
>     C) By _gnutls_x509_parse_dn_oid in lib/x509/dn.c.  That function is
>        also documented to return a LDAP string, but does not perform RFC
>        2253 escaping so it is arguable broken.
[...]
> I believe 2 is actually the right solution but it may not be worth it,
> especially considering that there are not likely to be any non-malicious
> uses of NUL in a DNs.

It looks a lot of effort to me and I also do not think it can weight the
gain. I see no reason to allow nulls in a string that has the purpose to
be shown and/or typed.

regards,
Nikos