URI query string for multiscrapes isn't always properly zero terminated

[Mandor]

I've been noticing that some multiscrape requests appear corrupted, they contain binary garbage that looks like random memory bytes.

After investigating for a bit, the only common factors I found are that 1) those corrupted URLs are always multi-scrape requests and 2) the clients are various versions of Transmission ranging from 0.8 to 2.5.

I'm not ruling out a problem in my server's configuration, but as the only affected client seems to be Transmission that'd be one big coincidence. I attached an nginx error log (only the paths and IPs have been doctored, the rest is "as is") that shows what I'm dealing with here. You'll notice that suhosin kicks in and attempts to sanitize the mangled requests, but that's irrelevant to the actual issue.

[Mandor]

I'm no C expert, but maybe it could a problem somewhere in libtransmission/crypto.c

tr_sha1() seems to fit the bill for "leaking" memory chunks into a hash...

[Mandor]

I parsed today's logs and the problem occured with the following clients:

Transmission/2.51
Transmission/2.50
Transmission/2.42
Transmission/2.41
Transmission/2.33

[gvdl]

tr_sha1() does not write to any memory, and SHA1_Update only reads data with a const void * so it doesn't modify its input data. That only leaves SHA1_Update overwriting the end of the SHA_CTX sha and I'd be amazed if it did, since it is only updating a fixed length hash and not growing data and sha is allocated using SHA_Init and it must surely know how big the hash is.

Anyway tr_sha1() is not to blame, sorry.

[Mandor]

You certainly know better than I do, all I can say for sure is that every version of Transmission since the multiscrape feature was implemented appears to occasionally append binary data to multiscrape requests. It's definitely not an issue on the tracker's side and the binary data seems to be memory chunks from the client.

Search for 'castle' in the log file attached to this ticket- that client was unlucky enough to have some filenames leaked in the multiscrape request and incidentally his windows username ('elephant' apparently). Who knows what else ends up getting sent to trackers- with enough bad luck or someone finding a proper way to exploit this, it could even constitute a security issue.

[x190]

Are you running your own tracker which maybe is mangling its own dictionaries?

[Mandor]

Replying to x190:

Mandor: Aren't these improper responses from some other client? For example, "while reading response header from upstream, client: xx.xx.81.142".

No, you misunderstand; that log is from a tracker's http server, nginx to be specific. upstream is an array of PHP-FCGI worker pools and the clients are various bittorent clients. The error is actually raised by suhosin, a PHP security extension, due to the parameters being malformed (no kidding, there's binary in them). For obvious reasons I trimmed an extract of the logs and 'anonymized' them a bit.

Before writing this ticket, I came up with a small script to 1) extract a list of bogus IPs and 2) match those IPs to user-agents, the only common points I could find are that they are all Transmission clients (2.31+) and that it only happens with multiscrape requests. Regular scrape and announce requests are fine, and the OS varies: various versions of Mac OS X and Linux seem affected.

[Mandor]

Replying to x190:

Are you running your own tracker which maybe is mangling its own dictionaries?

I am, and that was my first bet- some kind of issue with my tracker software. But no, the requests are received by the webserver (nginx) as shown in the sample log attached to the ticket, the issue is definitely client-side and specific to Transmission multiscrape requests. Those requests should only contain a list of info_hash arguments and even announce requests don't contain things such as an absolute file paths anyway so the client is definitely sending stuff it shouldn't somehow, how else would my server come up with that? :)

[x190]

Thanks Mandor. This issue concerns me a great deal. If you believe Transmission really has the potential to send out unauthorized data, should you not raise the priority and severity of this ticket to the highest levels?

[x190]

Mandor: Please see the following for suhosin settings that may need to be raised.

​http://www.hardened-php.net/suhosin/configuration.html#suhosin.get.max_vars

suhosin.get.max_totalname_length and suhosin.get.max_value_length look like likely candidates to raise. Also, possibly suhosin.get.max_name_length.

You will need a value of 64*36 = 2304 bytes plus the variable name, etc. So try maybe 4096 bytes minimum.

[Mandor]

While I believe the issue is real and potentially serious, it's not my place to raise priority/severity.
I'll let the Transmission devs be the judge of that. :)

[Mandor]

Replying to x190:

Mandor: Please see the following for suhosin settings that may need to be raised.

Thank you for the suggestion, but in this case suhosin is doing exactly what it is supposed to:
Dropping malformed variables with binary in them...

[x190]

Replying to Mandor:

Replying to x190:

Mandor: Please see the following for suhosin settings that may need to be raised.

Thank you for the suggestion, but in this case suhosin is doing exactly what it is supposed to:
Dropping malformed variables with binary in them...

You haven't even carefully read your own logs yet alone done any proper research. I was trying to be helpful. Your own logs indicate variables are being dropped because they are too long for its (suhosin) incorrect settings, not malformed. I suspect suhosin then leaks local memory into your logs.

I am not a developer, but I personally consider this ticket to be invalid.

[Mandor]

Replying to x190:

Replying to Mandor:

Replying to x190:

Mandor: Please see the following for suhosin settings that may need to be raised.

Thank you for the suggestion, but in this case suhosin is doing exactly what it is supposed to:
Dropping malformed variables with binary in them...

You haven't even carefully read your own logs yet alone done any proper research. I was trying to be helpful. Your own logs indicate variables are being dropped because they are too long for its incorrect settings, not malformed. I suspect suhosin then leaks local memory into your logs.

I am not a developer, but I personally consider this ticket to be invalid.

The variables end up being too long because Transmission leaks memory in them so changing the suhosin configuration wouldn't accomplish anything besides making it ignore the faulty requests.
Why try to mask the issue?

As for the problem being server-side, I've considered it and poked around but it's highly unlikely because 1) the multiscrape requests sent by EVERY other bittorent client work just fine and 2) the leaked binary sometimes contains information that is definitely client-related (see above with the absolute file paths and CRL URLs, where would that come from if not from the client?).

Please believe that I HAVE read my logs carefully and done my research before writing this ticket. However what you are proposing is to adjust security settings to accomodate for a misbehaving client, which serves no functional purpose on top of being potentially unsafe.

Finally I'd ask you to tone down the unpleasantness as it has no place in a bug report to begin with and reflects badly on the project.

[x190]

#4501 deals with trackers that are not properly configured to handle multiscrapes.

[cfpp2p]

I'm attempting to decipher what is going on here. I checked the submitted nginx error log and I can only find three lines with a Transmission Peer-Id https://trac.transmissionbt.com/wiki/PeerId

2012/05/05 21:09:01 [error] 8948#0: *1112637 FastCGI sent in stderr: "ALERT - configured request variable name length limit exceeded - dropped variable '.C.Zs..*.-TR2420-6c5kuyxk2675..RDh...,Q...U...QI>....d..?.....l..:.ih' (attacker 'xx.xx.0.8', file '/home/mywebsite/scrape.php')" while reading response header from upstream, client: xx.xx.0.8, server: mywebsite.com, request: "GET /d/4034/scrape?info_hash=%15%ba%b8%d0W%9c%fb%dd%eeR%a3%cb%c4%02%02%2f%b3%b0%fbP&info_hash=%5c%d0%5d%14%c9%a6%ab%ecF%15%26%9aC%a2Zs%cb%05%2a%b9&info_hash=c%ed%db%01%fe%ceO%1fg%03%14%0e%5c%88%93%8b%f1%81%06%16&info_hash=%20%f4%12%f5%a9%2f%ca%27%a8~pn%b5r%c3%bdtl%15%b1&info_hash=%fb%82%b1%b5%1fn%fa%0e%c5%c0%c8%b7%dejY%dc.%dc%e2%e9&info_hash=%11U%fc%3aJ-%f4%98l%dc9%2b%17%ab%a7cP%a67%ea&info_hash=%abs%e1%21%9d%e4%2bs%e4%87t%faJGR%5d%d3F%07%1f&info_hash=%b8%0a%7b%3eY%a2%90%11A6%f3%9b%fc%18%beJ%26W6%84&info_hash=%beZ%d6%d9o%02%c0%7f%a3%97%f0%fbv%a8%21~%9cd%29%05&info_hash=%dcKp%a6%bf%40m%86%bfs%97l0h3%a9%98%e0%3b%04&šC¢ZsË*¹-TR2420-6c5kuyxk2675ÜüRDh¥Í,Q”U–¥ûQI>±´¶Ýd	â?˪älÑ:êih=Ç ê×EÀý•´ðal.comodoca.com/UTN-USERFirst-Hardware.crl06 4 2†0http://crl.comodo.net/UTN-USERFirst-Hardware.crl0†+ z0x0;+ 0†/http://crt.comodoca.com/UTNAddTrustServerCA.crt09+ 0†-http://crt.comodo.net/UTNAddTrustServerCA.crt HTTP/1.1", upstream: "fastcgi://unix:/var/run/php/php3.sock:", host: "mywebsite.com"
2012/05/05 21:39:11 [error] 8949#0: *1251003 FastCGI sent in stderr: "ALERT - configured request variable name length limit exceeded - dropped variable '.C.Zs..*.-TR2420-6c5kuyxk2675..RDh...,Q...U...QI>....d..?.....l..:.ih' (attacker 'xx.xx.0.8', file '/home/mywebsite/scrape.php')" while reading response header from upstream, client: xx.xx.0.8, server: mywebsite.com, request: "GET /d/4034/scrape?info_hash=%15%ba%b8%d0W%9c%fb%dd%eeR%a3%cb%c4%02%02%2f%b3%b0%fbP&info_hash=%5c%d0%5d%14%c9%a6%ab%ecF%15%26%9aC%a2Zs%cb%05%2a%b9&info_hash=c%ed%db%01%fe%ceO%1fg%03%14%0e%5c%88%93%8b%f1%81%06%16&info_hash=%20%f4%12%f5%a9%2f%ca%27%a8~pn%b5r%c3%bdtl%15%b1&info_hash=%fb%82%b1%b5%1fn%fa%0e%c5%c0%c8%b7%dejY%dc.%dc%e2%e9&info_hash=%11U%fc%3aJ-%f4%98l%dc9%2b%17%ab%a7cP%a67%ea&info_hash=%abs%e1%21%9d%e4%2bs%e4%87t%faJGR%5d%d3F%07%1f&info_hash=%b8%0a%7b%3eY%a2%90%11A6%f3%9b%fc%18%beJ%26W6%84&info_hash=%beZ%d6%d9o%02%c0%7f%a3%97%f0%fbv%a8%21~%9cd%29%05&info_hash=%dcKp%a6%bf%40m%86%bfs%97l0h3%a9%98%e0%3b%04&šC¢ZsË*¹-TR2420-6c5kuyxk2675ÜüRDh¥Í,Q”U–¥ûQI>±´¶Ýd	â?˪älÑ:êih=Ç ê×EÀý•´ðal.comodoca.com/UTN-USERFirst-Hardware.crl06 4 2†0http://crl.comodo.net/UTN-USERFirst-Hardware.crl0†+ z0x0;+ 0†/http://crt.comodoca.com/UTNAddTrustServerCA.crt09+ 0†-http://crt.comodo.net/UTNAddTrustServerCA.crt HTTP/1.1", upstream: "fastcgi://unix:/var/run/php.sock:", host: "mywebsite.com"
2012/05/05 22:09:21 [error] 8949#0: *1388110 FastCGI sent in stderr: "ALERT - configured request variable name length limit exceeded - dropped variable '.C.Zs..*.-TR2420-6c5kuyxk2675..RDh...,Q...U...QI>....d..?.....l..:.ih' (attacker 'xx.xx.0.8', file '/home/mywebsite/scrape.php')" while reading response header from upstream, client: xx.xx.0.8, server: mywebsite.com, request: "GET /d/4034/scrape?info_hash=%15%ba%b8%d0W%9c%fb%dd%eeR%a3%cb%c4%02%02%2f%b3%b0%fbP&info_hash=%5c%d0%5d%14%c9%a6%ab%ecF%15%26%9aC%a2Zs%cb%05%2a%b9&info_hash=c%ed%db%01%fe%ceO%1fg%03%14%0e%5c%88%93%8b%f1%81%06%16&info_hash=%20%f4%12%f5%a9%2f%ca%27%a8~pn%b5r%c3%bdtl%15%b1&info_hash=%fb%82%b1%b5%1fn%fa%0e%c5%c0%c8%b7%dejY%dc.%dc%e2%e9&info_hash=%11U%fc%3aJ-%f4%98l%dc9%2b%17%ab%a7cP%a67%ea&info_hash=%abs%e1%21%9d%e4%2bs%e4%87t%faJGR%5d%d3F%07%1f&info_hash=%b8%0a%7b%3eY%a2%90%11A6%f3%9b%fc%18%beJ%26W6%84&info_hash=%beZ%d6%d9o%02%c0%7f%a3%97%f0%fbv%a8%21~%9cd%29%05&info_hash=%dcKp%a6%bf%40m%86%bfs%97l0h3%a9%98%e0%3b%04&šC¢ZsË*¹-TR2420-6c5kuyxk2675ÜüRDh¥Í,Q”U–¥ûQI>±´¶Ýd	â?˪älÑ:êih=Ç ê×EÀý•´ðal.comodoca.com/UTN-USERFirst-Hardware.crl06 4 2†0http://crl.comodo.net/UTN-USERFirst-Hardware.crl0†+ z0x0;+ 0†/http://crt.comodoca.com/UTNAddTrustServerCA.crt09+ 0†-http://crt.comodo.net/UTNAddTrustServerCA.crt HTTP/1.1", upstream: "fastcgi://unix:/var/run/php.sock:", host: "mywebsite.com"

I am unclear as to what the other 39 lines user Peer-Id or User-Agent are. How were these 39 lines Peer-Id or User-Agent determined?

[rb07]

Mandor :

It would help if you can show the traffic from Transmission's point of view: Define TR_CURL_VERBOSE in the environment before running your application (daemon or different), and watch the log.

Another good option could be to see the actual traffic with another tool, for instance "tcpflow -ce host <tracker> and port <tracker-port>".

I'm trying to find a tracker that uses multiscrape, but this will take some time (the 30 min frequency of normal scrapes).

[Mandor]

Replying to cfpp2p:

I am unclear as to what the other 39 lines user Peer-Id or User-Agent are. How were these 39 lines Peer-Id or User-Agent determined?

I determined that those requests were all sent by Transmission clients by cross-referencing the IPs in the error log with the access log which conveniently contains the full user-agent HTTP header for each request. I've actually been doing this cross-referencing regularly over the last week to be sure and only Transmission 2.3+ user-agents ever came up. If you won't just take my word for it, maybe I could get you a sample of matching error/access nginx logs but besides proving I'm not making stuff up I don't think it'd add anything. :)

Also note that as those requests are scrapes, they should never contain any peer_id anyway, only a list of url-encoded info_hash. The three you spotted are merely coincidence, they just happened to be what was randomly leaked in the URL that time.

If I knew my way around a C codebase, I'd go look around the code that constructs a multiscrape request, I'd bet that the issue lies in there somewhere. If you need to reproduce the issue you can probably do it easily enough by loading up 30-ish torrents from the same tracker (ideally an HTTPS-only one in case that matters), sit on them for a while and sniff multiscrape requests (or watch your webserver logs if you have your own tracker).

[x190]

Replying to Mandor:

If you need to reproduce the issue you can probably do it easily enough by loading up 30-ish torrents from the same tracker

The multiscrape bep allows 74 and Transmission uses 64. I also noted the 4 entries in your log for xx.xx.21.194 contain nothing but perfectly formed info_hashes, yet it still gets cut-off at around 1711 characters. Why? Well, according to your logs it is because "configured GET variable value length limit exceeded".

[Mandor]

Replying to rb07:

It would help if you can show the traffic from Transmission's point of view: Define TR_CURL_VERBOSE in the environment before running your application (daemon or different), and watch the log.

I'll ask the users, see if some are willing to do this.

I'm trying to find a tracker that uses multiscrape, but this will take some time (the 30 min frequency of normal scrapes).

The tracker doesn't have to actually support multiscrape, Transmission assumes it does anyway (#4501)

[Mandor]

Replying to x190:

Replying to Mandor:

If you need to reproduce the issue you can probably do it easily enough by loading up 30-ish torrents from the same tracker

The multiscrape bep allows 74 and Transmission uses 64.

My point was that from what I've seen the bug is triggered by Transmission users who are connected to anything from a dozen to hundreds of torrents so 30 is a good number as any to reproduce.

Replying to x190:

I also noted the 4 entries in your log for xx.xx.21.194 contain nothing but perfectly formed info_hashes, yet it still gets cut-off at around 1711 characters.

I've seen those too. They correspond to requests that are so long nginx cuts the log line off but a faulty info_hash parameter is there regardless, only later on the line which, granted, may make it look confusing.

Replying to x190:

Why? Well, according to your logs it is because "configured GET variable value length limit exceeded"

You seem to misunderstand the concrete meaning of those suhosin error messages:

• "configured GET variable value length limit exceeded":

This means that the length of a single parameter's value exceeded the limit - 512 by default.
Since a info_hash's value is 20 bytes url-encoded, that should never happen.

• "configured request variable name length limit exceeded":

This means that the length of a single parameter's name exceeded the limit - 64 by default.
Since the ONLY parameter name in a multiscrape request is info_hash, this should never happen either.

The reason both errors are observed is because it depends where in the request Transmission leaks binary. If it's between = and the next & or EOL, the first rule is triggered. Between ? or & and the next =, the second one. The first one is more common because it's statistically more likely since values are longer than names.

There is actually no rule for a maximum total request length, which is what you seem to think the above is, only a maximum total number of parameters and that's set to 100 by default - more than enough as you yourself underlined when mentioning Transmission uses 64 maximum.

Anyway why do you keep trying to find fault with the logs or the server? I'll state it again: out of a wide variety of different bittorent clients and about 50 scrape requests per second on average, ONLY Transmission multiscrape requests error out with fishy binary. I doesn't take a genius to put one and one together so maybe time would be better spent finally looking into the Transmission codebase?

I'd gladly do it myself and provide a patch if I had the technical knowhow but unfortunately I don't.
I'm however getting a bit tired of having to "sell" this issue, I really shouldn't have to.

I don't want to be an ass about it and I've been trying very hard to be helpful, but the truth is this issue is nothing short of a serious privacy liability: what's telling me that if Transmission is leaking things like local file paths and random peer IDs to my tracker, it's not also leaking say my tracker's URL to other random trackers on the internet? I'm going to ban Transmission from version 2.3 to 2.5 for that reason regardless of what happens next, but my hope in reporting the bug here was that there would be another version or a patch out there before I do so that users could just upgrade instead of having to get a different client. It doesn't really concern me personnally either way as I'm a uTorrent user, the multiscrapes of which, incidentally, are all consistently just fine.

Bottom line, I'll stick around to convey any new relevant technical details and provide logs from Transmission's point of vue as requested by rb07 if I can get my hands on helpful users willing to do it but I won't be debating the validity of the ticket anymore- I've been at it for a week now, at this point it's up to the powers that be to decide whether they want to actually look into it or not.

[rb07]

I think you guys stumbled into a bug, I don't know if it is the bug reported, but it looks like a bug.

From libtransmission/web.c :

    /* announce and scrape requests have tiny payloads. */
    if( isScrape || isAnnounce )
    {
        const int sndbuf = 1024;
        const int rcvbuf = isScrape ? 2048 : 3072;
        setsockopt( fd, SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf) );
        setsockopt( fd, SOL_SOCKET, SO_RCVBUF, &rcvbuf, sizeof(rcvbuf) );
    }

The size of hash codes on 64 torrents alone is over that (sndbuf) buffer size.

I don't know if this can be reproduced with 30 (or any number of) torrents on the same tracker, but now that we have that information it should be easier to track down.

Of course there are more details: these are http or https requests, not udp.

[Mandor]

Replying to rb07:

I don't know if this can be reproduced with 30 (or any number of) torrents on the same tracker

The issue seems frequent enough, it should be possible to reproduce it consistently.

Replying to rb07:

Of course there are more details: these are http or https requests, not udp.

In my case, the tracker is HTTPS only.

[x190]

Replying to rb07:

I think you guys stumbled into a bug, I don't know if it is the bug reported, but it looks like a bug.

From libtransmission/web.c :

    /* announce and scrape requests have tiny payloads. */
    if( isScrape || isAnnounce )
    {
        const int sndbuf = 1024;
        const int rcvbuf = isScrape ? 2048 : 3072;
        setsockopt( fd, SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf) );
        setsockopt( fd, SOL_SOCKET, SO_RCVBUF, &rcvbuf, sizeof(rcvbuf) );
    }

The size of hash codes on 64 torrents alone is over that (sndbuf) buffer size.

I don't know if this can be reproduced with 30 (or any number of) torrents on the same tracker, but now that we have that information it should be easier to track down.

Of course there are more details: these are http or https requests, not udp.

Aren't socket buffers set up as an efficiency item rather than anything that could cause a critical error? On OS X, they are mainly controlled by system defaults anyway.

[rb07]

Replying to x190:

Aren't socket buffers set up as an efficiency item rather than anything that could cause a critical error? On OS X, they are mainly controlled by system defaults anyway.

You are misunderstanding the code, it has nothing to do with system defaults, if you look at the top of the file (around line 36 I think), the use of this code depends on the version of libcurl, so in one sense yes, that code section is used for efficiency, but you can't control its use, other than reverting to a very old libcurl or some such non-sense.

The point is simple: that buffer size looks too small for the use case of multi-scraping more than X torrents. I don't know the value of X, but as you said (and I haven't checked) Transmission has an upper limit of 64, which wouldn't work with that size. How little can X be and multi-scrape still work? I don't know.

[gvdl]

Replying to rb07:

Replying to x190:

Aren't socket buffers set up as an efficiency item rather than anything that could cause a critical error? On OS X, they are mainly controlled by system defaults anyway.

You are misunderstanding the code, it has nothing to do with system defaults, if you look at the top of the file (around line 36 I think), the use of this code depends on the version of libcurl, so in one sense yes, that code section is used for efficiency, but you can't control its use, other than reverting to a very old libcurl or some such non-sense.

The point is simple: that buffer size looks too small for the use case of multi-scraping more than X torrents. I don't know the value of X, but as you said (and I haven't checked) Transmission has an upper limit of 64, which wouldn't work with that size. How little can X be and multi-scrape still work? I don't know.

I'm afraid x190 is right, from setsockopt(2):-

SO_SNDBUF and SO_RCVBUF are options to adjust the normal buffer sizes allocated for output and input buffers, respectively. The buffer size may be increased for high-volume connections, or may be decreased to limit the possible backlog of incoming data. The system places an abso- lute limit on these values.

setsockopt() is a system call, the kernel does not truncate or otherwise modify data that it sends or receives rather these options have to do with the throughput optimisation within the TCP stack's buffer pools. I haven't got a copy of Stephens to hand, but I'm reasonably sure that this is a red herring too.

[gvdl]

Ok, I've tracked down the SO_{SND,RCV}BUF reference in Stephens. It is only used for throughput control. Here is the quote

From Stephen's "UNIX Network Programing" Vol 1, Third Edition Pg 207

SO_RCVBUF and SO_SNDBUF socket options

The receive buffers are used by TCP and USB to hold received data until it is read by the application. With TCP, the available room in the socket receive buffer limited the window that TCP can advertise to the other end. The TCP socket receive buffer cannot overflow because the peer is not allowed so send data beyond the advertised window. This is TCP's flow control, and if the peer ignores the advertised window and sends data beyond the window, the receiving TCP discards it. With UDP, however, when a datagram arrives that will not fit in the socket receive buffer, that datagram is discarded.

These buffers are usually much larger than what is being requested here, often around 64KiB for send and 40KiB for receive.

[x190]

ALERT - configured request variable total name length limit exceeded

suhosin.request.max_totalname_length

*

Type: Integer

*

Default: 256

Defines the maximum length of variable names for variables registered through the COOKIE, the URL or through a POST request. This is the complete name string, including all indicies. This setting is also an upper limit for the separate GET, POST, COOKIE configuration directives.

64 * info_hash = 576

​http://www.hardened-php.net/suhosin/configuration.html#suhosin.get.max_name_length

Other clients may be using lower multiscrape limits and likely have also dealt with #4501 as well.

[x190]

For those interested, "static void multiscrape" can be found at Line 1386 in announcer.c. transmission.h defines SHA_DIGEST_LENGTH @ Line 71 and TR_MULTISCRAPE_MAX is enumed @ Line 34 in announcer-common.h.

[Mandor]

Replying to x190

1) That rule, like every other except max_vars, applies individually to each parameter,

2) Its purpose is to impose a limit on the total name length of nested array parameters which is what the "all indicies" part of the doc refers to, except it's irrelevant here- there aren't any in a multiscrape,

3) The error occured because instead of only busting get.max_name_length (64), the Transmission leak happened to be so large it also busted request.max_totalname_length (256) hence different error messages,

4) The reason errors sometimes concern a parameter's name and not its value is because the leak itself, while always starting in a info_hash's value, can contain the "&" character thereby unwittingly ending the previous argument and declaring a new bogus one, hence again different error messages,

I'll state it one last time: suhosin is only revealing the bug, not causing one. And while several different error messages can be observed in practice depending on what the actual content of the binary garbage in the multiscrapes happens to be, the problem itself is always the same: there's binary garbage in the multiscrapes!

It seems to me that the bug should most likely lie somewhere around where a info_hash's value is generated (crypto.c:tr_sha1()), URL encoded (web.c:tr_http_escape_sha1()) or concatenated into an URL (announcer-http.c:tr_tracker_http_scrape()) but I don't know enough about the subtleties of C to locate it precisely.

Also I can tell that announcer-http.c:tr_tracker_http_scrape() uses evbuffer from libevent to build the multiscrape request so it might also be possible that it's where the bug is.
Are there any known issues with certain versions of that library maybe?

[jordan]

Mandor, thanks for reporting this and for providing so much useful information. :)

If this affects versions of Transmission earlier than 2.30 as described in the original description, please also attach a log with multiscrape from an earlier version for me to look at. I suspect the problem was introduced in 2.30, which had an announcer rewrite to support UDP trackers and multiscrape.

gvdl is almost certainly right about SO_SNDBUF and SO_RCVBUF being red herrings. Even if they were hard limits rather than guidelines, one would expect to see cutoff id hashes instead of the output in this log, which looks more like random memory at the end of the string as if the URI's query string wasn't being zero terminated.

Thinking out loud:

So, that scrape URL in T >= 2.30 is generated in scrape_url_new() which uses evbuffer_add_printf(), which is implemented with vsnprintf() and so should be zero-terminating every pass through that loop.

Also I don't see any pattern in how many info_hashes we get through in the multiscrape before garbage shows up. That makes me suspect that maybe tr_http_escape_sha1() isn't zero terminating, but it is.... hm.

Mandor, can you confirm that these scrapes are coming in through a traditional HTTP scrape and aren't UDP scrapes that the tracker munges into HTTP form form to reuse the its internal HTTP scrape parsing code? I'm wondering if we need to consider transmission's UDP multiscrape code here as well.

[Mandor]

Replying to jordan:

If this affects versions of Transmission earlier than 2.30 as described in the original description,

No it actually doesn't, I have been hasty in my original description.
See #4 for the exact list of confirmed affected versions :)

Replying to jordan:

I suspect the problem was introduced in 2.30, which had an announcer rewrite to support UDP trackers and multiscrape.

Most definitely. I haven't actually seen any 2.30 or 2.31 user-agents show up the logs but I suspect it's only because no one is using those. However I can confirm that the bug definitely affects everything from 2.33 to 2.51 consistently though.

Replying to jordan:

Mandor, can you confirm that these scrapes are coming in through a traditional HTTP scrape and aren't UDP scrapes that the tracker munges into HTTP form form to reuse the its internal HTTP scrape parsing code? I'm wondering if we need to consider transmission's UDP multiscrape code here as well.

I can confirm nginx's only listening to HTTPS on port 443 so I'm quite sure they couldn't be UDP scrapes. Nothing on the server's even listening to UDP except for ntpd.

[Mandor]

I had someone who knows C look the ticket over, here's what he tells me:

The evbuffer stores the size in a struct member so it doesnt need null-termination but then after that pullup it's just thrown in a normal char *c string while strdup expects null-termination.

His proposed solution would be to add evbuffer_add_printf(buf, "%c", 0, str) just before scrape_url_new() returns (line 448).

[cfpp2p]

I'm not sure why I don't see something like:

evbuffer_expand( buf, scrapeUrlNewCharMaxSize );

or similar within scrape_url_new()

Without any

evbuffer_expand(struct evbuffer * buf,
		size_t datlen	 
                )

I don't see where the memory for struct evbuffer * buf is allocated. At this moment and without some further explanation a missing evbuffer_expand() looks wrong to me.

[jordan]

Replying to Mandor:

The evbuffer stores the size in a struct member so it doesnt need null-termination but then after that pullup it's just thrown in a normal char *c string while strdup expects null-termination.

His proposed solution would be to add evbuffer_add_printf(buf, "%c", 0, str) just before scrape_url_new() returns (line 448).

Well, evbuffers do double duty. It's true that they sometimes aren't zero terminated if you use them as an array, but if you use it as a string buffer via the evbuffer printf() functions, it is zero terminated by vsnprintf(). So we should have a zero termination after the pullup.

Forcing a zero termination after the pullup might fix the symptom but it would be better to figure out the real source of this bug.

Replying to cfpp2p:

I'm not sure why I don't see something like evbuffer_expand or similar within scrape_url_new()

It's done automatically by libevent in the evbuffer_add_printf() call. Specifically, evbuffer_add_printf() delegates to evbuffer_add_vprintf(), which calls evbuffer_expand_singlechain().

[jordan]

I think I see where the bug is coming from. evbuffer has a private field total_len which is used for knowing how long the entire buffer is. evbuffer_add_printf() increments this by the return value of vsnprintf(), which returns the the number of characters printed excluding the null byte.

So the buffer is zero-terminated after each call to evbuffer_add_printf(), but then evbuffer_pullup() may actually lose the zero termination by not copying that byte because it's not accounted for in evbuffer.total_len.

[x190]

FWIW, announce_url_new() [Line 54 in announcer-http.c] does specifically call evbuffer_expand().

[jordan]

Replying to x190:

FWIW, announce_url_new() [Line 54 in announcer-http.c] does specifically call evbuffer_expand().

Yes. Calling evbuffer_expand() in announce_url_new() is a blind guess of how much space will be needed to build the announce string. It's there to avoid excess mallocs/memcpys/frees in most cases. In the context of this bug it probably helps a little by ensuring that pullup() is a noop for announce strings shorter than 1024 characters, but longer strings will still have the potential error.

[jordan]

Okay, I've landed r13300 which avoids using evbuffer_add_printf() and evbuffer_pullup() together. There are several C* programmers already commenting in this thread, could one (or more) of you review this change?

There are several places where libtransmission uses evbuffer_add_printf() or evbuffer_pullup() but not in conjunction with each other, which is the trigger as described in comment:40.

[gvdl]

Review comments:

util.c: tr_deepLog() const char *str; appears to be unused
announcer-http.c: No problem
announcer.c: No problem
peer-msgs.c: No problem

[jordan]

Thanks gvdl :)

[jordan]

(unused-variable warning fixed in r13301)

[jordan]

Mandor, now that the problem's been fixed (probably ;), I'm going to delete that log attachment to preserve the user's privacy. Thanks again for providing so much useful information to help track down this issue.

[Mandor]

Replying to jordan:

Mandor, now that the problem's been fixed (probably ;), I'm going to delete that log attachment to preserve the user's privacy. Thanks again for providing so much useful information to help track down this issue.

I had already trimmed anything sensitive from it, but by all means go ahead. :)

The ​latest nightly includes the bugfix, right? I may have users upgrade to it, is it stable enough?
Also what is the user-agent like with that nightly? Transmission/2.51+ right?

[jordan]

Yes. Also, the peer-id is -TR251Z-

[Mandor]

Thanks. I'll let you know if it didn't do the trick. Good work! :)

[Mandor]

I have a handful (6 or so) users who've been running r13301 for a while-
No more wonky multiscrapes, it seems the bug is officially fixed. :)

Great job jordan!

[jordan]

Mandor, you're welcome. This is an important bug and, again, I'm grateful for you reporting it here. I would have gotten to this sooner but I've been traveling for work and wasn't following trac.

[x190]

@Mandor: I too, thank you for 'hanging in there' despite my repeated dumb questions and comments.

@Jordan: Will you be reporting this problem to the libevent folks so that others do not get tripped-up by this issue?

[x190]

evbuffer_add_printf() calls evbuffer_add_vprintf() which calls evutil_vsnprintf() which calls vsnprintf(). evutil_vsnprintf() returns only the character count but does append the '\0'. evbuffer_add_vprintf() increments buf->total_len by the return value of evutil_vsnprintf() which is the character count only. Using evbuffer_pullup( buf, -1 ) then sets size to buf->total_len (character count only) which does not include the '\0' termination byte.

Should libevent not include a warning in its documentation for evbuffer_add_printf() to the effect that calling evbuffer_pullup() could result in a non '\0' terminated string unless evbuffer_expand() is called first and given a sufficiently high value?

[cfpp2p]

for the record:

tested v2.51 with only announcer-http.c scrape_url_new() patched to:

     char delimiter;
     struct evbuffer * buf = evbuffer_new( );
 
+    evbuffer_expand( buf, 8192 );
+
     evbuffer_add_printf( buf, "%s", req->url );
     delimiter = strchr( req->url, '?' ) ? '&' : '?';

max size of multisrape request can be calculated and 8192 determined to be sufficiently large for datlen value without fail.

evbuffer_expand(struct evbuffer * buf,
		size_t datlen	 
                )

test environment of 11 transmission clients v2.51 patched as above. Each and every transmission client => 100+ torrents => private-tracker each and every torrent. Private-tracker server's log analysis after 60+ hours and thousands of scrape requests sent ==> zero corrupted/garbage/binary/wonky multiscrape requests sent.

same environment without above patch yielded multiple corrupted multiscrape requests sent after just two hours.