Opened 6 years ago

Last modified 6 years ago

#5736 new Bug

overflow when piece size exceeds 4GB

Reported by: cfpp2p Owned by:
Priority: Normal Milestone: None Set
Component: Transmission Version: 2.84
Severity: Normal Keywords:
Cc:

Description

A malicious metafile can be created that will cause an overflow of the piece size.

The problem arises when the type conversion of i ( int64_t i ) is downcast to pieceSize ( uint32_t pieceSize )

metainfo.c line 533 inf->pieceSize = i;

The result for integers greater than 4gb is that piece size will be set to zero for many values of i. Then transmission will crash at line 560.

if ((uint64_t) inf->pieceCount != (inf->totalSize + inf->pieceSize - 1) / inf->pieceSize)

Otherwise transmission will continue on and proceed with the improperly overflowed piece size following the rules of ticket:5734#comment:3

Testing confirms this.

Something like this stops it.

line 531

if (!tr_variantDictFindInt (infoDict, TR_KEY_piece_length, &i)
    || (i < 1) || ( i >= 4294967296))

Change History (4)

comment:1 Changed 6 years ago by pathetic_loser

Hmm, it sounds like trouble on the way, making it possible to crash remote clients? E.g. by tricking into DLing rogue magnet link, etc?

comment:2 Changed 6 years ago by cfpp2p

There can be more overflows when piece size exceeds 229

( 536,870,912 )

reference: ticket:4005#comment:29

As suitessay stated:

In torrentInitFromInfo(), assigning 65536 to the 16-bit blockCountInPiece field is overflowing to 0. tr_torBlockPiece() then tries to divide by it.

torrentInitFromInfo()

  tor->blockCountInPiece = tor->blockSize
    ? info->pieceSize / tor->blockSize
    : 0;

but it's not a magnet in this case so a zero result won't work and divide by zero results.

tr_torBlockPiece (const tr_torrent * tor, const tr_block_index_t block)
{
    return block / tor->blockCountInPiece;
}

The minimum a block size can be ( determined by tr_getBlockSize() ) for piece sizes greater than 16384 ( MAX_BLOCK_SIZE ) is block size 8193.

So lets limit piece size to less than 8192 X 65536 = 536870912 which limits

pieceSize / blockSize to 65535 or less.

metainfo.c line 531

if (!tr_variantDictFindInt (infoDict, TR_KEY_piece_length, &i)
    || (i < 1) || ( i >= 536870912))

This all works as far as I can tell.

A torrent with a piece size of 500mb is ridiculously large but I don't see anything in the specs that there is a limit.

comment:3 follow-up: Changed 6 years ago by jordan

The blockCountInPiece overflow is also discussed in ticket #5754 and IMO widening it to a uint32_t would fix that, bringing us back to the 4294967295 limit here.

I'm not overjoyed with hardcoding such a number and am leaning towards this:

 #include <assert.h>
+#include <limits.h> /* UINT32_MAX */
 #include <string.h> /* strlen () */

...

-      if (!tr_variantDictFindInt (infoDict, TR_KEY_piece_length, &i) || (i < 1))
+      if (!tr_variantDictFindInt (infoDict, TR_KEY_piece_length, &i) || (i < 1) || (i > UINT32_MAX))

Any dissent/discussion on this?

comment:4 in reply to: ↑ 3 Changed 6 years ago by cfpp2p

Replying to jordan:

Any dissent/discussion on this?

discussion

The blockCountInPiece overflow is also discussed in ticket #5754 and IMO widening it to a uint32_t would fix that,

If we allow gigantically huge piece sizes greater than 229 ( 536,870,912 ) and small to tiny block sizes of 1 to 8192 then we need uint32_t

bringing us back to the 4294967295 limit here

If we limit piece sizes to something less than 229 and block sizes to greater than 8192 ( ticket:5755 ) then everything works without the uint32_t and without overflows.

Something like this would suffice. use 213 for: MIN_BLOCK_SIZE = ( 1024 * 8 )

if (!tr_variantDictFindInt (infoDict, TR_KEY_piece_length, &i) || (i < 1) || (i >= (UINT16_MAX * MIN_BLOCK_SIZE))

By using UINT16_MAX * MIN_BLOCK_SIZE ( 216 * 213 = 229 = 536,870,912 ) for the maximum piece size it follows that blockCountInPiece ( info->pieceSize / tor->blockSize ) maximum / minimum would be a maximum of 229 / 213 = UINT16_MAX ( 216 ).

In the rare cases of when tr_getBlockSize() sets block size to less than MIN_BLOCK_SIZE this would be because the piece size was less than MAX_BLOCK_SIZE to begin with. In this case tr_getBlockSize() sets block size to piece size and pieceSize / blockSize will always equal 1

No problem at all with not using hardcoding.

So in my opinion it's just what do we want to allow as far as piece and block sizes go.

Last edited 6 years ago by cfpp2p (previous) (diff)
Note: See TracTickets for help on using tickets.