Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential security issue in lib/ftp.c: Unchecked return from initialization function #5412

Closed
monocle-ai opened this issue May 18, 2020 · 4 comments

Comments

@monocle-ai
Copy link

@monocle-ai monocle-ai commented May 18, 2020

What is a Conditionally Uninitialized Variable? The return value of a function that is potentially used to initialize a local variable is not checked. Therefore, reading the local variable may result in undefined behavior.

3 instances of this defect were found in the following locations:

Instance 1
File : lib/ftp.c
Function: Curl_GetFTPResponse

curl/lib/ftp.c

Line 412 in 17b1405

Curl_GetFTPResponse(&nread, conn, &ftpcode);

Code extract:

    }
    else if(result & CURL_CSELECT_IN) {
      infof(data, "Ctrl conn has data while waiting for data conn\n");
      Curl_GetFTPResponse(&nread, conn, &ftpcode); <------ HERE

      if(ftpcode/100 > 3)

How can I fix it?
Correct reference usage found in lib/ftp.c at line 3380.

curl/lib/ftp.c

Line 3380 in 17b1405

result = Curl_GetFTPResponse(&nread, conn, &ftpcode);

Code extract:

      pp->response = Curl_now(); /* timeout relative now */

      result = Curl_GetFTPResponse(&nread, conn, &ftpcode); <------ HERE
      if(result)
        return result;

Instance 2
File : lib/ftp.c
Function: Curl_inet_ntop

curl/lib/ftp.c

Line 1060 in 17b1405

Curl_inet_ntop(sa->sa_family, &sa4->sin_addr, hbuf, sizeof(hbuf));

Code extract:

      break;
#endif
    default:
      Curl_inet_ntop(sa->sa_family, &sa4->sin_addr, hbuf, sizeof(hbuf)); <------ HERE
      break;
    }

How can I fix it?
Correct reference usage found in lib/connect.c at line 650.

if(Curl_inet_ntop(sa->sa_family, &si6->sin6_addr,

Code extract:

#ifdef ENABLE_IPV6
    case AF_INET6:
      si6 = (struct sockaddr_in6 *)(void *) sa;
      if(Curl_inet_ntop(sa->sa_family, &si6->sin6_addr, <------ HERE
                        addr, MAX_IPADR_LEN)) {
        unsigned short us_port = ntohs(si6->sin6_port);

Instance 3
File : lib/ftp.c
Function: Curl_printable_address

curl/lib/ftp.c

Line 3453 in 17b1405

Curl_printable_address(ai, buf, sizeof(buf));

Code extract:

                 int port)
{
  char buf[256];
  Curl_printable_address(ai, buf, sizeof(buf)); <------ HERE
  infof(conn->data, "Connecting to %s (%s) port %d\n", newhost, buf, port);
}

How can I fix it?
Correct reference usage found in lib/socks.c at line 785.

if(Curl_printable_address(hp, dest, sizeof(dest))) {

Code extract:

      return CURLE_COULDNT_RESOLVE_HOST;
    }

    if(Curl_printable_address(hp, dest, sizeof(dest))) { <------ HERE
      size_t destlen = strlen(dest);
      msnprintf(dest + destlen, sizeof(dest) - destlen, ":%d", remote_port);
bagder added a commit that referenced this issue May 18, 2020
They're done on purpose, make that visible in the code.
Reported-by: MonocleAI
Fixes #5412
@bagder bagder closed this in dbc5c17 May 19, 2020
@siva-msft
Copy link
Contributor

@siva-msft siva-msft commented Jun 23, 2020

@bagder what do you think about instance 2?
In curl/lib/ftp.c; line 412 in 17b1405
Curl_GetFTPResponse(&nread, conn, &ftpcode);
Edit:
In curl/lib/ftp.c line 1060
Curl_inet_ntop(sa->sa_family, &sa4->sin_addr, hbuf, sizeof(hbuf));

@bagder
Copy link
Member

@bagder bagder commented Jun 23, 2020

I already added a (void) in front of that function call to mark the not-using-the-returncode as deliberate. I don't think there's an issue there.

@siva-msft
Copy link
Contributor

@siva-msft siva-msft commented Jun 23, 2020

Thanks @bagder - I think I pasted the wrong line there. How about this one?
Instace 2
Curl_inet_ntop(sa->sa_family, &sa4->sin_addr, hbuf, sizeof(hbuf));

@bagder
Copy link
Member

@bagder bagder commented Jun 23, 2020

I missed those. There seems to be two calls in ftp.c and one in if2ip.c that we should fix...

bagder added a commit that referenced this issue Jun 23, 2020
Reported-by: Siva Sivaraman
Fixes #5412
bagder added a commit that referenced this issue Jun 24, 2020
Reported-by: Siva Sivaraman
Fixes #5412
bagder added a commit that referenced this issue Jun 24, 2020
Reported-by: Siva Sivaraman
Fixes #5412
Closes #5597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.