[PATCH v1] Add bounds checking in sys_query_module

[Mike Frysinger]

On Mon 04 Aug 2014 08:31:30 zubin.mithra at gmail.com wrote:
> --- a/bjm.c
> +++ b/bjm.c
> @@ -121,6 +121,8 @@ sys_query_module(struct tcb *tcp)
>  								(idx ? ", " : ""),
>  								mod);
>  							mod += strlen(mod)+1;
> +							if (mod-data >= tcp->u_arg[3])
> +								break;

this check is incomplete.  if the buffer is not NUL terminated, then the 
string reading code will read beyond the end.  you can protect against that by 
doing:
	char *data = malloc(tcp->u_arg[3] + 1);
then before the for loop here, make sure the buffer is NUL terminated:
	data[tcp->u_arg[3]] = '\0';

as for this check, i think it's weirdly written.  it makes more sense to me:
	if (mod >= data + tcp->u_arg[3])

also, when you do break, don't you want to print out a ... to indicate ?

> @@ -144,6 +146,8 @@ sys_query_module(struct tcb *tcp)
>  						tprintf(" /* %lu entries */ ", (unsigned long)ret);
>  					} else {
>  						for (idx = 0; idx < ret; idx++) {
> +							if ((long)sym->name >= tcp->u_arg[3])
> +								break;
>  							tprintf("%s{name=%s, value=%lu}",
>  								(idx ? " " : ""),
>  								data+(long)sym->name,

i think this too is incomplete.  the sym++ might walk past the end of the 
data, sym->name is unsigned while u_arg is signed, and the symbol name might 
start near the end of the valid region but then walk beyond it (no NUL 
termination).  so i guess what are you trying to protect against ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20140806/132bebbb/attachment.bin>