- OpenBSD CVS Sync

   - djm@cvs.openbsd.org 2010/03/03 01:44:36
     [auth-options.c key.c]
     reject strings with embedded ASCII nul chars in certificate key IDs,
     principal names and constraints

3 files changed, 48 insertions, 21 deletions

diff --git a/ChangeLog b/ChangeLog
index 248fdfa91..36ef5c911 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -6,6 +6,11 @@
    imorgan AT nas.nasa.gov in bz#1731
  - (djm) [.cvsignore] Ignore ssh-pkcs11-helper
  - (djm) [regress/Makefile] Cleanup sshd_proxy_orig
+ - OpenBSD CVS Sync
+   - djm@cvs.openbsd.org 2010/03/03 01:44:36
+     [auth-options.c key.c]
+     reject strings with embedded ASCII nul chars in certificate key IDs,
+     principal names and constraints
 
 20100303
  - (djm) [PROTOCOL.certkeys] Add RCS Ident
diff --git a/auth-options.c b/auth-options.c
index 396bda62f..d14624bf4 100644
--- a/auth-options.c
+++ b/auth-options.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: auth-options.c,v 1.45 2010/02/26 20:29:54 djm Exp $ */
+/* $OpenBSD: auth-options.c,v 1.46 2010/03/03 01:44:36 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -391,7 +391,7 @@ int
 auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
 {
         u_char *name = NULL, *data_blob = NULL;
-        u_int len;
+        u_int nlen, dlen, clen;
         Buffer c, data;
         int ret = -1;
 
@@ -410,14 +410,18 @@ auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
         buffer_append(&c, buffer_ptr(c_orig), buffer_len(c_orig));
 
         while (buffer_len(&c) > 0) {
-                if ((name = buffer_get_string_ret(&c, NULL)) == NULL ||
-                    (data_blob = buffer_get_string_ret(&c, &len)) == NULL) {
+                if ((name = buffer_get_string_ret(&c, &nlen)) == NULL ||
+                    (data_blob = buffer_get_string_ret(&c, &dlen)) == NULL) {
                         error("Certificate constraints corrupt");
                         goto out;
                 }
-                buffer_append(&data, data_blob, len);
+                buffer_append(&data, data_blob, dlen);
                 debug3("found certificate constraint \"%.100s\" len %u",
-                    name, len);
+                    name, dlen);
+                if (strlen(name) != nlen) {
+                        error("Certificate constraint name contains \\0");
+                        goto out;
+                }
                 if (strcmp(name, "permit-X11-forwarding") == 0)
                         cert_no_x11_forwarding_flag = 0;
                 else if (strcmp(name, "permit-agent-forwarding") == 0)
@@ -429,13 +433,17 @@ auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
                 else if (strcmp(name, "permit-user-rc") == 0)
                         cert_no_user_rc = 0;
                 else if (strcmp(name, "force-command") == 0) {
-                        char *command = buffer_get_string_ret(&data, NULL);
+                        char *command = buffer_get_string_ret(&data, &clen);
 
                         if (command == NULL) {
                                 error("Certificate constraint \"%s\" corrupt",
                                     name);
                                 goto out;
                         }
+                        if (strlen(command) != clen) {
+                                error("force-command constrain contains \\0");
+                                goto out;
+                        }
                         if (cert_forced_command != NULL) {
                                 error("Certificate has multiple "
                                     "forced-command constraints");
@@ -444,7 +452,7 @@ auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
                         }
                         cert_forced_command = command;
                 } else if (strcmp(name, "source-address") == 0) {
-                        char *allowed = buffer_get_string_ret(&data, NULL);
+                        char *allowed = buffer_get_string_ret(&data, &clen);
                         const char *remote_ip = get_remote_ipaddr();
                         
                         if (allowed == NULL) {
@@ -452,6 +460,10 @@ auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
                                     name);
                                 goto out;
                         }
+                        if (strlen(allowed) != clen) {
+                                error("source-address constrain contains \\0");
+                                goto out;
+                        }
                         if (cert_source_address_done++) {
                                 error("Certificate has multiple "
                                     "source-address constraints");
diff --git a/key.c b/key.c
index 387190b53..e6266fa58 100644
--- a/key.c
+++ b/key.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: key.c,v 1.83 2010/02/26 20:29:54 djm Exp $ */
+/* $OpenBSD: key.c,v 1.84 2010/03/03 01:44:36 djm Exp $ */
 /*
  * read_bignum():
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1000,7 +1000,7 @@ static int
 cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 {
         u_char *principals, *constraints, *sig_key, *sig;
-        u_int signed_len, plen, clen, sklen, slen;
+        u_int signed_len, plen, clen, sklen, slen, kidlen;
         Buffer tmp;
         char *principal;
         int ret = -1;
@@ -1012,7 +1012,7 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 
         principals = constraints = sig_key = sig = NULL;
         if (buffer_get_int_ret(&key->cert->type, b) != 0 ||
-            (key->cert->key_id = buffer_get_string_ret(b, NULL)) == NULL ||
+            (key->cert->key_id = buffer_get_string_ret(b, &kidlen)) == NULL ||
             (principals = buffer_get_string_ret(b, &plen)) == NULL ||
             buffer_get_int64_ret(&key->cert->valid_after, b) != 0 ||
             buffer_get_int64_ret(&key->cert->valid_before, b) != 0 ||
@@ -1024,6 +1024,11 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
                 goto out;
         }
 
+        if (kidlen != strlen(key->cert->key_id)) {
+                error("%s: key ID contains \\0 character", __func__);
+                goto out;
+        }
+
         /* Signature is left in the buffer so we can calculate this length */
         signed_len = buffer_len(&key->cert->certblob) - buffer_len(b);
 
@@ -1041,11 +1046,16 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
         buffer_append(&tmp, principals, plen);
         while (buffer_len(&tmp) > 0) {
                 if (key->cert->nprincipals >= CERT_MAX_PRINCIPALS) {
-                        error("Too many principals");
+                        error("%s: Too many principals", __func__);
                         goto out;
                 }
-                if ((principal = buffer_get_string_ret(&tmp, NULL)) == NULL) {
-                        error("Principals data invalid");
+                if ((principal = buffer_get_string_ret(&tmp, &plen)) == NULL) {
+                        error("%s: Principals data invalid", __func__);
+                        goto out;
+                }
+                if (strlen(principal) != plen) {
+                        error("%s: Principal contains \\0 character",
+                            __func__);
                         goto out;
                 }
                 key->cert->principals = xrealloc(key->cert->principals,
@@ -1061,7 +1071,7 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
         while (buffer_len(&tmp) != 0) {
                 if (buffer_get_string_ptr(&tmp, NULL) == NULL ||
                     buffer_get_string_ptr(&tmp, NULL) == NULL) {
-                        error("Constraints data invalid");
+                        error("%s: Constraints data invalid", __func__);
                         goto out;
                 }
         }
@@ -1069,12 +1079,12 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 
         if ((key->cert->signature_key = key_from_blob(sig_key,
             sklen)) == NULL) {
-                error("Signature key invalid");
+                error("%s: Signature key invalid", __func__);
                 goto out;
         }
         if (key->cert->signature_key->type != KEY_RSA &&
             key->cert->signature_key->type != KEY_DSA) {
-                error("Invalid signature key type %s (%d)",
+                error("%s: Invalid signature key type %s (%d)", __func__,
                     key_type(key->cert->signature_key),
                     key->cert->signature_key->type);
                 goto out;
@@ -1083,17 +1093,17 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
         switch (key_verify(key->cert->signature_key, sig, slen, 
             buffer_ptr(&key->cert->certblob), signed_len)) {
         case 1:
+                ret = 0;
                 break; /* Good signature */
         case 0:
-                error("Invalid signature on certificate");
+                error("%s: Invalid signature on certificate", __func__);
                 goto out;
         case -1:
-                error("Certificate signature verification failed");
+                error("%s: Certificate signature verification failed",
+                    __func__);
                 goto out;
         }
 
-        ret = 0;
-
  out:
         buffer_free(&tmp);
         if (principals != NULL)