Subject: bug in cgd+blowfish
To: None <tech-security@NetBSD.ORG>
From: Roland Dowdeswell <elric@imrryr.org>
List: tech-security
Date: 04/15/2003 04:01:19
------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <29567.1050393522.1@imrryr.org>

I've discovered with the help of Dan Carosone a bug in the cgd(4)
driver which was caused by my misunderstanding of the blowfish API.
I was under the impression when I wrote the code that BF_set_key()
took its key length in bits but it in fact takes said length in
bytes.

Just fixing the bug will cause an upgrade problem for existing
users of cgd(4), so I have come up with a strategy that will allow
a new cgdconfig(8) to come up with older blowfish keys by extending
their length and filling the additional bits with what the previous
cgdconfig(8) would have.

My proposed action is to:

	1.  Define a new directive for the params file called
	    ``broken_blowfish'' which takes an int and will if
	    supplied generate one of the two old keys by extending
	    the number of bits and filling them properly.
	2.  Check it in.
	3.  Fix the kernel bug.
	4.  Alert people that they need to add the new directive
	    if they are using cgd and blowfish.

I've written the code for cgdconfig(8) [step 1] to implement the
new directive and include the patch here for review.  I still
haven't cleaned up some of the debugging that I put in it to
facilitate the writing of it.

A full dump/restore might be wise, but for the patient I am working
on cgdrekey(8) a utility to change the encryption of the disk in
place.  (I make no guarantees about how long it will take to finish,
given that I have a day job and it is a few items down on the TODO
list.  :-)

Comments?

Thanks,

--
    Roland Dowdeswell                      http://www.Imrryr.ORG/~elric/

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <29567.1050393522.2@imrryr.org>

cvs server: Diffing .
Index: cgdconfig.c
===================================================================
RCS file: /cvsroot/src/sbin/cgdconfig/cgdconfig.c,v
retrieving revision 1.6
diff -u -r1.6 cgdconfig.c
--- cgdconfig.c	2003/03/24 03:12:22	1.6
+++ cgdconfig.c	2003/04/15 07:55:40
@@ -94,7 +94,7 @@
 
 static int	 configure_params(int, const char *, const char *,
 				  struct params *);
-static bits_t	*getkey(const char *, struct keygen *, int);
+static bits_t	*getkey(const char *, struct keygen *, int, int);
 static bits_t	*getkey_storedkey(const char *, struct keygen *, int);
 static bits_t	*getkey_randomkey(const char *, struct keygen *, int);
 static bits_t	*getkey_pkcs5_pbkdf2(const char *, struct keygen *, int);
@@ -104,6 +104,7 @@
 static int	 verify_disklabel(int);
 static int	 verify_ffs(int);
 
+static void	 key_print(FILE *, const u_int8_t *, int);
 static void	 usage(void);
 
 /* Verbose Framework */
@@ -242,12 +243,39 @@
 }
 
 static bits_t *
-getkey(const char *dev, struct keygen *kg, int len)
+getkey(const char *dev, struct keygen *kg, int len, int broken_blowfish)
 {
 	bits_t	*ret = NULL;
 	bits_t	*tmp;
+	int	 gen_len;
+	int	 pkcs5_pbkdf2_len;
 
 	VPRINTF(3, ("getkey(\"%s\", %p, %d) called\n", dev, kg, len));
+
+	/*
+	 * This code is a hack to deal with a bug in the first release
+	 * of cgd(4) in the blowfish-cbc cipher.  The problem is that
+	 * BF_set_key() was being passed the number of bits in the key
+	 * rather than the number of bytes.  This led to the key being
+	 * padded in two different ways which we deal with both
+	 * BROKEN_BLOWFISH_V1 and BROKEN_BLOWFISH_V2.  This should be
+	 * deprecated at some point in the near future to clean up this
+	 * code.
+	 */
+	pkcs5_pbkdf2_len = len;
+	gen_len = len;
+	switch (broken_blowfish) {
+	case BROKEN_BLOWFISH_V1:
+		pkcs5_pbkdf2_len = (len / 160 + 1) * 160;
+		/*FALLTHROUGH*/
+	case BROKEN_BLOWFISH_V2:
+		gen_len = len * 8;
+		break;
+	default:
+		break;
+	}
+VPRINTF(1, ("broken_blowfish = %d\n", broken_blowfish));
+
 	for (; kg; kg=kg->next) {
 		switch (kg->kg_method) {
 		case KEYGEN_STOREDKEY:
@@ -257,7 +285,7 @@
 			tmp = getkey_randomkey(dev, kg, len);
 			break;
 		case KEYGEN_PKCS5_PBKDF2:
-			tmp = getkey_pkcs5_pbkdf2(dev, kg, len);
+			tmp = getkey_pkcs5_pbkdf2(dev, kg, pkcs5_pbkdf2_len);
 			break;
 		default:
 			warnx("unrecognised keygen method %d in getkey()",
@@ -273,6 +301,7 @@
 			ret = tmp;
 	}
 
+	bits_extend(ret, gen_len);
 	return ret;
 }
 
@@ -430,7 +459,8 @@
 		if (p->key)
 			bits_free(p->key);
 
-		p->key = getkey(argv[1], p->keygen, p->keylen);
+		p->key = getkey(argv[1], p->keygen, p->keylen,
+		    p->broken_blowfish);
 		if (!p->key)
 			goto bail_err;
 
@@ -529,7 +559,7 @@
 	ci.ci_alg = (char *)string_tocharstar(p->algorithm);
 	ci.ci_ivmethod = (char *)string_tocharstar(p->ivmeth);
 	ci.ci_key = (char *)bits_getbuf(p->key);
-	ci.ci_keylen = p->keylen;
+	ci.ci_keylen = bits_len(p->key);
 	ci.ci_blocksize = p->bsize;
 
 	VPRINTF(1, ("    with alg %s keylen %d blocksize %d ivmethod %s\n",
@@ -538,6 +568,8 @@
 	VPRINTF(2, ("key: "));
 	VERBOSE(2, bits_fprint(stdout, p->key));
 	VPRINTF(2, ("\n"));
+	VERBOSE(2, key_print(stdout, bits_getbuf(p->key), bits_len(p->key)));
+	VPRINTF(2, ("\n"));
 
 	if (nflag)
 		return 0;
@@ -680,7 +712,8 @@
 		return -1;
 	}
 
-	oldp->key = getkey("old file", oldp->keygen, oldp->keylen);
+	oldp->key = getkey("old file", oldp->keygen, oldp->keylen,
+	    oldp->broken_blowfish);
 
 	/* we copy across the non-keygen info, here. */
 
@@ -703,7 +736,7 @@
 	}
 	params_filldefaults(p);
 	keygen_filldefaults(p->keygen, p->keylen);
-	p->key = getkey("new file", p->keygen, p->keylen);
+	p->key = getkey("new file", p->keygen, p->keylen, p->broken_blowfish);
 
 	kg = keygen_generate(KEYGEN_STOREDKEY);
 	kg->kg_key = bits_xor(p->key, oldp->key);
@@ -770,4 +803,28 @@
 		words_free(my_argv, my_argc);
 	}
 	return ret;
+}
+
+/*
+ * XXX: key_print doesn't work quite exactly properly if the keylength
+ *      is not evenly divisible by 8.  If the key is not divisible by
+ *      8 then a few extra bits are printed.
+ */
+
+static void
+key_print(FILE *f, const u_int8_t *key, int len)
+{
+	int	i;
+	int	col;
+
+	len = BITS2BYTES(len);
+	fprintf(f, "key: ");
+	for (i=0, col=5; i < len; i++, col+=2) {
+		fprintf(f, "%02x", key[i]);
+		if (col > 70) {
+			col = 5 - 2;
+			fprintf(f, "\n     ");
+		}
+	}
+	fprintf(f, "\n");
 }
Index: cgdlex.l
===================================================================
RCS file: /cvsroot/src/sbin/cgdconfig/cgdlex.l,v
retrieving revision 1.1
diff -u -r1.1 cgdlex.l
--- cgdlex.l	2003/03/24 02:02:50	1.1
+++ cgdlex.l	2003/04/15 07:55:40
@@ -88,6 +88,7 @@
 keygen_salt				{ RETTOKEN(KEYGEN_SALT); }
 keygen_iterations			{ RETTOKEN(KEYGEN_ITERATIONS); }
 xor_key					{ RETTOKEN(XOR_KEY); }
+broken_blowfish				{ RETTOKEN(BROKEN_BLOWFISH); }
 [;\n]					{ return EOL; }
 \\\n					/* ignore a quoted nl */
 [ \t]					/* ignore spaces and tabs */
Index: cgdparse.y
===================================================================
RCS file: /cvsroot/src/sbin/cgdconfig/cgdparse.y,v
retrieving revision 1.1
diff -u -r1.1 cgdparse.y
--- cgdparse.y	2003/03/24 02:02:50	1.1
+++ cgdparse.y	2003/04/15 07:55:40
@@ -86,6 +86,7 @@
 
 /* Deprecated tokens */
 %token <token> KEYGEN_METHOD KEYGEN_SALT KEYGEN_ITERATIONS XOR_KEY
+%token <token> BROKEN_BLOWFISH
 
 %%
 
@@ -98,6 +99,7 @@
 	| KEYLENGTH INTEGER EOL		{ $$ = params_keylen($2); }
 	| IVMETHOD stringlit EOL	{ $$ = params_ivmeth($2); }
 	| VERIFY_METHOD stringlit EOL	{ $$ = params_verify_method($2); }
+	| BROKEN_BLOWFISH INTEGER EOL	{ $$ = params_broken_blowfish($2); }
 	| kgrule			{ $$ = params_keygen($1); }
 	| deprecated			{ $$ = params_dep_keygen($1); }
 	| EOL				{ $$ = NULL; }
Index: params.c
===================================================================
RCS file: /cvsroot/src/sbin/cgdconfig/params.c,v
retrieving revision 1.6
diff -u -r1.6 params.c
--- params.c	2003/04/10 05:45:29	1.6
+++ params.c	2003/04/15 07:55:40
@@ -103,6 +103,9 @@
 	p->verify_method = VERIFY_UNKNOWN;
 	p->dep_keygen = NULL;
 	p->keygen = NULL;
+
+	/* for an old bug */
+	p->broken_blowfish = BROKEN_BLOWFISH_UNKNOWN;
 }
 
 void
@@ -140,6 +143,8 @@
 		p->bsize = p2->bsize;
 	if (p2->verify_method != VERIFY_UNKNOWN)
 		p->verify_method = p2->verify_method;
+	if (p2->broken_blowfish != BROKEN_BLOWFISH_UNKNOWN)
+		p->broken_blowfish = p2->broken_blowfish;
 
 	p->dep_keygen = keygen_combine(p->dep_keygen, p2->dep_keygen);
 	keygen_addlist(&p->keygen, p2->keygen);
@@ -287,6 +292,15 @@
 	struct params *p = params_new();
 
 	p->dep_keygen = in;
+	return p;
+}
+
+struct params *
+params_broken_blowfish(int version)
+{
+	struct params *p = params_new();
+
+	p->broken_blowfish = version;
 	return p;
 }
 
Index: params.h
===================================================================
RCS file: /cvsroot/src/sbin/cgdconfig/params.h,v
retrieving revision 1.4
diff -u -r1.4 params.h
--- params.h	2003/03/24 02:02:51	1.4
+++ params.h	2003/04/15 07:55:40
@@ -56,6 +56,7 @@
 	int		 keylen;
 	int		 bsize;
 	int		 verify_method;
+	int		 broken_blowfish;
 	struct keygen	*dep_keygen;
 	struct keygen	*keygen;
 };
@@ -74,6 +75,13 @@
 #define VERIFY_DISKLABEL	0x2
 #define VERIFY_FFS		0x3
 
+/* broken blowfish number... */
+
+#define BROKEN_BLOWFISH_UNKNOWN	0x0
+#define BROKEN_BLOWFISH_NONE	0x0
+#define BROKEN_BLOWFISH_V1	0x1
+#define BROKEN_BLOWFISH_V2	0x2
+
 __BEGIN_DECLS
 struct params	*params_new(void);
 void		 params_free(struct params *);
@@ -89,6 +97,8 @@
 struct params	*params_verify_method(string_t *);
 struct params	*params_keygen(struct keygen *);
 struct params	*params_dep_keygen(struct keygen *);
+
+struct params	*params_broken_blowfish(int);
 
 struct params	*params_fget(FILE *);
 struct params	*params_cget(const char *);
Index: utils.c
===================================================================
RCS file: /cvsroot/src/sbin/cgdconfig/utils.c,v
retrieving revision 1.2
diff -u -r1.2 utils.c
--- utils.c	2003/03/24 02:02:52	1.2
+++ utils.c	2003/04/15 07:55:41
@@ -300,6 +300,21 @@
 }
 
 bits_t *
+bits_extend(bits_t *in, int new_len)
+{
+	char	*old;
+
+	if (new_len < 0)
+		return NULL;
+	old = in->text;
+	in->text = calloc(1, BITS2BYTES(new_len));
+	memcpy(in->text, old, BITS2BYTES(in->length));
+	in->length = new_len;
+	free(old);
+	return in;
+}
+
+bits_t *
 bits_xor(const bits_t *x1, const bits_t *x2)
 {
 	bits_t	*b;
@@ -311,6 +326,11 @@
 	b->text = calloc(1, BITS2BYTES(b->length));
 	for (i=0; i < BITS2BYTES(MIN(x1->length, x2->length)); i++)
 		b->text[i] = x1->text[i] ^ x2->text[i];
+	/* only one of the following two loops will run */
+	for (; i < BITS2BYTES(x1->length); i++)
+		b->text[i] = x1->text[i];
+	for (; i < BITS2BYTES(x2->length); i++)
+		b->text[i] = x2->text[i];
 	return b;
 }
 
Index: utils.h
===================================================================
RCS file: /cvsroot/src/sbin/cgdconfig/utils.h,v
retrieving revision 1.2
diff -u -r1.2 utils.h
--- utils.h	2003/03/24 02:02:52	1.2
+++ utils.h	2003/04/15 07:55:41
@@ -76,6 +76,7 @@
 const void	 *bits_getbuf(bits_t *);
 int		  bits_len(bits_t *);
 
+bits_t		 *bits_extend(bits_t *, int);
 bits_t		 *bits_xor(const bits_t *, const bits_t *);
 bits_t		 *bits_xor_d(bits_t *, bits_t *);
 

------- =_aaaaaaaaaa0--