Save encoded KRB-SAFE-BODY to avoid problems caused by re-encoding it.

Also, handle correctly implemented RFC 1510 KRB-SAFE i.e., checksummed
over KRB-SAFE-BODY only.

ticket: 1893
tags: pullup

git-svn-id: svn://anonsvn.mit.edu/krb5/trunk@15831 dc483132-0cff-0310-8789-dd5450dbe970

7 files changed, 134 insertions, 5 deletions

diff --git a/src/lib/krb5/asn.1/ChangeLog b/src/lib/krb5/asn.1/ChangeLog
index 578352bdb..520999d7f 100644
--- a/src/lib/krb5/asn.1/ChangeLog
+++ b/src/lib/krb5/asn.1/ChangeLog
@@ -1,3 +1,19 @@
+2003-10-08  Tom Yu  <tlyu@mit.edu>
+
+	* asn1_k_encode.c (asn1_encode_krb_saved_safe_body): New function;
+	kludge to insert a raw pre-encoded KRB-SAFE-BODY.
+
+	* asn1_k_encode.h (asn1_encode_krb_saved_safe_body): Add
+	prototype.
+
+	* krb5_decode.c (decode_krb5_safe_with_body): New function; saves
+	a copy of the encoding of the KRB-SAFE-BODY to avoid problems
+	caused by re-encoding it during verification.
+
+	* krb5_encode.c (encode_krb5_safe_with_body): New function;
+	re-encode a KRB-SAFE using a saved KRB-SAFE-BODY encoding, to
+	avoid trouble with re-encoding a KRB-SAFE-BODY.
+
 2003-07-22  Sam Hartman  <hartmans@avalanche-breakdown.mit.edu>
 
 	* asn1_k_decode.c (asn1_decode_etype_info2_entry_1_3): Decoder for
diff --git a/src/lib/krb5/asn.1/asn1_k_encode.c b/src/lib/krb5/asn.1/asn1_k_encode.c
index 325a6ce77..00cfab032 100644
--- a/src/lib/krb5/asn.1/asn1_k_encode.c
+++ b/src/lib/krb5/asn.1/asn1_k_encode.c
@@ -942,3 +942,20 @@ asn1_error_code asn1_encode_predicted_sam_response(asn1buf *buf, const krb5_pred
 
   asn1_cleanup();
 }
+
+/*
+ * Do some ugliness to insert a raw pre-encoded KRB-SAFE-BODY.
+ */
+asn1_error_code asn1_encode_krb_saved_safe_body(asn1buf *buf, const krb5_data *body, unsigned int *retlen)
+{
+  asn1_error_code retval;
+
+  retval = asn1buf_insert_octetstring(buf, body->length,
+				      (krb5_octet *)body->data);
+  if (retval){
+    asn1buf_destroy(&buf);
+    return retval; 
+  }
+  *retlen = body->length;
+  return 0;
+}
diff --git a/src/lib/krb5/asn.1/asn1_k_encode.h b/src/lib/krb5/asn.1/asn1_k_encode.h
index a2429a778..caa46c570 100644
--- a/src/lib/krb5/asn.1/asn1_k_encode.h
+++ b/src/lib/krb5/asn.1/asn1_k_encode.h
@@ -266,4 +266,7 @@ asn1_error_code asn1_encode_predicted_sam_response
 	(asn1buf *buf, const krb5_predicted_sam_response *val, 
 		   unsigned int *retlen);
 
+asn1_error_code asn1_encode_krb_saved_safe_body
+	(asn1buf *buf, const krb5_data *body, unsigned int *retlen);
+
 #endif
diff --git a/src/lib/krb5/asn.1/krb5_decode.c b/src/lib/krb5/asn.1/krb5_decode.c
index 4172c882b..596997fe9 100644
--- a/src/lib/krb5/asn.1/krb5_decode.c
+++ b/src/lib/krb5/asn.1/krb5_decode.c
@@ -90,6 +90,7 @@ if((var) == NULL) clean_return(ENOMEM)
   construction = t2.construction;		\
   tagnum = t2.tagnum;				\
   indef = t2.indef;				\
+  taglen = t2.length;				\
 }
 
 #define get_eoc()						\
@@ -107,6 +108,7 @@ if((var) == NULL) clean_return(ENOMEM)
 
 /* decode sequence header and initialize tagnum with the first field */
 #define begin_structure()\
+unsigned int taglen;\
 asn1buf subbuf;\
 int seqindef;\
 int indef;\
@@ -494,8 +496,26 @@ krb5_error_code decode_krb5_kdc_req_body(const krb5_data *code, krb5_kdc_req **r
   cleanup(free);
 }
 
-krb5_error_code decode_krb5_safe(const krb5_data *code, krb5_safe **rep)
+/*
+ * decode_krb5_safe_with_body
+ *
+ * Like decode_krb5_safe(), but grabs the encoding of the
+ * KRB-SAFE-BODY as well, in case re-encoding would produce a
+ * different encoding.  (Yes, we're using DER, but there's this
+ * annoying problem with pre-1.3.x code using signed sequence numbers,
+ * which we permissively decode and cram into unsigned 32-bit numbers.
+ * When they're re-encoded, they're no longer negative if they started
+ * out negative, so checksum verification fails.)
+ *
+ * This does *not* perform any copying; the returned pointer to the
+ * encoded KRB-SAFE-BODY points into the input buffer.
+ */
+krb5_error_code decode_krb5_safe_with_body(
+  const krb5_data *code,
+  krb5_safe **rep,
+  krb5_data *body)
 {
+  krb5_data tmpbody;
   setup();
   alloc_field(*rep,krb5_safe);
   clear_field(rep,checksum);
@@ -511,12 +531,26 @@ krb5_error_code decode_krb5_safe(const krb5_data *code, krb5_safe **rep)
       if(msg_type != KRB5_SAFE) clean_return(KRB5_BADMSGTYPE);
 #endif
     }
+    /*
+     * Gross kludge to extract pointer to encoded safe-body.  Relies
+     * on tag prefetch done by next_tag().  Don't handle indefinite
+     * encoding, as it's too much work.
+     */
+    if (!indef) {
+      tmpbody.length = taglen;
+      tmpbody.data = subbuf.next;
+    } else {
+      tmpbody.length = 0;
+      tmpbody.data = NULL;
+    }
     get_field(**rep,2,asn1_decode_krb_safe_body);
     alloc_field((*rep)->checksum,krb5_checksum);
     get_field(*((*rep)->checksum),3,asn1_decode_checksum);
   (*rep)->magic = KV5M_SAFE;
     end_structure();
   }
+  if (body != NULL)
+    *body = tmpbody;
   cleanup_manual();
 error_out:
   if (rep && *rep) {
@@ -526,6 +560,11 @@ error_out:
   return retval;
 }
 
+krb5_error_code decode_krb5_safe(const krb5_data *code, krb5_safe **rep)
+{
+  return decode_krb5_safe_with_body(code, rep, NULL);
+}
+
 krb5_error_code decode_krb5_priv(const krb5_data *code, krb5_priv **rep)
 {
   setup();
diff --git a/src/lib/krb5/asn.1/krb5_encode.c b/src/lib/krb5/asn.1/krb5_encode.c
index eed6faf45..ecdfa1878 100644
--- a/src/lib/krb5/asn.1/krb5_encode.c
+++ b/src/lib/krb5/asn.1/krb5_encode.c
@@ -478,6 +478,43 @@ krb5_error_code encode_krb5_safe(const krb5_safe *rep, krb5_data **code)
   krb5_cleanup();
 }
 
+/*
+ * encode_krb5_safe_with_body
+ *
+ * Like encode_krb5_safe(), except takes a saved KRB-SAFE-BODY
+ * encoding to avoid problems with re-encoding.
+ */
+krb5_error_code encode_krb5_safe_with_body(
+  const krb5_safe *rep,
+  const krb5_data *body,
+  krb5_data **code)
+{
+  krb5_setup();
+
+  if (body == NULL) {
+      asn1buf_destroy(&buf);
+      return ASN1_MISSING_FIELD;
+  }
+
+  /* cksum[3]		Checksum */
+  krb5_addfield(rep->checksum,3,asn1_encode_checksum);
+
+  /* safe-body[2]	KRB-SAFE-BODY */
+  krb5_addfield(body,2,asn1_encode_krb_saved_safe_body);
+
+  /* msg-type[1]	INTEGER */
+  krb5_addfield(ASN1_KRB_SAFE,1,asn1_encode_integer);
+
+  /* pvno[0]		INTEGER */
+  krb5_addfield(KVNO,0,asn1_encode_integer);
+
+  /* KRB-SAFE ::= [APPLICATION 20] SEQUENCE */
+  krb5_makeseq();
+  krb5_apptag(20);
+
+  krb5_cleanup();
+}
+
 krb5_error_code encode_krb5_priv(const krb5_priv *rep, krb5_data **code)
 {
   krb5_setup();
diff --git a/src/lib/krb5/krb/ChangeLog b/src/lib/krb5/krb/ChangeLog
index 5d777909a..57d63bfd7 100644
--- a/src/lib/krb5/krb/ChangeLog
+++ b/src/lib/krb5/krb/ChangeLog
@@ -1,3 +1,10 @@
+2003-10-08  Tom Yu  <tlyu@mit.edu>
+
+	* rd_safe.c (krb5_rd_safe_basic): Save the encoded KRB-SAFE-BODY
+	to avoid trouble caused by re-encoding.  Also, handle correctly
+	implemented RFC 1510 KRB-SAFE, i.e., checksummed over
+	KRB-SAFE-BODY only.
+
 2003-09-02  Tom Yu  <tlyu@mit.edu>
 
 	* conv_creds.c (krb524_convert_creds_plain): Apply patch from
diff --git a/src/lib/krb5/krb/rd_safe.c b/src/lib/krb5/krb/rd_safe.c
index 41c2596b7..15dc6dccc 100644
--- a/src/lib/krb5/krb/rd_safe.c
+++ b/src/lib/krb5/krb/rd_safe.c
@@ -51,6 +51,7 @@ krb5_rd_safe_basic(krb5_context context, const krb5_data *inbuf, const krb5_keyb
 {
     krb5_error_code 	  retval;
     krb5_safe 		* message;
+    krb5_data safe_body;
     krb5_checksum our_cksum, *his_cksum;
     krb5_octet zero_octet = 0;
     krb5_data *scratch;
@@ -59,7 +60,7 @@ krb5_rd_safe_basic(krb5_context context, const krb5_data *inbuf, const krb5_keyb
     if (!krb5_is_krb_safe(inbuf))
 	return KRB5KRB_AP_ERR_MSG_TYPE;
 
-    if ((retval = decode_krb5_safe(inbuf, &message)))
+    if ((retval = decode_krb5_safe_with_body(inbuf, &message, &safe_body)))
 	return retval;
 
     if (!krb5_c_valid_cksumtype(message->checksum->checksum_type)) {
@@ -113,7 +114,7 @@ krb5_rd_safe_basic(krb5_context context, const krb5_data *inbuf, const krb5_keyb
 
     message->checksum = &our_cksum;
 
-    if ((retval = encode_krb5_safe(message, &scratch)))
+    if ((retval = encode_krb5_safe_with_body(message, &safe_body, &scratch)))
 	goto cleanup;
 
     message->checksum = his_cksum;
@@ -126,8 +127,17 @@ krb5_rd_safe_basic(krb5_context context, const krb5_data *inbuf, const krb5_keyb
     krb5_free_data(context, scratch);
     
     if (!valid) {
-	retval = KRB5KRB_AP_ERR_MODIFIED;
-	goto cleanup;
+	/*
+	 * Checksum over only the KRB-SAFE-BODY, like RFC 1510 says, in
+	 * case someone actually implements it correctly.
+	 */
+	retval = krb5_c_verify_checksum(context, keyblock,
+					KRB5_KEYUSAGE_KRB_SAFE_CKSUM,
+					&safe_body, his_cksum, &valid);
+	if (!valid) {
+	    retval = KRB5KRB_AP_ERR_MODIFIED;
+	    goto cleanup;
+	}
     }
 
     replaydata->timestamp = message->timestamp;