pidgin: 65195a42: The output buffer passed to qq_encrypt n...

markdoliner at pidgin.im markdoliner at pidgin.im
Fri Sep 4 18:57:00 EDT 2009


-----------------------------------------------------------------
Revision: 65195a427a4340ca3f017d8d052c0b85c1f482f2
Ancestor: 62fae74c076cf02f9c8fab63b3da8c5cff13bf3d
Author: markdoliner at pidgin.im
Date: 2009-09-04T22:50:26
Branch: im.pidgin.pidgin
URL: http://d.pidgin.im/viewmtn/revision/info/65195a427a4340ca3f017d8d052c0b85c1f482f2

Modified 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

ChangeLog: 

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.

-------------- next part --------------
============================================================
--- libpurple/protocols/qq/file_trans.c	fca62851a2a3c22e64c44f18723b44372680f226
+++ libpurple/protocols/qq/file_trans.c	1d0109c8fab3bc30ff2168f73d1a2603b9bacb11
@@ -334,7 +334,7 @@ void qq_send_file_ctl_packet(PurpleConne
 		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 */
 
============================================================
--- libpurple/protocols/qq/qq_base.c	9c785f33f0dac7ee34edb881e722a26dc9cce481
+++ libpurple/protocols/qq/qq_base.c	0a423713841bc1492cff2b78864861925eb0afad
@@ -245,10 +245,10 @@ void qq_request_login(PurpleConnection *
 
 	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 @@ void qq_request_get_server(PurpleConnect
 	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 @@ void qq_request_token_ex(PurpleConnectio
 
 	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 @@ void qq_request_token_ex_next(PurpleConn
 
 	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 @@ static void request_token_ex_code(Purple
 	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 @@ void qq_request_check_pwd(PurpleConnecti
 
 	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 @@ void qq_request_login_2007(PurpleConnect
 
 	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 @@ void qq_request_login_2008(PurpleConnect
 
 	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;
============================================================
--- libpurple/protocols/qq/qq_crypt.c	9b27461eca274cecb798230c5195850fe677fbbd
+++ libpurple/protocols/qq/qq_crypt.c	3425e7f0e445c61e986da29f1bcf85ec6efbbd30
@@ -171,7 +171,12 @@ static inline void encrypt_out(guint8 *c
 	}
 }
 
-/* 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 */
============================================================
--- libpurple/protocols/qq/qq_network.c	105c215f6e274425d41428930520b4da1c08af9c
+++ libpurple/protocols/qq/qq_network.c	c71d6be108ba219163dfb3adb3eb2fa492c174f1
@@ -1146,8 +1146,8 @@ static gint send_cmd_detail(PurpleConnec
 	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 @@ gint qq_send_server_reply(PurpleConnecti
 		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 @@ static gint send_room_cmd(PurpleConnecti
 	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",


More information about the Commits mailing list