ICU-21106 avoid buffer overflow in u_fopen_u, fix WIN32_API fclose(NULL)

icu4c/source/io/ufile.cpp

@@ -40,6 +40,7 @@
 #include "unicode/ures.h"
 #include "unicode/ucnv.h"
 #include "unicode/ustring.h"
+#include "unicode/unistr.h"
 #include "cstring.h"
 #include "cmemory.h"
 
@@ -142,18 +143,42 @@ u_fopen(const char    *filename,
     return result; /* not a file leak */
 }
 
+// FILENAME_BUF_MAX represents the largest size that we are willing to use for a
+// stack-allocated buffer to contain a file name or path. If PATH_MAX (POSIX) or MAX_PATH
+// (Windows) are defined and are smaller than this we will use their defined value;
+// otherwise, we will use FILENAME_BUF_MAX for the stack-allocated buffer, and dynamically
+// allocate a buffer for any file name or path that is that length or longer.
+#define FILENAME_BUF_MAX 296
+#if defined PATH_MAX && PATH_MAX < FILENAME_BUF_MAX
+#define FILENAME_BUF_CAPACITY PATH_MAX
+#elif defined MAX_PATH && MAX_PATH < FILENAME_BUF_MAX
+#define FILENAME_BUF_CAPACITY MAX_PATH
+#else
+#define FILENAME_BUF_CAPACITY FILENAME_BUF_MAX
+#endif
+
 U_CAPI UFILE* U_EXPORT2
 u_fopen_u(const UChar   *filename,
         const char    *perm,
         const char    *locale,
         const char    *codepage)
 {
-    UFILE     *result;
-    char buffer[256];
+    UFILE *result;
+    char buffer[FILENAME_BUF_CAPACITY];
+    char *filenameBuffer = buffer;
 
-    u_austrcpy(buffer, filename);
+    icu::UnicodeString filenameString(true, filename, -1); // readonly aliasing, does not allocate memory
+    // extract with conversion to platform default codepage, return full length (not including 0 termination)
+    int32_t filenameLength = filenameString.extract(0, filenameString.length(), filenameBuffer, FILENAME_BUF_CAPACITY);
+    if (filenameLength >= FILENAME_BUF_CAPACITY) { // could not fit (with zero termination) in buffer
+        filenameBuffer = static_cast<char *>(uprv_malloc(++filenameLength)); // add one for zero termination
+        if (!filenameBuffer) {
+            return nullptr;
+        }
+        filenameString.extract(0, filenameString.length(), filenameBuffer, filenameLength);
+    }
 
-    result = u_fopen(buffer, perm, locale, codepage);
+    result = u_fopen(filenameBuffer, perm, locale, codepage);
 #if U_PLATFORM_USES_ONLY_WIN32_API
     /* Try Windows API _wfopen if the above fails. */
     if (!result) {
@@ -161,20 +186,25 @@ u_fopen_u(const UChar   *filename,
         wchar_t wperm[40] = {};
         size_t  retVal;
         mbstowcs_s(&retVal, wperm, UPRV_LENGTHOF(wperm), perm, _TRUNCATE);
-        FILE *systemFile = _wfopen((const wchar_t *)filename, wperm);
+        FILE *systemFile = _wfopen(reinterpret_cast<const wchar_t *>(filename), wperm); // may return NULL for long filename
         if (systemFile) {
             result = finit_owner(systemFile, locale, codepage, TRUE);
         }
-        if (!result) {
+        if (!result && systemFile) {
             /* Something bad happened.
-               Maybe the converter couldn't be opened. */
+               Maybe the converter couldn't be opened.
+               Bu do not fclose(systemFile) if systemFile is NULL. */
             fclose(systemFile);
         }
     }
 #endif
+    if (filenameBuffer != buffer) {
+        uprv_free(filenameBuffer);
+    }
     return result; /* not a file leak */
 }
 
+
 U_CAPI UFILE* U_EXPORT2
 u_fstropen(UChar *stringBuf,
            int32_t      capacity,

icu4c/source/io/unicode/ustdio.h

@@ -231,7 +231,7 @@ typedef enum {
  * That is, data written to a UFILE will be formatted using the conventions
  * specified by that UFILE's Locale; this data will be in the character set
  * specified by that UFILE's codepage.
- * @param filename The name of the file to open.
+ * @param filename The name of the file to open. Must be 0-terminated.
  * @param perm The read/write permission for the UFILE; one of "r", "w", "rw"
  * @param locale The locale whose conventions will be used to format 
  * and parse output. If this parameter is NULL, the default locale will 
@@ -254,7 +254,7 @@ u_fopen(const char    *filename,
  * That is, data written to a UFILE will be formatted using the conventions
  * specified by that UFILE's Locale; this data will be in the character set
  * specified by that UFILE's codepage.
- * @param filename The name of the file to open.
+ * @param filename The name of the file to open. Must be 0-terminated.
  * @param perm The read/write permission for the UFILE; one of "r", "w", "rw"
  * @param locale The locale whose conventions will be used to format
  * and parse output. If this parameter is NULL, the default locale will

icu4c/source/test/iotest/filetst.c

@@ -15,6 +15,7 @@
  */
 
 #include "cmemory.h"
+#include "cstring.h"
 #include "iotest.h"
 #include "unicode/ustdio.h"
 #include "unicode/ustring.h"
@@ -27,9 +28,18 @@ const char *STANDARD_TEST_FILE = "iotest-c.txt";
 
 const char *STANDARD_TEST_LOCALE = "en_US_POSIX";
 
+const char *MEDIUMNAME_TEST_FILE =
+"iotest_medium_filename_4567_30_234567_40_234567_50_234567_60_234567_70_234567_80_234567_90_23456_100"
+"_23456_110_23456_120.txt"; // 124 chars
+
+const char *LONGNAME_TEST_FILE =
+"iotest_long_filename_234567_30_234567_40_234567_50_234567_60_234567_70_234567_80_234567_90_23456_100"
+"_23456_110_23456_120_23456_130_23456_140_23456_150_23456_160_23456_170_23456_180_23456_190_23456_200"
+"_23456_210_23456_220_23456_230_23456_240_23456_250_23456_260.txt"; // 264 chars, may  be too long on some filesystems
+
 
 #if !UCONFIG_NO_FORMATTING
-static void TestFileFromICU(UFILE *myFile) {
+static void TestFileFromICU(UFILE *myFile, const char* description) {
     int32_t n[1];
     float myFloat = -1234.0;
     int32_t newValuePtr[1];
@@ -48,7 +58,11 @@ static void TestFileFromICU(UFILE *myFile) {
     memset(testBuf, '*', UPRV_LENGTHOF(testBuf));
 
     if (myFile == NULL) {
-        log_err("Can't write test file.\n");
+        if (uprv_strstr(description, "ULONGNAME")) {
+            log_info("Can't %s test file, OK.\n", description);
+        } else {
+            log_err("Can't %s test file.\n", description);
+        }
         return;
     }
 
@@ -314,10 +328,24 @@ static void TestFileFromICU(UFILE *myFile) {
     u_fclose(myFile);
 }
 
+enum { kUFilenameBufLen = 296 };
 static void TestFile(void) {
+    
+    UChar ufilename[kUFilenameBufLen + 1]; // +1 for guaranteed 0 termination
+    ufilename[kUFilenameBufLen] = 0; // ensure 0 termination
+ 
+    log_verbose("Testing u_fopen with STANDARD_TEST_FILE\n");
+    TestFileFromICU(u_fopen(STANDARD_TEST_FILE, "w", STANDARD_TEST_LOCALE, NULL), "u_fopen STANDARD");
 
-    log_verbose("Testing u_fopen\n");
-    TestFileFromICU(u_fopen(STANDARD_TEST_FILE, "w", STANDARD_TEST_LOCALE, NULL));
+    u_uastrncpy(ufilename, MEDIUMNAME_TEST_FILE, kUFilenameBufLen); 
+    log_verbose("Testing u_fopen_u with UMEDIUMNAME_TEST_FILE\n");
+    TestFileFromICU(u_fopen_u(ufilename, "w", STANDARD_TEST_LOCALE, NULL), "u_fopen_u UMEDIUMNAME");
+
+    // The following u_fopen_u will fail to open a file on many filesystems (name too long)
+    // but we want to make sure that at least we do not crash in u_fopen_u name conversion.
+    u_uastrncpy(ufilename, LONGNAME_TEST_FILE, kUFilenameBufLen); 
+    log_verbose("Testing u_fopen_u with ULONGNAME_TEST_FILE\n");
+    TestFileFromICU(u_fopen_u(ufilename, "w", STANDARD_TEST_LOCALE, NULL), "u_fopen_u ULONGNAME");
 }
 
 static void TestFinit(void) {
@@ -325,7 +353,7 @@ static void TestFinit(void) {
 
     log_verbose("Testing u_finit\n");
     standardFile = fopen(STANDARD_TEST_FILE, "w");
-    TestFileFromICU(u_finit(standardFile, STANDARD_TEST_LOCALE, NULL));
+    TestFileFromICU(u_finit(standardFile, STANDARD_TEST_LOCALE, NULL), "u_finit STANDARD");
     fclose(standardFile);
 }
 
@@ -334,7 +362,7 @@ static void TestFadopt(void) {
 
     log_verbose("Testing u_fadopt\n");
     standardFile = fopen(STANDARD_TEST_FILE, "w");
-    TestFileFromICU(u_fadopt(standardFile, STANDARD_TEST_LOCALE, NULL));
+    TestFileFromICU(u_fadopt(standardFile, STANDARD_TEST_LOCALE, NULL), "u_fadopt STANDARD");
 }
 #endif
 

icu4c/source/test/iotest/iotest.cpp

@@ -889,15 +889,20 @@ int main(int argc, char* argv[])
     nerrors = runTestRequest(root, argc, argv);
 
 #if 1
+    static const char* filenamesToRemove[] = { STANDARD_TEST_FILE, MEDIUMNAME_TEST_FILE, LONGNAME_TEST_FILE, nullptr };
+    const char** filenamesToRemovePtr = filenamesToRemove;
+    const char* filenameToRemove;
+    while ((filenameToRemove = *filenamesToRemovePtr++) != nullptr)
     {
-        FILE* fileToRemove = fopen(STANDARD_TEST_FILE, "r");
+
+        FILE* fileToRemove = fopen(filenameToRemove, "r");
         /* This should delete any temporary files. */
         if (fileToRemove) {
             fclose(fileToRemove);
-            log_verbose("Deleting: %s\n", STANDARD_TEST_FILE);
-            if (remove(STANDARD_TEST_FILE) != 0) {
+            log_verbose("Deleting: %s\n", filenameToRemove);
+            if (remove(filenameToRemove) != 0) {
                 /* Maybe someone didn't close the file correctly. */
-                fprintf(stderr, "FAIL: Could not delete %s\n", STANDARD_TEST_FILE);
+                fprintf(stderr, "FAIL: Could not delete %s\n", filenameToRemove);
                 nerrors += 1;
             }
         }

icu4c/source/test/iotest/iotest.h

@@ -36,6 +36,8 @@ U_CDECL_BEGIN
 extern const UChar NEW_LINE[];
 extern const char C_NEW_LINE[];
 extern const char *STANDARD_TEST_FILE;
+extern const char *MEDIUMNAME_TEST_FILE;
+extern const char *LONGNAME_TEST_FILE;
 U_CDECL_END
 
 #define STANDARD_TEST_NUM_RANGE 1000