solutions

[Simon Josefsson]

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.

* Solution 1) and 3) appear to break
  gnutls_x509_crq_get_challenge_password which is used to extract the
  certificate request challenge password.  Password strings are not DNs
  and should not be escaped as per RFC 2253.  If any of the escaped
  characters in RFC 2253 is used in a password (e.g., plus or comma),
  and if _gnutls_x509_oid_data2string escapes them, things will break.
  Further, the gnutls_x509_crq_get_challenge_password API does not use
  zero-terminated strings, so it can handle embedded NULs in password.

  The same problem could apply to DN SAN's and CRL distribution points,
  they are not documented in GnuTLS to be in LDAP escape format
  (although not documented to be in any other format either).

  However, if we really want to use 1) or 3) as the solution, the
  gnutls_x509_crq_get_challenge_password function could be modified to
  use some other code path to yield correct results.

* 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.

  Both _gnutls_x509_parse_dn and _gnutls_x509_parse_dn_oid are used in
  many places in GnuTLS.  Most of them are from APIs that actually are
  documented to return LDAP escaped strings (yay!) so we are relatively
  safe.  However there is one usages that could be problematic:

    A) _gnutls_parse_general_name uses _gnutls_x509_parse_dn to extract
    a DirectoryName type of GeneralName.  The _gnutls_parse_general_name
    function is also by gnutls_x509_crt_get_crl_dist_points to get the
    CRL distribution point.

  I suspect DN SAN's and use of CRL dist points are quite rare.  Anyone
  with more insight?

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.

So while I prefer 2) since it allows GnuTLS to handle embedded NULs, I
suspect 4) will result in more robust overall design.  The patch could
be as simple as the one below, but I need to do more testing.

/Simon

--- common.c~	2009-06-02 20:59:32.000000000 +0200
+++ common.c	2009-08-05 13:18:17.000000000 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008 Free Software Foundation
+ * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009 Free Software Foundation
  *
  * Author: Nikos Mavrogiannopoulos
  *
@@ -242,6 +242,9 @@
     {
       str[len] = 0;
 
+      if (strlen (str) != len)
+	return GNUTLS_E_ASN1_DER_ERROR;
+
       if (res)
 	_gnutls_str_cpy (res, *res_size, str);
       *res_size = len;
@@ -296,6 +299,8 @@
 	  if (non_printable == 0)
 	    {
 	      str[len] = 0;
+	      if (strlen (str) != len)
+		return GNUTLS_E_ASN1_DER_ERROR;
 	      _gnutls_str_cpy (res, *res_size, str);
 	      *res_size = len;
 	    }