[Swan-dev] integer types biting us [long]

D. Hugh Redelmeier hugh at mimosa.com
Thu Aug 11 18:38:42 EEST 2022


1)

intmax_t and uintmax_t should never be used.

Those types were always  bit sloppy.  For example, they may be quite 
inefficient.

They used to be safe to use in code that was trying to be generic.  A 
function parameter of this type could handle any int type thrown at it (if 
the signedness was right).  A macro could define a temporary that could 
handle any int type thrown at it (but the typeof extension was better).

The *intmax_t types were almost never a good idea for the result of a 
function unless the receiver expected that type.  A nice exception is 
illustrated by a max function: the value of the result is bounded by the 
values of its arguments.

But the C Standards committee has decided that preserving old ABIs is more 
important than preserving the standard's promise to programmers.
Some ABIs (note: not produced by the Committee) use *intmax_t.  The 
Committee wants to allow new, wider int types.  It does not want to break 
those ABIs, so it is breaking *intmax_t: new wider integer types can be 
wider than *intmax_t.

This is a Bad Thing:

- the only point of the *intmax_t types was to create a universal integer 
  type.  This point disappears.

- this promise to the C programmer has been rescinded.

- The is a silent change: no compile-time error message can be generated.

- there is explicitly a promise that C changes will not break programs and 
  equally explicitly the Committee has reserved the right to break 
  implementation

This change breaks the C Standards Committee's explicit remit.

I told the Committee so but they did this anyway.
breaks their explicit remit.  I think that this is a scandal, but they 
didn't listen to me.

	*intmax_t no longer is promised to be able to encompass all
	integral types of the appropriate signedness.

So *intmax_t has lost all its purpose.  

The Committee should have removed it from the language rather than 
silently breaking code that used it.

The ABIs that cannot change binaries should change declarations to use the 
appropriate explict types.  This is an easy change for ABIs.


2)

Andrew mentioned https://github.com/libreswan/libreswan/issues/763

[12:37] <cagney>  addconn overflows uintmax_t replay-window=4294967294 to 
replay-window=18446744073709551614 #763 
[12:46] <cagney> 
testing/pluto/pfkeyv2-replay-window-freebsd/freebsdw.console.txt
[12:48] <cagney> replay-window=4294967294 # UINT32_MAX - 1

4294967294 is (uint32) -2 
18446744073709551614 is (uint64_t) -1

This demonstrates more than one bug.


3)
[Exploring a bit]

In libipsecconf the universal integer type is uintmax_t

include/ipsecconf/keywords.h:

struct kw_list {
        struct kw_list *next;
        struct keyword keyword;
        char *string;
/**/    uintmax_t number;
};


But also plain "int" (i.e. signed).

include/ipsecconf/confread.h:

typedef int knf[KEY_NUMERIC_ROOF];

struct starter_config {
	struct {
                ksf strings;
                knf options;
                str_set strings_set;
                int_set options_set;
	} setup;

  	/* conn %default */
	struct starter_conn conn_default;

	char *ctlsocket;  /* location of pluto control socket */

	/* connections list (without %default) */
  	TAILQ_HEAD(, starter_conn) conns;
};

There are cases of a knf member being assigned a kw_list number
without bounds checking.  There are cases of a knf member being
assigned to an unsigned object without bounds checking.  I have not
done a census to count the times.

How many options are actually signed?  (I.e. can we eliminated signed
integral types?)

How many (signed or unsigned) integer options cannot be limited to 
INTMAX_T? (I.e. can we use an signed type as a universal option
holder?)

Maybe we cannot have a universal integral option holder.


4)

Every time we narrow an integral value to assign it to an object with an 
integer type, we should check if it fits.

This is a special case of overflow checking.  We should always do overflow 
checking.

This is tiresome busywork.  But this is major source of bugs and in 
particular security bugs.  Especially in C code.

When I code in C, I'm constantly proving to myself that overflow cannot 
happen.  Sometimes I slip up.  Sometimes the proof is tricky and should 
appear in comments (tiresome busywork).

We should add helper functions so that it is less annoying to add the 
required checks.

It would be nice if the C compiler could flag suspicious narrowing at
compile time.  Unfortunately, that would probably be unacceptably
noisy.


4)

Retrofitting overflow checking is typically a lot of tricky work.  But
it needs doing.  It can have security implications.


More information about the Swan-dev mailing list