> IIRC return value is solely about whether data was available
> (and read). These low-level functions do not know what data is
> valid. Maybe dio_get_uint8() has a bug?

I think so. I will try to investigate a bit deeper...

> Usually lack of valid data will lead to connection being closed
> in upper level, but the original bug here was that low-level
> ended to infinite loop and it never returned to upper level.

I understood this point. However, no test is performed in the packets body, or nearly not. I will try to build a more complete patch.

> Well, third one is what I've planned to do for a long time*:
> give dio_get_xxx() functions return values telling if they
> succeeded or failed. Patch attached.
>
> *) According to very old TODO I had actually foreseen
> possibility of infinite loop somewhere when I first came across
> the dio_get_xxx() functions and noticed their lack of return
> value.
>
> Any volunteers to do throughout checking of all dio_get_xxx()
> callers in case there's other places where return values (added
> by this patch) should be checked.

When working on porting this patch to warclient, numerous questions came to me. I had also noticed something wrong in those functions, including the case of infinite loops and wrong data for a very long time.

What value returns the dio_get_xxx() ? According to your comment, I understand that these functions returns TRUE if the value is read and valid. However, the code doesn't match this (for example can dio_get_uint8() returns TRUE even if there was no more byte, functions like dio_get_bit_string() look strange).

I guess that all receive_packet_xxx() functions should also test the results of the dio_get_xxx() ones.

Also, shouldn't the server of the client cut the connection of a such packet ? It clearly doesn't match the protocol, so it is not a compatible connection.

> CVE-2012-5645

They had missed the fact that two issues were reported in this single ticket. CVE description contained both, but they provided only one fix. I informed them about this. I assume they will assign new CVE to the other half (rather than update existing one to contain second fix also, as that would lead to confusion with those who already list current CVE as fixed)

I have checked your patches against the exploits and they do fix it:

2: Lost connection: c1 from localhost (illegal packet size).

for part 1, instead of the out of memory error, and

1: Receiving packet_player_info at the server.
1: Received value isn't boolean: 255
1: last message repeated 2 times
1: last message repeated 2 times (total 4 repeats)
1: last message repeated 4 times (total 8 repeats)
1: last message repeated 8 times (total 16 repeats)
1: last message repeated 16 times (total 32 repeats)
1: last message repeated 32 times (total 64 repeats)
1: last message repeated 23 times (total 87 repeats)
1: received bad string in packet (type 51, len 103) from c2 from localhost (connection incomplete)
1: received short packet (type 51, len 103) from c2 from localhost (connection incomplete)
1: Received game packet PACKET_PLAYER_INFO(51) from unaccepted connection c2 from localhost (connection incomplete).

for part 2 instead of a hang.

All fixed :-)

Added return value indicating success or failure for all dio_get_xxx()
functions, and check that value to avoid infinite loop in reading arrays
from network when there's no more data even though it's expected.

From http://aluigi.altervista.org/adv/freecivet-adv.txt

Reported by Patrick Welche

See gna bug #20003

(Browse SVN revision 21705)

Added return value indicating success or failure for all dio_get_xxx()
functions, and check that value to avoid infinite loop in reading arrays
from network when there's no more data even though it's expected.

From http://aluigi.altervista.org/adv/freecivet-adv.txt

Reported by Patrick Welche

See gna bug #20003

(Browse SVN revision 21703)

Added return value indicating success or failure for all dio_get_xxx()
functions, and check that value to avoid infinite loop in reading arrays
from network when there's no more data even though it's expected.

From http://aluigi.altervista.org/adv/freecivet-adv.txt

Reported by Patrick Welche

See gna bug #20003

(Browse SVN revision 21701)

> (What seems odd is that the exploit seems to send many 0xff's,
> and I would have expected the opposite)

Problem is that in error situation - when there's no more data - dio_get_uint8() returns 0, not 255. So if there's not enough data, it will try to read it in infinite loop.

I see two possible ways to fix this. I have to investigate consequences to other parts of the code more to decide better one.

Sanity check packet length received over network against values
less than header length alone to avoid situation where body length
is considered negative.

From http://aluigi.altervista.org/adv/freecivet-adv.txt

Reported by Patrick Welche

See gna bug #20003

(Browse SVN revision 21673)

Sanity check packet length received over network against values
less than header length alone to avoid situation where body length
is considered negative.

From http://aluigi.altervista.org/adv/freecivet-adv.txt

Reported by Patrick Welche

See gna bug #20003

(Browse SVN revision 21671)

Thank you for your patch which fixes part A].

As to part B], it seems that the infinite loop comes from this part of common/generate_packets.py:
<pre>
544 else:
545 return '''
546 for (;;) {
547 int i;
548
549 dio_get_uint8(&din, &i);
550 if(i == 255) {
551 break;
552 }
553 if(i > %(array_size_u)s) {
554 log_error("packets_gen.c: WARNING: ignoring intra array diff");
555 } else {
556 %(c)s
557 }
558 }'''%self.get_dict(vars())
</pre>

The only way out of the for(;;) is if we manage to read 255.
(What seems odd is that the exploit seems to send many 0xff's, and I would have expected the opposite)

> is this still news?

Yes. It would be nice if those who already invest so much time to investigate security and write advisories would bother to inform us too so that these things would get also fixed.

Fix for A] for S2_3, S2_4 and TRUNK attached. This patch doesn't apply to S2_2, but security fix is still worth backporting (will check also S2_0 in case warclient folks will update their fork)

(file #16241)