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

Jump to bottom

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

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

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

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

Comments

@monocle-ai
Copy link

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.

curl/lib/connect.c

Line 650 in 17b1405

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.

curl/lib/socks.c

Line 785 in 17b1405

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
@bagder
ftp: mark return-ignoring calls to Curl_GetFTPResponse with (void)
07f7d26 
They're done on purpose, make that visible in the code.
Reported-by: MonocleAI
Fixes #5412
@bagder bagder mentioned this issue May 18, 2020
Closed
@bagder bagder closed this as completed in dbc5c17 May 19, 2020
@siva-msft
Copy link
Contributor

siva-msft commented Jun 23, 2020 •
edited

@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 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 commented Jun 23, 2020 •
edited

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 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
@bagder
Curl_inet_ntop: always check the return code
e447be4 
Reported-by: Siva Sivaraman
Fixes #5412
@bagder bagder mentioned this issue Jun 23, 2020
Closed
bagder added a commit that referenced this issue Jun 24, 2020
@bagder
Curl_inet_ntop: always check the return code
03a8716 
Reported-by: Siva Sivaraman
Fixes #5412
bagder added a commit that referenced this issue Jun 24, 2020
@bagder
Curl_inet_ntop: always check the return code
d2baa50 
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
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@bagder @monocle-ai @siva-msft