[Toybox] Implement wget

[Rich Felker]

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 <dalias at 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 at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net


 1456803097.0