On Tue, Mar 01, 2016 at 12:08:52PM +0900, Lipi C. H. Lee wrote: > Thanks for your comments, Rich Felker. > > I think 'get_info' function checked path length from url before calling > strcpy as belows, > > if (strlen(url+i) < 1024) strcpy(path, url+i); > else error_exit("too long path in URL"); > > Do you think it is not enough? > > And you commented whole string operations is unsafe in the code. > Could you kindly tell me any test case it is unsafe? > > It will be very helpful to me.
I don't know for sure that there are exploitable bugs, but having string length checks that are non-local with respect to where the length is assumed to fit is a very dangerous practice and does not belong anywhere near network-facing code. Really strcpy does not belong anywhere network-facing. This could be safe and worry-free with no significant cost by using snprintf or similar functions. Rich > On Sat, Feb 27, 2016 at 8:28 AM, Rich Felker <dal...@libc.org> wrote: > > > On Thu, Feb 25, 2016 at 05:28:20PM +0900, Lipi C. H. Lee wrote: > > > Thanks for your comment, Felix. > > > > > > I am sorry my response is so late. > > > As your comments, I modified some code. > > > > > > wget: clean up > > > > > > - shorten error messages > > > - replace mk_rq with sprintf > > > > This is almost surely unsafe. > > > > > - remove struct and defines > > > - change unsigned int to unsigned > > > > > > > > > [...] > > > void wget_main(void) > > > { > > > int sock; > > > - struct httpinfo hi; > > > FILE *fp; > > > - size_t len, body_len; > > > - char *body, *result, *rc, *r_str, ua[18] = "toybox wget/", ver[6]; > > > + ssize_t len, body_len; > > > + char *body, *result, *rc, *r_str; > > > + char ua[18] = "toybox wget/", ver[6], hostname[1024], port[6], > > path[1024]; > > > > > > // TODO extract filename to be saved from URL > > > - if (!(toys.optflags & FLAG_f)) > > > - help_exit("The filename to be saved should be needed."); > > > - if (fopen(TT.filename, "r")) > > > - error_exit("The file(%s) you specified already exists.", > > TT.filename); > > > + if (!(toys.optflags & FLAG_f)) help_exit("no filename"); > > > + if (fopen(TT.filename, "r")) perror_exit("file already exists"); > > > > > > - if(!toys.optargs[0]) help_exit("The URL should be specified."); > > > - get_info(&hi, toys.optargs[0]); > > > + if(!toys.optargs[0]) help_exit("no URL"); > > > + get_info(toys.optargs[0], hostname, port, path); > > > > > > - sock = conn_svr(hi.hostname, hi.port); > > > + sock = conn_svr(hostname, port); > > > > > > // compose HTTP request > > > - mk_rq(hi.path); > > > - mk_fld("Host", hi.hostname); > > > + sprintf(toybuf, "GET %s HTTP/1.1\r\n", path); > > > > Depending on the size of toybuf, it might be safe, but the above > > get_info is almost certainly unsafe already; it strcpy's into path[] > > with no limits on size. > > > > Right now this whole utility looks extremely unsafe to me -- all sorts > > of unbounded string operations. Even if you want to say you're only > > going to be passing 'safe' urls to it, it's likely vulnerable to > > malicous HTTP redirects and other similar issues. I think it needs > > some major overhaul with attention to safe string handling. > > > > Rich > > > _______________________________________________ > Toybox mailing list > Toybox@lists.landley.net > http://lists.landley.net/listinfo.cgi/toybox-landley.net _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net