<div dir="ltr"><span style="font-size:13px">Thanks for your comments, Rich Felker.</span><div style="font-size:13px"><br></div><div style="font-size:13px">I think 'get_info' function checked path length from url before calling strcpy as belows,</div><div style="font-size:13px"><br></div><div style="font-size:13px"><div>    if (strlen(url+i) < 1024) strcpy(path, url+i);</div><div>    else error_exit("too long path in URL");</div></div><div style="font-size:13px"><br></div><div style="font-size:13px">Do you think it is not enough?</div><div style="font-size:13px"><br></div><div style="font-size:13px">And you commented whole string operations is unsafe in the code.</div><div style="font-size:13px">Could you kindly tell me any test case it is unsafe?</div><div style="font-size:13px"><br></div><div style="font-size:13px">It will be very helpful to me.</div><div style="font-size:13px"><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Feb 27, 2016 at 8:28 AM, Rich Felker <span dir="ltr"><<a href="mailto:dalias@libc.org" target="_blank">dalias@libc.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Feb 25, 2016 at 05:28:20PM +0900, Lipi C. H. Lee wrote:<br>
> Thanks for your comment, Felix.<br>
><br>
> I am sorry my response is so late.<br>
> As your comments, I modified some code.<br>
><br>
>     wget: clean up<br>
><br>
>     - shorten error messages<br>
>     - replace mk_rq with sprintf<br>
<br>
</span>This is almost surely unsafe.<br>
<span class=""><br>
>     - remove struct and defines<br>
>     - change unsigned int to unsigned<br>
><br>
><br>
</span>> [...]<br>
>  void wget_main(void)<br>
>  {<br>
>    int sock;<br>
> -  struct httpinfo hi;<br>
>    FILE *fp;<br>
> -  size_t len, body_len;<br>
> -  char *body, *result, *rc, *r_str, ua[18] = "toybox wget/", ver[6];<br>
> +  ssize_t len, body_len;<br>
> +  char *body, *result, *rc, *r_str;<br>
> +  char ua[18] = "toybox wget/", ver[6], hostname[1024], port[6], path[1024];<br>
<span class="">><br>
>    // TODO extract filename to be saved from URL<br>
</span>> -  if (!(toys.optflags & FLAG_f))<br>
> -    help_exit("The filename to be saved should be needed.");<br>
> -  if (fopen(TT.filename, "r"))<br>
> -    error_exit("The file(%s) you specified already exists.", TT.filename);<br>
> +  if (!(toys.optflags & FLAG_f)) help_exit("no filename");<br>
> +  if (fopen(TT.filename, "r")) perror_exit("file already exists");<br>
><br>
> -  if(!toys.optargs[0]) help_exit("The URL should be specified.");<br>
> -  get_info(&hi, toys.optargs[0]);<br>
> +  if(!toys.optargs[0]) help_exit("no URL");<br>
> +  get_info(toys.optargs[0], hostname, port, path);<br>
><br>
> -  sock = conn_svr(hi.hostname, hi.port);<br>
> +  sock = conn_svr(hostname, port);<br>
><br>
>    // compose HTTP request<br>
> -  mk_rq(hi.path);<br>
> -  mk_fld("Host", hi.hostname);<br>
> +  sprintf(toybuf, "GET %s HTTP/1.1\r\n", path);<br>
<br>
Depending on the size of toybuf, it might be safe, but the above<br>
get_info is almost certainly unsafe already; it strcpy's into path[]<br>
with no limits on size.<br>
<br>
Right now this whole utility looks extremely unsafe to me -- all sorts<br>
of unbounded string operations. Even if you want to say you're only<br>
going to be passing 'safe' urls to it, it's likely vulnerable to<br>
malicous HTTP redirects and other similar issues. I think it needs<br>
some major overhaul with attention to safe string handling.<br>
<span class="HOEnZb"><font color="#888888"><br>
Rich<br>
</font></span></blockquote></div><br></div>