#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)
Change History (8)
comment:1 Changed 8 years ago by cfpp2p
comment:2 Changed 7 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: ↓ 5 Changed 7 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.
comment:4 Changed 7 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 7 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: ↓ 7 Changed 7 years ago by mike.dld
I don't see the reason for it to be int. Changed to uint32_t in r14569.
edit:
and anytime the sum of pieceCount + 7 would exceed SIZE_MAX