Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#5732 closed Bug (fixed)

possible overflow in call to messageLengthIsCorrect()

Reported by: cfpp2p Owned by: mike.dld
Priority: Normal Milestone: 2.90
Component: libtransmission Version: 2.84
Severity: Normal Keywords:
Cc:

Description

I think there could be a possible overflow in peer-msgs.c when messageLengthIsCorrect() is called.

line 1495

if (!messageLengthIsCorrect (msgs, id, msglen + 1))

When msglen is equal to SIZE_MAX then we would overflow.

Also a possible overflow if pieceCount equals SIZE_MAX line 1387

return len == (msg->torrent->info.pieceCount + 7u) / 8u + 1u;

I'm not sure how or if these could ever happen but I thought it might be worth looking into.

Attachments (1)

5732-peer-msgs-c.diff (1.0 KB) - added by cfpp2p 6 years ago.
patch to fix

Download all attachments as: .zip

Change History (8)

comment:1 Changed 7 years ago by cfpp2p

edit:

and anytime the sum of pieceCount + 7 would exceed SIZE_MAX

Changed 6 years ago by cfpp2p

patch to fix

comment:2 Changed 6 years ago by mike.dld

  • Component changed from Transmission to libtransmission
  • Owner set to mike.dld
  • Status changed from new to assigned

comment:3 follow-up: Changed 6 years ago by mike.dld

First is not really true. When initialized, msglen is greater or equal to one (message ID size) and less or equal to UINT_MAX (variable type is uint32_t), then decrement is made with an appropriate comment:

--msglen; /* id length */

This means that msglen is now greater or equal to zero and less than UINT_MAX, so incrementing now will not lead to overflow. Adding an assertion on initial "greater than zero" condition may still be a nice change though.

Second part could be true, although not sure how as well.

Last edited 6 years ago by mike.dld (previous) (diff)

comment:4 Changed 6 years ago by mike.dld

  • Milestone changed from None Set to 2.90
  • Resolution set to fixed
  • Status changed from assigned to closed

Committed as r14568. Thanks!

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

Replying to mike.dld:

This means that msglen is now greater or equal to zero and less than UINT_MAX, so incrementing now will not lead to overflow. Adding an assertion on initial "greater than zero" condition may still be a nice change though.

I understand variable type is uint32_t
Considered in function messageLengthIsCorrect would pass true for a value of
INT_MAX at case BT_LTEP:

line 1675 peer-msgs.c
type conversion of msglen ( uint32_t ) downcast to int for call to parseLtep

static void
parseLtep (tr_peerMsgs * msgs, int msglen, struct evbuffer * inbuf)

That's what I was thinking about with the INT_MAX

Second part could be true, although not sure how as well.

I don't see that we need to get carried away with either part :)

comment:6 follow-up: Changed 6 years ago by mike.dld

I don't see the reason for it to be int. Changed to uint32_t in r14569.

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

Replying to mike.dld:

I don't see the reason for it to be int. Changed to uint32_t in r14569.

r14569 looks good

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