Changeset 91631 in vbox for trunk/src/VBox/Devices/Network

Timestamp:

Oct 7, 2021 10:00:09 PM (3 years ago)

Author:

vboxsync

Message:

NAT/tftp: bugref:9350 - Redo r128102. Just reject pathnames that try
to use "..", like BSD tftpd does. Always convert backslashes into
forward slashes.

File:

• trunk/src/VBox/Devices/Network/slirp/tftp.c (modified) (9 diffs)

trunk/src/VBox/Devices/Network/slirp/tftp.c

@@ -75 +75 @@
     TFPTPSESSIONOPTDESC OptionTimeout;
 
+    const char *pcszFilenameHost;
     char szFilename[TFTP_FILENAME_MAX];
 } TFTPSESSION, *PTFTPSESSION, **PPTFTPSESSION;
@@ -135 +136 @@
 
 /**
- * This function evaluate file name.
- * @param pu8Payload
- * @param cbPayload
- * @param cbFileName
- * @return VINF_SUCCESS -
- *         VERR_INVALID_PARAMETER -
+ * This function resolves file name relative to tftp prefix.
+ * @param pData
+ * @param pTftpSession
  */
-DECLINLINE(int) tftpSecurityFilenameCheck(PNATState pData, PCTFTPSESSION pcTftpSession)
-{
-    int rc = VINF_SUCCESS;
-    AssertPtrReturn(pcTftpSession, VERR_INVALID_PARAMETER);
-
-    /* only allow exported prefixes */
-    if (!tftp_prefix)
-        rc = VERR_INTERNAL_ERROR;
-    else
-    {
-        char *pszFullPathAbs = RTPathAbsExDup(tftp_prefix, pcTftpSession->szFilename, RTPATH_STR_F_STYLE_HOST);
-
-        if (   !pszFullPathAbs
-            || !RTPathStartsWith(pszFullPathAbs, tftp_prefix))
-            rc = VERR_FILE_NOT_FOUND;
-
-        RTStrFree(pszFullPathAbs);
-    }
+DECLINLINE(int) tftpSecurityFilenameCheck(PNATState pData, PTFTPSESSION pTftpSession)
+{
+    int rc = VERR_FILE_NOT_FOUND; /* guilty until proved innocent */
+
+    AssertPtrReturn(pTftpSession, VERR_INVALID_PARAMETER);
+    AssertReturn(pTftpSession->pcszFilenameHost == NULL, VERR_INVALID_PARAMETER);
+
+    /* prefix must be set to an absolute pathname.  assert? */
+    if (tftp_prefix == NULL || RTPathSkipRootSpec(tftp_prefix) == tftp_prefix)
+        goto done;
+
+    /* replace backslashes with forward slashes */
+    char *s = pTftpSession->szFilename;
+    while ((s = strchr(s, '\\')) != NULL)
+        *s++ = '/';
+
+    /* deny attempts to break out of tftp dir */
+    if (RTStrStartsWith(pTftpSession->szFilename, "../"))
+        goto done;
+
+    const char *dotdot = RTStrStr(pTftpSession->szFilename, "/..");
+    if (dotdot != NULL && (dotdot[3] == '/' || dotdot[3] == '\0'))
+        goto done;
+
+    char *pszPathHostAbs;
+    int cbLen = RTStrAPrintf(&pszPathHostAbs, "%s/%s",
+                           tftp_prefix, pTftpSession->szFilename);
+    if (cbLen == -1)
+        goto done;
+
+    LogRel2(("NAT: TFTP: %s\n", pszPathHostAbs));
+    pTftpSession->pcszFilenameHost = pszPathHostAbs;
+    rc = VINF_SUCCESS;
+
+  done:
     LogFlowFuncLeaveRC(rc);
     return rc;
@@ -254 +269 @@
 DECLINLINE(void) tftpSessionTerminate(PTFTPSESSION pTftpSession)
 {
+    if (pTftpSession->pcszFilenameHost != NULL)
+    {
+        RTStrFree((char *)pTftpSession->pcszFilenameHost);
+        pTftpSession->pcszFilenameHost = NULL;
+    }
+
     pTftpSession->fInUse = 0;
 }
@@ -378 +399 @@
 
  found:
-    memset(pTftpSession, 0, sizeof(*pTftpSession));
+    if (pTftpSession->pcszFilenameHost != NULL)
+    {
+        RTStrFree((char *)pTftpSession->pcszFilenameHost);
+        // pTftpSession->pcszFilenameHost = NULL; /* will be zeroed out below */
+    }
+    RT_ZERO(*pTftpSession);
+
     memcpy(&pTftpSession->IpClientAddress, &pcTftpIpHeader->IPv4Hdr.ip_src, sizeof(pTftpSession->IpClientAddress));
     pTftpSession->u16ClientPort = pcTftpIpHeader->UdpHdr.uh_sport;
@@ -437 +464 @@
 }
 
-DECLINLINE(int) pftpSessionOpenFile(PNATState pData, PTFTPSESSION pTftpSession, PRTFILE pSessionFile)
-{
-    char szSessionFilename[TFTP_FILENAME_MAX];
-    ssize_t cchSessionFilename;
+DECLINLINE(int) pftpSessionOpenFile(PTFTPSESSION pTftpSession, PRTFILE pSessionFile)
+{
     int rc;
     LogFlowFuncEnter();
 
-    cchSessionFilename = RTStrPrintf2(szSessionFilename, TFTP_FILENAME_MAX, "%s/%s", tftp_prefix, pTftpSession->szFilename);
-    if (cchSessionFilename > 0)
-    {
-        LogFunc(("szSessionFilename: %s\n", szSessionFilename));
-        if (RTFileExists(szSessionFilename))
-        {
-            rc = RTFileOpen(pSessionFile, szSessionFilename, RTFILE_O_READ | RTFILE_O_OPEN | RTFILE_O_DENY_WRITE);
-        }
-        else
+    if (pTftpSession->pcszFilenameHost == NULL)
+    {
+        rc = VERR_FILE_NOT_FOUND;
+    }
+    else
+    {
+        rc = RTFileOpen(pSessionFile, pTftpSession->pcszFilenameHost,
+                        RTFILE_O_OPEN | RTFILE_O_READ | RTFILE_O_DENY_WRITE);
+        if (RT_FAILURE(rc))
             rc = VERR_FILE_NOT_FOUND;
     }
-    else
-        rc = VERR_FILENAME_TOO_LONG;
 
     LogFlowFuncLeaveRC(rc);
@@ -462 +485 @@
 }
 
-DECLINLINE(int) tftpSessionEvaluateOptions(PNATState pData, PTFTPSESSION pTftpSession)
+DECLINLINE(int) tftpSessionEvaluateOptions(PTFTPSESSION pTftpSession)
 {
     int rc;
@@ -470 +493 @@
     LogFlowFunc(("pTftpSession:%p\n", pTftpSession));
 
-    rc = pftpSessionOpenFile(pData, pTftpSession, &hSessionFile);
+    rc = pftpSessionOpenFile(pTftpSession, &hSessionFile);
     if (RT_FAILURE(rc))
     {
@@ -569 +592 @@
 
     u16BlkSize = (uint16_t)pcTftpSession->OptionBlkSize.u64Value;
-    rc = pftpSessionOpenFile(pData, pcTftpSession, &hSessionFile);
+    rc = pftpSessionOpenFile(pcTftpSession, &hSessionFile);
     if (RT_FAILURE(rc))
     {
@@ -639 +662 @@
     int rc;
 
-    rc = tftpSessionEvaluateOptions(pData, pTftpSession);
+    rc = tftpSessionEvaluateOptions(pTftpSession);
     if (RT_FAILURE(rc))
     {