[Toybox] Implement wget

[Lipi C. H. Lee]

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.


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20160301/f263c94e/attachment-0004.htm>