bugFreeciv - Bugs: bug #20003, Security advisory (CVE-2012-5645,...

 
 
Show feedback again

bug #20003: Security advisory (CVE-2012-5645, CVE-2012-6083)

Submitted by:  Patrick Welche <prlw1>
Submitted on:  Sun 29 Jul 2012 06:41:34 PM UTC  
 
Category: generalSeverity: 6 - Security
Priority: 5 - NormalStatus: Fixed
Assigned to: NoneOpen/Closed: Closed
Release: 2.3.2Operating System: Any
Planned Release: 2.0.11, 2.2.8, 2.3.3, 2.4.0, 2.5.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Sun 17 Feb 2013 01:41:01 PM UTC, comment #24:

(Since this is a security bug: for those watching at home: the post-commit discussion here spawned a bunch of patches intended to make the low-level protocol handling more obvious and the endpoints less tolerant of malformation, e.g. patch #3685, patch #3687. I don't think any new security issues have been identified as a result of this work?)

Jacob Nevins <jtn>
Project Administrator
Sun 03 Feb 2013 01:26:34 PM UTC, comment #23:

> 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.

pepeto <pepeto>
Project Member
Sat 02 Feb 2013 06:35:31 PM UTC, comment #22:

> 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).


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?

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.

Marko Lindqvist <cazfi>
Project Administrator
Fri 01 Feb 2013 05:52:20 PM UTC, comment #21:

> 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.

pepeto <pepeto>
Project Member
Thu 03 Jan 2013 12:16:46 AM UTC, comment #20:

They've now clarified this:

Also, since prlw1 has verified the fixes, I don't see any reason to keep this ticket open any longer.

Jacob Nevins <jtn>
Project Administrator
Wed 19 Dec 2012 11:28:55 AM UTC, comment #19:

> 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)

Marko Lindqvist <cazfi>
Project Administrator
Wed 19 Dec 2012 09:19:21 AM UTC, comment #18:

These security issues have apparently been assigned the ID
CVE-2012-5645 -- see here.
(At time of writing it's not associated with Freeciv in the master database; I assume the description will trickle back later.)

Jacob Nevins <jtn>
Project Administrator
Fri 17 Aug 2012 06:05:54 PM UTC, comment #17:

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 :-)

Patrick Welche <prlw1>
Fri 03 Aug 2012 10:51:50 PM UTC, comment #16:

Patches have been committed, but I'm not closing this ticket until it's checked that there's no more to it - I have not yet tested with the exploit code at all, so it's not guaranteed that the bug I found (and fixed) while investigated is the one advisory originally meant.

Marko Lindqvist <cazfi>
Project Administrator
Fri 03 Aug 2012 10:48:01 PM UTC, 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 21705)

Marko Lindqvist <cazfi>
Project Administrator
Fri 03 Aug 2012 10:47:51 PM UTC, SVN revision 21704:

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 21704)

Marko Lindqvist <cazfi>
Project Administrator
Fri 03 Aug 2012 10:47:45 PM UTC, 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 21703)

Marko Lindqvist <cazfi>
Project Administrator
Fri 03 Aug 2012 10:47:38 PM UTC, SVN revision 21702:

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 21702)

Marko Lindqvist <cazfi>
Project Administrator
Fri 03 Aug 2012 10:47:30 PM UTC, SVN revision 21701:

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)

Marko Lindqvist <cazfi>
Project Administrator
Wed 01 Aug 2012 01:45:58 AM UTC, comment #10:

> I see two possible ways to fix this.


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.

(file #16260, file #16261, file #16262, file #16263)

Marko Lindqvist <cazfi>
Project Administrator
Tue 31 Jul 2012 11:35:59 PM UTC, comment #9:

> (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.

Marko Lindqvist <cazfi>
Project Administrator
Tue 31 Jul 2012 10:42:56 PM UTC, SVN revision 21674:

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 21674)

Marko Lindqvist <cazfi>
Project Administrator
Tue 31 Jul 2012 10:42:50 PM UTC, 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 21673)

Marko Lindqvist <cazfi>
Project Administrator
Tue 31 Jul 2012 10:42:13 PM UTC, SVN revision 21672:

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 21672)

Marko Lindqvist <cazfi>
Project Administrator
Tue 31 Jul 2012 10:42:05 PM UTC, SVN revision 21671:

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)

Marko Lindqvist <cazfi>
Project Administrator
Tue 31 Jul 2012 10:41:50 PM UTC, SVN revision 21670:

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 21670)

Marko Lindqvist <cazfi>
Project Administrator
Tue 31 Jul 2012 01:24:40 PM UTC, comment #3:

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)

Patrick Welche <prlw1>
Sun 29 Jul 2012 10:06:38 PM UTC, comment #2:

- Fix for A], S2_2 and S2_0 version.

(file #16242)

Marko Lindqvist <cazfi>
Project Administrator
Sun 29 Jul 2012 09:35:45 PM UTC, comment #1:

> 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)

Marko Lindqvist <cazfi>
Project Administrator
Sun 29 Jul 2012 06:41:34 PM UTC, original submission:

I came across

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

and a quick look at packet.c suggests that the code in 2.3.2 is the same as in the advisory. My attempts at querying the bug database haven't returned anything, so is this still news?

Patrick Welche <prlw1>

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #16260:  RetvalForDioGet.patch added by cazfi (15kB - text/x-patch)
file #16261:  RetvalForDioGet-S2_3.patch added by cazfi (12kB - text/x-patch)
file #16262:  RetvalForDioGet-S2_2.patch added by cazfi (13kB - text/x-patch)
file #16263:  RetvalForDioGet-S2_0.patch added by cazfi (12kB - text/x-patch)
file #16241:  CheckMinTotalPacketLen.patch added by cazfi (1022B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by pepeto (Posted a comment)
  • -unavailable- added by jtn (Posted a comment)
  • -unavailable- added by cazfi (Updated the item)
  • -unavailable- added by prlw1 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 07 Apr 2013 09:58:20 AM UTCjtnSummarySecurity advisory=>Security advisory (CVE-2012-5645, CVE-2012-6083)
    Fri 29 Mar 2013 11:48:49 AM UTCjtnSeverity3 - Normal=>6 - Security
    Thu 03 Jan 2013 12:16:46 AM UTCjtnOpen/ClosedOpen=>Closed
      Operating SystemNone=>Any
    Fri 03 Aug 2012 10:51:50 PM UTCcazfiStatusReady For Test=>Fixed
    Wed 01 Aug 2012 01:47:20 AM UTCcazfiStatusNone=>Ready For Test
    Wed 01 Aug 2012 01:45:58 AM UTCcazfiAttached File-=>Added RetvalForDioGet.patch, #16260
      Attached File-=>Added RetvalForDioGet-S2_3.patch, #16261
      Attached File-=>Added RetvalForDioGet-S2_2.patch, #16262
      Attached File-=>Added RetvalForDioGet-S2_0.patch, #16263
    Sun 29 Jul 2012 10:06:38 PM UTCcazfiAttached File-=>Added CheckMinTotalPacketLen-S2_2.patch, #16242
      Planned Release=>2.0.11, 2.2.8, 2.3.3, 2.4.0, 2.5.0
    Sun 29 Jul 2012 09:35:45 PM UTCcazfiAttached File-=>Added CheckMinTotalPacketLen.patch, #16241
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup