[Freeciv-Dev] Re: (PR#4594) topology fix for goto route packet

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

Jason Short wrote:
> Because the server doesn't rigorously check the input coordinates for a 
> client goto route, the client can send invalid (non-normal) coordinates 
> to either trigger an assert or get Bad memory access on the server (this 
> could be a security risk).

This is a server bug. It always needs to check portal inputs to protect
itself. Reliance on others for this is an endless maintenance headache
of the magnitude of the old void tile problems. Experienced programmers
just never build such problems into their code.

[...]

> The second attached patch, fix-server.diff, will avoid the crash by 
> testing for the normal-ness of the position.  Note that this means the 
> client must send NORMAL positions even though it seems the server could 
> easily call normalize_map_pos on them.  This patch is targeted toward 
> S1_14 and fixes a potential security hole.

This is a *bad* fix. Requiring clients to do things in order to prevent
server crashes just violates fundamental programming norms. And not doing
things to trivially cleanup the problem, i.e. failing the client, is just
rampant user-unfriendliness.

This case has been discussed many times and enforcing religious constraints
on client behaviour has always raised the same negative reaction.

There is absolutely *no* reason why a server should not check its inputs
and *no* reason why it should not just run normalize_map_pos() to do the
fix at the same time as it does the check. Use of is_normal_map_pos() is
always an indication of a religion induced bug (i.e. technically unnecessary
failure condition).

The only problem here is unreal coordinates which cannot be fixed, and which
should be returned as a failure for the client to deal with - server fixes
usually lead to more problems in this case. This is indicated by the false
return from normalize_map_pos().

Unnormal coordinates are perfectly fixable in general, and should routinely
just be fixed in passing at various points within and across module
boundaries. There is no need to throw religious asserts to crash programs
that are not broken in any technical way.

Cheers,
RossW
=====

[...]
> jason
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: common/packets.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/packets.c,v
> retrieving revision 1.249
> diff -u -r1.249 packets.c
> --- common/packets.c  2003/07/10 03:34:30     1.249
> +++ common/packets.c  2003/07/17 18:29:11
> @@ -3065,8 +3065,8 @@
>      }
>      /* if we finished fill the last chunk with NOPs */
>      for (; chunk_pos < GOTO_CHUNK; chunk_pos++) {
> -      dio_put_uint8(&dout, map.xsize);
> -      dio_put_uint8(&dout, map.ysize);
> +      dio_put_uint8(&dout, 255);
> +      dio_put_uint8(&dout, 255);
>      }
>  
>      dio_put_uint16(&dout, packet->unit_id);
> 
> 
> ------------------------------------------------------------------------
> 
> Index: common/packets.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/packets.c,v
> retrieving revision 1.219.2.3
> diff -u -r1.219.2.3 packets.c
> --- common/packets.c  2003/03/17 16:20:54     1.219.2.3
> +++ common/packets.c  2003/07/17 18:31:07
> @@ -2905,7 +2905,9 @@
>    for (i = 0; i < GOTO_CHUNK; i++) {
>      dio_get_uint8(&din, &pos[i].x);
>      dio_get_uint8(&din, &pos[i].y);
> -    if (pos[i].x != map.xsize) num_valid++;
> +    if (is_normal_map_pos(pos[i].x, pos[i].y)) {
> +      num_valid++;
> +    }
>    }
>    dio_get_uint16(&din, &unit_id);
>  
> 
> 
> ------------------------------------------------------------------------
> 
> Index: common/capstr.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/capstr.c,v
> retrieving revision 1.138
> diff -u -r1.138 capstr.c
> --- common/capstr.c   2003/07/14 21:01:49     1.138
> +++ common/capstr.c   2003/07/17 18:39:19
> @@ -79,7 +79,7 @@
>                     "+impr_req +waste +fastfocus +continent +small_dipl " \
>                     "+no_nation_selected +diplomacy +no_extra_tiles " \
>                     "+diplomacy2 +citizens_style +root_tech auth " \
> -                   "+nat_ulimit retake"
> +                   "+nat_ulimit retake +goto_pack"
>  
>  /* "+1.14.0" is protocol for 1.14.0 release.
>   *
> @@ -137,6 +137,9 @@
>   * allowing easy adding of arbitrarily many nations.
>   *
>   * "retake" means that a client can switch players during a running game.
> + *
> + * "goto_pack" changes the goto route packet to send (255,255) instead of
> + * map.xsize/map.ysize to mark the end of the goto chunk.
>   */
>  
>  void init_our_capability(void)
> Index: common/packets.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/packets.c,v
> retrieving revision 1.249
> diff -u -r1.249 packets.c
> --- common/packets.c  2003/07/10 03:34:30     1.249
> +++ common/packets.c  2003/07/17 18:39:20
> @@ -3064,9 +3064,10 @@
>        chunk_pos++;
>      }
>      /* if we finished fill the last chunk with NOPs */
> +    assert(!is_normal_map_pos(MAX_UINT8, MAX_UINT8));
>      for (; chunk_pos < GOTO_CHUNK; chunk_pos++) {
> -      dio_put_uint8(&dout, map.xsize);
> -      dio_put_uint8(&dout, map.ysize);
> +      dio_put_uint8(&dout, MAX_UINT8);
> +      dio_put_uint8(&dout, MAX_UINT8);
>      }
>  
>      dio_put_uint16(&dout, packet->unit_id);
> @@ -3106,7 +3107,9 @@
>    for (i = 0; i < GOTO_CHUNK; i++) {
>      dio_get_uint8(&din, &pos[i].x);
>      dio_get_uint8(&din, &pos[i].y);
> -    if (pos[i].x != map.xsize) num_valid++;
> +    if (is_normal_map_pos(pos[i].x, pos[i].y)) {
> +      num_valid++;
> +    }
>    }
>    dio_get_uint16(&din, &unit_id);
>  
> Index: common/shared.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/shared.h,v
> retrieving revision 1.113
> diff -u -r1.113 shared.h
> --- common/shared.h   2003/05/13 11:16:52     1.113
> +++ common/shared.h   2003/07/17 18:39:20
> @@ -110,6 +110,8 @@
>  
>  /* This is duplicated in rand.h to avoid extra includes: */
>  #define MAX_UINT32 0xFFFFFFFF
> +#define MAX_UINT16 0xFFFF
> +#define MAX_UINT8 0xFF
>  
>  #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>  #define ADD_TO_POINTER(p, n) ((void *)((char *)(p)+(n)))