Messages in this thread | | | Date | Fri, 11 Sep 2015 23:52:29 +0300 | From | Alexey Dobriyan <> | Subject | Re: [patch 27/95] scanf: fix type range overflow |
| |
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
| |