Mercurial > pidgin

changeset 28213:33921125348d

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
The output buffer passed to qq_encrypt needs to be 17 bytes bigger than the data you're encrypting, not 16 bytes bigger. Fixes #10191. It's hard to say whether this actually causes problems. My guess is that it does not. However, the way the qq protocol plugin constructs the plain text buffer to be passed to qq_encrypt is error prone, and the many calls to g_newa(guint8, MAX_PACKET_SIZE) are really bad because MAX_PACKET_SIZE is 64KB. This is a ridiculous amount of space to request on the stack. All these qq_put8 qq_put16 qq_put32 qq_putdata functions should be changed to insert data into a dynamically allocated GString instead of the stack-allocated buffers that they use now. This eliminates the potential for accidentally overwriting the end of the buffer. And the second g_newa() for the output buffer passed into qq_encrypt() should be changed to allocate space on the heap in most places because, as previously noted, 64KB is a ridiculous amount of memory to request from the stack. Heap allocation may be expensive when compared to stack allocation, but I feel it's usually worth it to eliminate the possibilty of buffer overflow.
author Mark Doliner <mark@kingant.net>
date Fri, 04 Sep 2009 22:50:26 +0000
parents 9af95186dde4
children 9ab9e5f1eec2
files libpurple/protocols/qq/file_trans.c libpurple/protocols/qq/qq_base.c libpurple/protocols/qq/qq_crypt.c libpurple/protocols/qq/qq_network.c
diffstat 4 files changed, 35 insertions(+), 30 deletions(-) [+]
line wrap: on
line diff
--- a/libpurple/protocols/qq/file_trans.c	Fri Sep 04 21:12:11 2009 +0000
+++ b/libpurple/protocols/qq/file_trans.c	Fri Sep 04 22:50:26 2009 +0000
@@ -334,7 +334,7 @@
 		raw_data, bytes,
 		"sending packet[%s]:", qq_get_file_cmd_desc(packet_type));
 
-	encrypted = g_newa(guint8, bytes + 16);
+	encrypted = g_newa(guint8, bytes + 17);
 	encrypted_len = qq_encrypt(encrypted, raw_data, bytes, info->file_session_key);
 	/*debug: try to decrypt it */
 
--- a/libpurple/protocols/qq/qq_base.c	Fri Sep 04 21:12:11 2009 +0000
+++ b/libpurple/protocols/qq/qq_base.c	Fri Sep 04 22:50:26 2009 +0000
@@ -245,10 +245,10 @@
 
 	g_return_if_fail(qd->ld.token != NULL && qd->ld.token_len > 0);
 
-	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 16);
-	memset(raw_data, 0, MAX_PACKET_SIZE - 16);
+	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 17);
+	memset(raw_data, 0, MAX_PACKET_SIZE - 17);
 
-	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 16 bytes more */
+	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 17 bytes more */
 
 	bytes = 0;
 	/* now generate the encrypted data
@@ -609,7 +609,7 @@
 	raw_data = g_newa(guint8, 128);
 	memset(raw_data, 0, 128);
 
-	encrypted = g_newa(guint8, 128 + 16);	/* 16 bytes more */
+	encrypted = g_newa(guint8, 128 + 17);	/* 17 bytes more */
 
 	bytes = 0;
 	if (qd->redirect == NULL) {
@@ -682,10 +682,10 @@
 
 	g_return_if_fail(qd->ld.token != NULL && qd->ld.token_len > 0);
 
-	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 16);
-	memset(raw_data, 0, MAX_PACKET_SIZE - 16);
+	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 17);
+	memset(raw_data, 0, MAX_PACKET_SIZE - 17);
 
-	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 16 bytes more */
+	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 17 bytes more */
 
 	bytes = 0;
 	bytes += qq_put8(raw_data + bytes, qd->ld.token_len);
@@ -721,10 +721,10 @@
 
 	g_return_if_fail(qd->ld.token != NULL && qd->ld.token_len > 0);
 
-	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 16);
-	memset(raw_data, 0, MAX_PACKET_SIZE - 16);
+	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 17);
+	memset(raw_data, 0, MAX_PACKET_SIZE - 17);
 
-	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 16 bytes more */
+	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 17 bytes more */
 
 	bytes = 0;
 	bytes += qq_put8(raw_data + bytes, qd->ld.token_len);
@@ -765,10 +765,10 @@
 	g_return_if_fail(qd->ld.token != NULL && qd->ld.token_len > 0);
 	g_return_if_fail(code != NULL && code_len > 0);
 
-	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 16);
-	memset(raw_data, 0, MAX_PACKET_SIZE - 16);
+	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 17);
+	memset(raw_data, 0, MAX_PACKET_SIZE - 17);
 
-	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 16 bytes more */
+	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 17 bytes more */
 
 	bytes = 0;
 	bytes += qq_put8(raw_data + bytes, qd->ld.token_len);
@@ -998,10 +998,10 @@
 
 	g_return_if_fail(qd->ld.token_ex != NULL && qd->ld.token_ex_len > 0);
 
-	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 16);
-	memset(raw_data, 0, MAX_PACKET_SIZE - 16);
+	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 17);
+	memset(raw_data, 0, MAX_PACKET_SIZE - 17);
 
-	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 16 bytes more */
+	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 17 bytes more */
 
 	/* Encrypted password and put in encrypted */
 	bytes = 0;
@@ -1166,10 +1166,10 @@
 
 	g_return_if_fail(qd->ld.token != NULL && qd->ld.token_len > 0);
 
-	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 16);
-	memset(raw_data, 0, MAX_PACKET_SIZE - 16);
+	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 17);
+	memset(raw_data, 0, MAX_PACKET_SIZE - 17);
 
-	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 16 bytes more */
+	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 17 bytes more */
 
 	/* Encrypted password and put in encrypted */
 	bytes = 0;
@@ -1342,10 +1342,10 @@
 
 	g_return_if_fail(qd->ld.token != NULL && qd->ld.token_len > 0);
 
-	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 16);
-	memset(raw_data, 0, MAX_PACKET_SIZE - 16);
+	raw_data = g_newa(guint8, MAX_PACKET_SIZE - 17);
+	memset(raw_data, 0, MAX_PACKET_SIZE - 17);
 
-	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 16 bytes more */
+	encrypted = g_newa(guint8, MAX_PACKET_SIZE);	/* 17 bytes more */
 
 	/* Encrypted password and put in encrypted */
 	bytes = 0;
--- a/libpurple/protocols/qq/qq_crypt.c	Fri Sep 04 21:12:11 2009 +0000
+++ b/libpurple/protocols/qq/qq_crypt.c	Fri Sep 04 22:50:26 2009 +0000
@@ -171,7 +171,12 @@
 	}
 }
 
-/* length of crypted buffer must be plain_len + 16*/
+/* length of crypted buffer must be plain_len + 17*/
+/*
+ * The above comment used to say "plain_len + 16", but based on the
+ * behavior of the function that is wrong.  If you give this function
+ * a plain string with len%8 = 7 then the returned length is len+17
+ */
 gint qq_encrypt(guint8* crypted, const guint8* const plain, const gint plain_len, const guint8* const key)
 {
 	guint8 *crypted_ptr = crypted;		/* current position of dest */
--- a/libpurple/protocols/qq/qq_network.c	Fri Sep 04 21:12:11 2009 +0000
+++ b/libpurple/protocols/qq/qq_network.c	Fri Sep 04 22:50:26 2009 +0000
@@ -1146,8 +1146,8 @@
 	qd = (qq_data *)gc->proto_data;
 	g_return_val_if_fail(data != NULL && data_len > 0, -1);
 
-	/* at most 16 bytes more */
-	encrypted = g_newa(guint8, data_len + 16);
+	/* at most 17 bytes more */
+	encrypted = g_newa(guint8, data_len + 17);
 	encrypted_len = qq_encrypt(encrypted, data, data_len, qd->session_key);
 	if (encrypted_len < 16) {
 		purple_debug_error("QQ_ENCRYPT", "Error len %d: [%05d] 0x%04X %s\n",
@@ -1223,8 +1223,8 @@
 		purple_debug_info("QQ", "<== [SRV-%05d] %s(0x%04X), datalen %d\n",
 				seq, qq_get_cmd_desc(cmd), cmd, data_len);
 #endif
-	/* at most 16 bytes more */
-	encrypted = g_newa(guint8, data_len + 16);
+	/* at most 17 bytes more */
+	encrypted = g_newa(guint8, data_len + 17);
 	encrypted_len = qq_encrypt(encrypted, data, data_len, qd->session_key);
 	if (encrypted_len < 16) {
 		purple_debug_error("QQ_ENCRYPT", "Error len %d: [%05d] 0x%04X %s\n",
@@ -1270,8 +1270,8 @@
 	seq = qd->send_seq;
 
 	/* Encrypt to encrypted with session_key */
-	/* at most 16 bytes more */
-	encrypted = g_newa(guint8, buf_len + 16);
+	/* at most 17 bytes more */
+	encrypted = g_newa(guint8, buf_len + 17);
 	encrypted_len = qq_encrypt(encrypted, buf, buf_len, qd->session_key);
 	if (encrypted_len < 16) {
 		purple_debug_error("QQ_ENCRYPT", "Error len %d: [%05d] %s (0x%02X)\n",