Messages in this thread | | | Date | Tue, 02 Jun 2009 08:55:17 +0800 | From | Li Zefan <> | Subject | Re: [PATCH 2/2] tracing/filters: use strcmp() instead of strncmp() |
| |
Frederic Weisbecker wrote: > On Mon, Jun 01, 2009 at 01:45:47PM +0800, Li Zefan wrote: >>>>>> I don't think there's any security issue. It's irrelevant how big the user-input >>>>>> strings are. The point is those strings are guaranteed to be NULL-terminated. >>>>>> Am I missing something? >>>>>> >>>>>> And I don't think it's necessary to make 2 patches that each patch converts >>>>>> one strncmp to strcmp. But maybe it's better to improve this changelog? >>>>> Hmm, you must be right, indeed they seem to be guaranted beeing NULL-terminated >>>>> strings. >>>>> >>>> Sorry, I was wrong. :( >>>> >>>> Though the user-input strings are guaranted to be NULL-terminated, strings >>>> generated by TRACE_EVENT might not. >>>> >>>> We define static strings this way: >>>> TP_struct( >>>> __array(char, foo, LEN) >>>> ) >>>> But foo is not necessarily a string, though I doubt someone will use it >>>> as non-string char array. >>> >>> Yeah, but the user defined comparison operand is NULL terminated. >>> So the strcmp will stop at this boundary. >>> >> The user input string is NULL terminated and is limited to MAX_FILTER_STR_VAL, >> and it's strcmp() not strcpy(), but it's still unsafe. No? >> >> cmp = strcmp(addr, pred->str_val); >> >> If addr is not NULL-terminated string but char array, and length of >> str_val > length of addr, then we'll be exceeding the boundary of the >> array. > > > > No, once both strings appear to be different, strcmp returns. > As an example, the generic strcmp in lib/string.c is as follows: > > int strcmp(const char *cs, const char *ct) > { > signed char __res; > > while (1) { > if ((__res = *cs - *ct++) != 0 || !*cs++) > break; > } > return __res; > } > > Once cs[n] != ct[n], or !cs[n] || !ct[n], strcmp() stops, > and the x86 implementation does exactly the same. > > So I guess it's safe. >
See this example:
cmp = strcmp(addr, pred->str_val);
length(addr) == 6, strlen(str_val) == 10
123456 addr: abcdef? ^ | v str_val: abcdefzzzz\0
or the 2 happen to match even after addr overflowed:
123456 addr: abcdefzzzz? ^ | v str_val: abcdefzzzz\0
| |