Re: [elinks-dev] Errors in bittorent
Witold Filipczyk <[EMAIL PROTECTED]> writes:
> + uri = get_uri(uri_string.source, URI_BASE);
> + done_string(&uri_string);
> + if (!uri) {
> + done_bittorrent_peer_connection(peer);
> + return BITTORRENT_STATE_OUT_OF_MEM;
> + }
> + uri->protocol = PROTOCOL_BITTORRENT;
Changing uri->protocol like that is not safe, I think. get_uri()
may have returned a cached URI to which there are other references.
Because a malicious BitTorrent tracker can return arbitrary
hostnames in the peer list, and this code will then change the
protocol of the corresponding HTTP URIs to BitTorrent and affect
future accesses to those hosts, I think this needs to be fixed.
A pretty clear solution is to add a "bittorrent-peer" URI scheme
for use in these connections, but other fixes are fine too.
The following patch is also available from a Git repository:
git://repo.or.cz/elinks/kon.git 0.12/bittorrent-peer-uri
Fix blacklist crash in BitTorrent
make_bittorrent_peer_connection() used to construct a struct uri on
the stack. This was hacky but worked nicely because the struct uri
was not really accessed after make_connection() returned. However,
since commit a83ff1f565a4a7bc25a4b8353ee26bc1b97410e3, the struct uri
is also needed when the connection is being closed. Valgrind shows:
Invalid read of size 2
at 0x8100764: get_blacklist_entry (blacklist.c:33)
by 0x8100985: del_blacklist_entry (blacklist.c:64)
by 0x80DA579: complete_connect_socket (socket.c:448)
by 0x80DA84A: connected (socket.c:513)
by 0x80D0DDF: select_loop (select.c:297)
by 0x80D00C6: main (main.c:353)
Address 0xBEC3BFAE is just below the stack ptr. To suppress, use:
--workaround-gcc296-bugs=yes
To fix this, allocate the struct uri on the heap instead, by
constructing a string and giving that to get_uri(). This string
cannot use the "bittorrent" URI scheme because parse_uri() does not
recognize the host and port fields in that. (The "bittorrent" scheme
has protocol_backend.free_syntax = 1 in order to support strings like
"bittorrent:http://beta.legaltorrents.com/get/159-noisome-beasts";.)
Instead, define a new "bittorrent-peer" URI scheme for this purpose.
If the user attempts to use this URI scheme, its handler aborts the
connection with an error; but when make_bittorrent_peer_connection()
uses a bittorrent-peer URI, the handler is not called.
This change also lets get_uri() set the ipv6 flag if peer_info->ip is
an IPv6 address literal.
Reported by Witold Filipczyk.
---
commit d93bceb9bd6ab32c614ac20dc5c87e3af2a7f85f
tree a32c808927944db6717398c8595626c958008579
parent 7de8b9940c96f51c55c2706ed9aa6ad11257d7ee
author Kalle Olavi Niemitalo <[EMAIL PROTECTED]> Sun, 07 Sep 2008 06:10:52 +0300
committer Kalle Olavi Niemitalo <[EMAIL PROTECTED]> Sun, 07 Sep 2008 06:31:36
+0300
src/network/state.c |1 +
src/network/state.h |1 +
src/protocol/bittorrent/connection.c |6
src/protocol/bittorrent/connection.h |2 +
src/protocol/bittorrent/peerconnect.c | 45 -
src/protocol/protocol.c |1 +
src/protocol/protocol.h |1 +
7 files changed, 39 insertions(+), 18 deletions(-)
diff --git a/src/network/state.c b/src/network/state.c
index 3a94982..9ecd6f3 100644
--- a/src/network/state.c
+++ b/src/network/state.c
@@ -124,6 +124,7 @@ static const struct s_msg_dsc msg_dsc[] = {
{S_BITTORRENT_METAINFO, N_("The BitTorrent metainfo file contained
errors")},
{S_BITTORRENT_TRACKER, N_("The tracker requesting failed")},
{S_BITTORRENT_BAD_URL, N_("The BitTorrent URL does not point to a
valid URL")},
+ {S_BITTORRENT_PEER_URL, N_("The bittorrent-peer URL scheme is for
internal use only")},
#endif
/* fsp_open_session() failed but did not set errno.
diff --git a/src/network/state.h b/src/network/state.h
index 16faf41..f8358b2 100644
--- a/src/network/state.h
+++ b/src/network/state.h
@@ -103,6 +103,7 @@ enum connection_basic_state {
S_BITTORRENT_METAINFO = -100801,
S_BITTORRENT_TRACKER= -100802,
S_BITTORRENT_BAD_URL= -100803,
+ S_BITTORRENT_PEER_URL = -100804,
S_FSP_OPEN_SESSION_UNKN = -100900,
};
diff --git a/src/protocol/bittorrent/connection.c
b/src/protocol/bittorrent/connection.c
index f15bfe8..132283b 100644
--- a/src/protocol/bittorrent/connection.c
+++ b/src/protocol/bittorrent/connection.c
@@ -414,3 +414,9 @@ bittorrent_protocol_handler(struct connection *conn)
bittorrent_metainfo_callback, conn, 0);
done_uri(uri);
}
+
+void
+bittorrent_peer_protocol_handler(struct connection *conn)
+{
+ abort_connection(conn, connection_state(S_BITTORRENT_PEER_URL));
+}
diff --git a/src/protocol/bittorrent/connection.h
b/src/protocol/bittorrent/connection.h
index af6c89d..13aeca0 100644
--- a/src/protocol/bittorrent/connection.h
+++ b/src/protocol/bittorrent/co
Re: [elinks-dev] Errors in bittorent
On Sat, Aug 30, 2008 at 12:24:40PM +0200, Jonas Fonseca wrote:
> Some comments ...
>
> Witold Filipczyk <[EMAIL PROTECTED]> wrote Sat, Aug 30, 2008:
> > commit b589f19b73c65621c0a8582199509f58dbcac09f
> > Author: Witold Filipczyk <[EMAIL PROTECTED]>
> > AuthorDate: Sat Aug 30 10:52:00 2008 +0200
> > Commit: Witold Filipczyk <[EMAIL PROTECTED]>
> > CommitDate: Sat Aug 30 10:52:00 2008 +0200
> >
> > An issue with bittorrent.
> >
> > It was possible that the reference to the automatic variable
> > uri of the make_bittorrent_peer_connection trashed the stack.
> > In addition done_uri crashed because uri->string was NULL.
> >
> > Now uri is allocated and unlocked to avoid memleak.
>
>
> Instead of fixing this hack, why don't you add a setup_connection() that
> make_connection() can use as a wrapper and be done with this. Or if this
> is "bittorrent" URI thing actually works, why do you mess with the URI struct
> yourself? Didn't parse_uri() do all that for you already? Also don't
> unluck the URI, call done_uri(uri) after the call to make_connection().
It is not so easy, because bittorrent in ELinks uses free syntax.
I'm cheating a bit here and using http instead of bittorrent.
diff --git a/src/protocol/bittorrent/peerconnect.c
b/src/protocol/bittorrent/peerconnect.c
index aeafbf3..7a89523 100644
--- a/src/protocol/bittorrent/peerconnect.c
+++ b/src/protocol/bittorrent/peerconnect.c
@@ -271,9 +271,11 @@ enum bittorrent_state
make_bittorrent_peer_connection(struct bittorrent_connection *bittorrent,
struct bittorrent_peer *peer_info)
{
- struct uri uri;
+ struct string uri_string;
+ struct uri *uri;
struct bittorrent_peer_connection *peer;
- unsigned char port[5];
+ unsigned char port[6];
+ int port_length;
peer = init_bittorrent_peer_connection(-1);
if (!peer) return BITTORRENT_STATE_OUT_OF_MEM;
@@ -296,14 +298,27 @@ make_bittorrent_peer_connection(struct
bittorrent_connection *bittorrent,
/* FIXME: Rather change the make_connection() interface. This is an ugly
* hack. */
/* FIXME: Set the ipv6 flag iff ... */
- memset(&uri, 0, sizeof(uri));
- uri.protocol = PROTOCOL_BITTORRENT;
- uri.host = peer_info->ip;
- uri.hostlen = strlen(peer_info->ip);
- uri.port = port;
- uri.portlen = snprintf(port, sizeof(port), "%u", peer_info->port);
-
- make_connection(peer->socket, &uri, send_bittorrent_peer_handshake, 1);
+ if (!init_string(&uri_string)) {
+ done_bittorrent_peer_connection(peer);
+ return BITTORRENT_STATE_OUT_OF_MEM;
+ }
+ /* The bittorrent protocol uses free syntax.
+* It does not set uri->port nor uri->host */
+ add_to_string(&uri_string, "http://";);
+ add_to_string(&uri_string, peer_info->ip);
+ add_char_to_string(&uri_string, ':');
+ port_length = snprintf(port, sizeof(port), "%u", peer_info->port);
+ add_bytes_to_string(&uri_string, port, port_length);
+
+ uri = get_uri(uri_string.source, URI_BASE);
+ done_string(&uri_string);
+ if (!uri) {
+ done_bittorrent_peer_connection(peer);
+ return BITTORRENT_STATE_OUT_OF_MEM;
+ }
+ uri->protocol = PROTOCOL_BITTORRENT;
+ make_connection(peer->socket, uri, send_bittorrent_peer_handshake, 1);
+ done_uri(uri);
return BITTORRENT_STATE_OK;
}
___
elinks-dev mailing list
elinks-dev@linuxfromscratch.org
http://linuxfromscratch.org/mailman/listinfo/elinks-dev
Re: [elinks-dev] Errors in bittorent
Some comments ...
Witold Filipczyk <[EMAIL PROTECTED]> wrote Sat, Aug 30, 2008:
> commit b589f19b73c65621c0a8582199509f58dbcac09f
> Author: Witold Filipczyk <[EMAIL PROTECTED]>
> AuthorDate: Sat Aug 30 10:52:00 2008 +0200
> Commit: Witold Filipczyk <[EMAIL PROTECTED]>
> CommitDate: Sat Aug 30 10:52:00 2008 +0200
>
> An issue with bittorrent.
>
> It was possible that the reference to the automatic variable
> uri of the make_bittorrent_peer_connection trashed the stack.
> In addition done_uri crashed because uri->string was NULL.
>
> Now uri is allocated and unlocked to avoid memleak.
>
> diff --git a/src/protocol/bittorrent/peerconnect.c
> b/src/protocol/bittorrent/peerconnect.c
> index aeafbf3..5f65e68 100644
> --- a/src/protocol/bittorrent/peerconnect.c
> +++ b/src/protocol/bittorrent/peerconnect.c
> @@ -20,6 +20,7 @@
> #include "elinks.h"
>
> #include "config/options.h"
> +#include "main/object.h"
> #include "main/select.h"
> #include "main/timer.h"
> #include "network/connection.h"
> @@ -271,9 +272,12 @@ enum bittorrent_state
> make_bittorrent_peer_connection(struct bittorrent_connection *bittorrent,
> struct bittorrent_peer *peer_info)
> {
> - struct uri uri;
> + struct string uri_string;
> + struct uri *uri;
> struct bittorrent_peer_connection *peer;
> - unsigned char port[5];
> + unsigned char port[6];
> + int ip_start, port_start;
> + int port_length;
>
> peer = init_bittorrent_peer_connection(-1);
> if (!peer) return BITTORRENT_STATE_OUT_OF_MEM;
> @@ -296,14 +300,32 @@ make_bittorrent_peer_connection(struct
> bittorrent_connection *bittorrent,
> /* FIXME: Rather change the make_connection() interface. This is an ugly
>* hack. */
> /* FIXME: Set the ipv6 flag iff ... */
> - memset(&uri, 0, sizeof(uri));
> - uri.protocol = PROTOCOL_BITTORRENT;
> - uri.host = peer_info->ip;
> - uri.hostlen = strlen(peer_info->ip);
> - uri.port = port;
> - uri.portlen = snprintf(port, sizeof(port), "%u", peer_info->port);
> -
> - make_connection(peer->socket, &uri, send_bittorrent_peer_handshake, 1);
> + if (!init_string(&uri_string)) {
> + done_bittorrent_peer_connection(peer);
> + return BITTORRENT_STATE_OUT_OF_MEM;
> + }
> + add_to_string(&uri_string, "bittorrent:");
> + ip_start = uri_string.length;
> + add_to_string(&uri_string, peer_info->ip);
> + add_char_to_string(&uri_string, ':');
> + port_start = uri_string.length;
> +
> + port_length = snprintf(port, sizeof(port), "%u", peer_info->port);
> + add_bytes_to_string(&uri_string, port, port_length);
> + uri = get_uri(uri_string.source, URI_BASE);
> + done_string(&uri_string);
> + if (!uri) {
> + done_bittorrent_peer_connection(peer);
> + return BITTORRENT_STATE_OUT_OF_MEM;
> + }
> +
> + uri->host = uri->string + ip_start;
> + uri->hostlen = port_start - ip_start - 1;
> + uri->port = uri->string + port_start;
> + uri->portlen = port_length;
> + /* Do not lock it. */
> + object_unlock(uri);
> + make_connection(peer->socket, uri, send_bittorrent_peer_handshake, 1);
>
> return BITTORRENT_STATE_OK;
> }
Instead of fixing this hack, why don't you add a setup_connection() that
make_connection() can use as a wrapper and be done with this. Or if this
is "bittorrent" URI thing actually works, why do you mess with the URI struct
yourself? Didn't parse_uri() do all that for you already? Also don't
unluck the URI, call done_uri(uri) after the call to make_connection().
Else your checking of add_..._string() calls is a bit sloppy.
--
Jonas Fonseca
___
elinks-dev mailing list
elinks-dev@linuxfromscratch.org
http://linuxfromscratch.org/mailman/listinfo/elinks-dev
Re: [elinks-dev] Errors in bittorent
On Fri, Aug 29, 2008 at 10:38:20PM +0200, Witold Filipczyk wrote:
> Hi,
> I noticed a bug in the bittorent protocol code while trying to get an ISO from
> http://torrents.gentoo.org/.
>
> Here is a fix for it.
> 1) Before the uri was put on the stack and the access that uri later
> may trash the stack.
> 2) done_uri expects that uri->string is not NULL, so uri->string points to "".
>
That patch wasn't good. I attached the second one.
Witek
commit b589f19b73c65621c0a8582199509f58dbcac09f
Author: Witold Filipczyk <[EMAIL PROTECTED]>
AuthorDate: Sat Aug 30 10:52:00 2008 +0200
Commit: Witold Filipczyk <[EMAIL PROTECTED]>
CommitDate: Sat Aug 30 10:52:00 2008 +0200
An issue with bittorrent.
It was possible that the reference to the automatic variable
uri of the make_bittorrent_peer_connection trashed the stack.
In addition done_uri crashed because uri->string was NULL.
Now uri is allocated and unlocked to avoid memleak.
diff --git a/src/protocol/bittorrent/peerconnect.c
b/src/protocol/bittorrent/peerconnect.c
index aeafbf3..5f65e68 100644
--- a/src/protocol/bittorrent/peerconnect.c
+++ b/src/protocol/bittorrent/peerconnect.c
@@ -20,6 +20,7 @@
#include "elinks.h"
#include "config/options.h"
+#include "main/object.h"
#include "main/select.h"
#include "main/timer.h"
#include "network/connection.h"
@@ -271,9 +272,12 @@ enum bittorrent_state
make_bittorrent_peer_connection(struct bittorrent_connection *bittorrent,
struct bittorrent_peer *peer_info)
{
- struct uri uri;
+ struct string uri_string;
+ struct uri *uri;
struct bittorrent_peer_connection *peer;
- unsigned char port[5];
+ unsigned char port[6];
+ int ip_start, port_start;
+ int port_length;
peer = init_bittorrent_peer_connection(-1);
if (!peer) return BITTORRENT_STATE_OUT_OF_MEM;
@@ -296,14 +300,32 @@ make_bittorrent_peer_connection(struct
bittorrent_connection *bittorrent,
/* FIXME: Rather change the make_connection() interface. This is an ugly
* hack. */
/* FIXME: Set the ipv6 flag iff ... */
- memset(&uri, 0, sizeof(uri));
- uri.protocol = PROTOCOL_BITTORRENT;
- uri.host = peer_info->ip;
- uri.hostlen = strlen(peer_info->ip);
- uri.port = port;
- uri.portlen = snprintf(port, sizeof(port), "%u", peer_info->port);
-
- make_connection(peer->socket, &uri, send_bittorrent_peer_handshake, 1);
+ if (!init_string(&uri_string)) {
+ done_bittorrent_peer_connection(peer);
+ return BITTORRENT_STATE_OUT_OF_MEM;
+ }
+ add_to_string(&uri_string, "bittorrent:");
+ ip_start = uri_string.length;
+ add_to_string(&uri_string, peer_info->ip);
+ add_char_to_string(&uri_string, ':');
+ port_start = uri_string.length;
+
+ port_length = snprintf(port, sizeof(port), "%u", peer_info->port);
+ add_bytes_to_string(&uri_string, port, port_length);
+ uri = get_uri(uri_string.source, URI_BASE);
+ done_string(&uri_string);
+ if (!uri) {
+ done_bittorrent_peer_connection(peer);
+ return BITTORRENT_STATE_OUT_OF_MEM;
+ }
+
+ uri->host = uri->string + ip_start;
+ uri->hostlen = port_start - ip_start - 1;
+ uri->port = uri->string + port_start;
+ uri->portlen = port_length;
+ /* Do not lock it. */
+ object_unlock(uri);
+ make_connection(peer->socket, uri, send_bittorrent_peer_handshake, 1);
return BITTORRENT_STATE_OK;
}
___
elinks-dev mailing list
elinks-dev@linuxfromscratch.org
http://linuxfromscratch.org/mailman/listinfo/elinks-dev
