changeset 910:16a4844d963b base64-proposed-fix

Use BIO_FLAGS_BASE64_NO_NL in base64_{en,de}code() Use BIO_FLAGS_BASE64_NO_NL in base64_encode() and base64_decode() instead of manually adding and removing newlines in order to compensate for OpenSSL requiring or adding them: code which has been a rich source of bugs. As a result of this change the output of base64_encode() may no longer be assumed to be '\0' terminated and the caller has been updated accordingly. As a side-effect this resolves a bug in base64_encode where the length of the output is calculated to be one greater than it actually is by the code that strips newlines in the case where a 44 byte user name and authentication id is used with a 16 byte password. Signed-off-by: Simon Horman <horms@verge.net.au>
author Simon Horman <horms@verge.net.au>
date Fri, 27 Sep 2013 07:34:21 +0900
parents dfbd5973e7b9
children 0021ea8aaf17
files perdition/base64.c perdition/buf.h perdition/sasl_plain.c
diffstat 3 files changed, 44 insertions(+), 66 deletions(-) [+]
line wrap: on
line diff
--- a/perdition/base64.c	Mon Sep 23 15:04:50 2013 +0900
+++ b/perdition/base64.c	Fri Sep 27 07:34:21 2013 +0900
@@ -7,21 +7,6 @@
 #include "log.h"
 #include "buf.h"
 
-
-/* Openssl's base64 encode places an '\n' after every 64 bytes of encoded
- * text and at the end of the encoded text */
-
-static void base64_encode_clean(struct buf *buf)
-{
-	size_t i, o;
-
-	for (i = 65, o = 64; i < buf->len; i += 65, o += 64)
-		memmove(buf->data + o, buf->data + i, buf->len - i);
-
-	buf->len -= (buf->len + 64) / 65;
-	buf->data[buf->len] = '\0';
-}
-
 /**********************************************************************
  * base64_encode
  * pre: in: struct buf to decode
@@ -46,6 +31,8 @@
 		goto err;
 	}
 
+	BIO_set_flags(b64_bio, BIO_FLAGS_BASE64_NO_NL);
+
 	mem_bio = BIO_new(BIO_s_mem());
 	if (!mem_bio) {
 		VANESSA_LOGGER_DEBUG("BIO_new mem_bio");
@@ -84,7 +71,6 @@
 	}
 
 	memcpy(out.data, b64_data, out.len);
-	base64_encode_clean(&out);
 
 err:
 	if (!out.data)
@@ -94,45 +80,6 @@
 	return out;
 }
 
-#define MIN(x, y) (x < y ? x : y)
-
-static struct buf base64_decode_clean(const struct const_buf *in)
-{
-	STRUCT_BUF(out);
-	int i;
-	size_t o;
-
-	/* As per RFC2045 the maximum encoded line length is 76,
-	 * the line separator is "\r\n" and
-	 * the encoded text must end with a line separator
-	 */
-	out.len = in->len + ((in->len / 76) + 1) * 2;
-
-	/* Overflow */
-	if (in->len > out.len) {
-		VANESSA_LOGGER_DEBUG("string is too long");
-		goto err;
-	}
-
-	out.data = malloc(out.len);
-	if (!out.data) {
-		VANESSA_LOGGER_DEBUG_ERRNO("malloc");
-		goto err;
-	}
-
-	for (i = o = 0; o < out.len; i += 76, o += 78) {
-		size_t copy_len = MIN(76, in->len - i);
-		memcpy(out.data + o, in->data + i, copy_len);
-		out.data[o + copy_len] = '\r';
-		out.data[o + copy_len + 1] = '\n';
-	}
-
-err:
-	if (!out.data)
-		buf_zero(&out);
-	return out;
-}
-
 /**********************************************************************
  * base64_decode
  * pre: in: struct buf to decode
@@ -147,21 +94,23 @@
 	BIO *mem_bio = NULL, *b64_bio;
 	int ilen, error = 1;
 	STRUCT_BUF(out);
-	STRUCT_BUF(clean);
+	STRUCT_BUF(dup);
 
-	clean = base64_decode_clean(in);
-	if (buf_is_err(&clean)) {
-		VANESSA_LOGGER_DEBUG("base64_decode_clean");
+	/* This is necessary because BIO_new_mem_buf takes
+	 * a non-const first argument */
+	dup = buf_dup_from_const(in);
+	if (buf_is_err(&dup)) {
+		VANESSA_LOGGER_DEBUG("buf_dup_from_const");
 		goto err;
 	}
 
-	if (SIZE_MAX > INT_MAX && clean.len > INT_MAX) {
+	if (SIZE_MAX > INT_MAX && dup.len > INT_MAX) {
 		VANESSA_LOGGER_DEBUG("str is too long");
 		goto err;
 	}
-	ilen = (int)clean.len;
+	ilen = (int)dup.len;
 
-	mem_bio = BIO_new_mem_buf(clean.data, ilen);
+	mem_bio = BIO_new_mem_buf(dup.data, dup.len);
 	if (!mem_bio) {
 		VANESSA_LOGGER_DEBUG("BIO_new mem_bio");
 		goto err;
@@ -173,9 +122,10 @@
 		goto err;
 	}
 
+	BIO_set_flags(b64_bio, BIO_FLAGS_BASE64_NO_NL);
 	mem_bio = BIO_push(b64_bio, mem_bio);
 
-	out.data = calloc(1, clean.len);
+	out.data = calloc(1, dup.len);
 	if (!out.data) {
 		VANESSA_LOGGER_DEBUG_ERRNO("malloc");
 		goto err;
@@ -203,6 +153,6 @@
 	}
 	if (mem_bio)
 		BIO_free_all(mem_bio);
-	free(clean.data);
+	free(dup.data);
 	return out;
 }
--- a/perdition/buf.h	Mon Sep 23 15:04:50 2013 +0900
+++ b/perdition/buf.h	Fri Sep 27 07:34:21 2013 +0900
@@ -30,5 +30,26 @@
 	return b->data == NULL;
 }
 
+/**********************************************************************
+ * buf_dup_from_const
+ * pre: in: struct const_buf to duplicate
+ * return: on success: { .buf = str, .len = len }
+ *         on error: { .buf = NULL, .len = 0 }
+ *         buf_is_err() can be used to test for { .buf == NULL }
+ **********************************************************************/
+
+static inline struct buf buf_dup_from_const(const struct const_buf *src)
+{
+	STRUCT_BUF(dst);
+
+	dst.data = malloc(src->len);
+	if (dst.data) {
+		memcpy(dst.data, src->data, src->len);
+		dst.len = src->len;
+	}
+
+	return dst;
+}
+
 #endif /* PERDITION_BUF_H */
 
--- a/perdition/sasl_plain.c	Mon Sep 23 15:04:50 2013 +0900
+++ b/perdition/sasl_plain.c	Fri Sep 27 07:34:21 2013 +0900
@@ -187,7 +187,7 @@
 {
 	STRUCT_BUF(in);
 	STRUCT_BUF(out);
-	char *p;
+	char *p, *out_str = NULL;
 
 	if (auth->authorisation_id)
 		in.len += strlen(auth->authorisation_id);
@@ -218,7 +218,14 @@
 		goto err;
 	}
 
+	out_str = strn_to_str(out.data, out.len);
+	if (!out_str) {
+		VANESSA_LOGGER_DEBUG("strn_to_str");
+		goto err;
+	}
+
 err:
+	free(out.data);
 	free(in.data);
-	return out.data;
+	return out_str;
 }