Re: [patch 27/95] scanf: fix type range overflow

[Alexey Dobriyan <adobriyan@gmail.com>]

On Thu, Sep 10, 2015 at 11:37:53AM -0700, Linus Torvalds wrote:

1. Please use more caps, thread is rapidly dropping off Hacker News
   front page.

2. Security was never a concern here (or a very small one), this is all
   your imagination thinking strlcpy 2.0 is coming. Security was a concern
   when kstrto*_from_user() was added because people were copying and
   parsing numbers by hand so the opportunities for mistakes were real.
   I've checked grsec patches, they don't touch kstrto*_from_user(),
   which gives some hope that the code is all right. But then some idiot
   decided that kernel should also parse whitespace so the code like
   below couldn't be simplified:

	memset(buffer, 0, sizeof(buffer));
	if (count > sizeof(buffer) - 1)
		count = sizeof(buffer) - 1;
	if (copy_from_user(buffer, buf, count))
		return -EFAULT;
	rv = kstrtoint(strstrip(buffer), 0, &make_it_fail);
	if (rv < 0)
		return rv;
	if (make_it_fail < 0 || make_it_fail > 1)
		return -EINVAL;

   Returning to parse_integer(), whole thing is a matter of general
   robustness (no "be liberal in what you accept" rubbish) and
   hopefully nicer interface which is pleasant to use (1 function
   instead of 4, etc).

3. Your point that unchecked errors will result in bogus pointer is
   a valid one and I admit this is the case. Compiler warning on
   unchecked error in this case require gcc plugin or external checker
   or something non-trivial which kernel doesn't use. Quite sad.

4. Overall notion that overflow integer can't be security issue is
   a laughable one. I can't give you kernel example (kernel doesn't
   munge integers much obviously) but I can give you userspace example.
   with libpcre.

   Version 8.37 has notation (?[0-9]+) for forward/backward reference
   group. Internally group number is parsed into variable "int recno;"
   at pcre_compile.c:7324

	recno = 0;
	while(IS_DIGIT(*ptr))
		recno = recno * 10 + *ptr++ - CHAR_0;

   Typical code without overflow check. Later forward reference groups
   (starting with sign "+" or signless) are distinguished from backward
   reference groups (those starting with "-"). And there is very final
   check for overflowing past the number of match groups:

	if (recno != 0)
		called = PRIV(find_bracket)(cd->start_code, utf, recno);

	/* Forward reference */
	if (called == NULL)
	{
===>		if (recno > cd->final_bracount)
		{
			*errorcodeptr = ERR15;
			goto FAILED;
		}
		/* Fudge the value of "called" so that when it is inserted as an
		offset below, what it actually inserted is the reference number
		of the group. Then remember the forward reference. */

		called = cd->start_code + recno;


   So without type overflow check it is possible to supply seemingly
   positive number which will be parsed as negative and sneak in negative
   "recno" past all checks (signed variables for the win!).

   Real example: the following regex will crach pcre-8.37 with NULL
   pointer dereference (which is fallout, real bug happens earlier)
   if you try to pcre_compile()+pcre_study(PCRE_STUDY_JIT_COMPILE)

   	((?='))(?=(/(='D))?(?<!.(?2))(?=E(?2937177884))(?#(?0))

  Now I don't know if this particular crash is exploitable but exploit
  community never fail to impress.

  So, yes it can be a problem, and if someone wants to accept "-1" as
  a better user interface let him be explicit about it.

  As a matter of statistics there are 792+50=842 simple_strtoul/ll callers
  and 120+8=128 simple_strtol/ll callers, so the ratio is 1:6.6 not in
  favour of "-1" users because simple_strtoul/ll will not accept "-1".

5. The slight problem with your kitchen-sink proposal that C doesn't have
   optional/named macro and function arguments neither it does have real
   preprocessor. So people will have to specify INT_MIN/INT_MAX/...
   every invocation even if they don't care or if type information by itself
   is enough:

	uid_t loginuid;

	rv = kstrtou32_from_user(buf, count, 10, &loginuid);
	if (rv < 0)
		return rv;

   For there reasons parse_integer() leaves bound checking to the user.

   Adding "return -EINVAL" seems cool but, well, not with this language.
   Control flow changing macros are frowned upon, right?
   parse_integer() tries to behave like real C function to not create
   problems in this area.



At least it is good to know you aren't against __builtin_choose_expr() per se. :^)

	Alexey