@matheusbsbs / matheus-bagliano / commits / d2c84dad1c

Fork 0

mingw: refuse to access paths with trailing spaces or periods

When creating a directory on Windows whose path ends in a space or a period (or chains thereof), the Win32 API "helpfully" trims those. For example, `mkdir("abc ");` will return success, but actually create a directory called `abc` instead. This stems back to the DOS days, when all file names had exactly 8 characters plus exactly 3 characters for the file extension, and the only way to have shorter names was by padding with spaces. Sadly, this "helpful" behavior is a bit inconsistent: after a successful `mkdir("abc ");`, a `mkdir("abc /def")` will actually _fail_ (because the directory `abc ` does not actually exist). Even if it would work, we now have a serious problem because a Git repository could contain directories `abc` and `abc `, and on Windows, they would be "merged" unintentionally. As these paths are illegal on Windows, anyway, let's disallow any accesses to such paths on that Operating System. For practical reasons, this behavior is still guarded by the config setting `core.protectNTFS`: it is possible (and at least two regression tests make use of it) to create commits without involving the worktree. In such a scenario, it is of course possible -- even on Windows -- to create such file names. Among other consequences, this patch disallows submodules' paths to end in spaces on Windows (which would formerly have confused Git enough to try to write into incorrect paths, anyway). While this patch does not fix a vulnerability on its own, it prevents an attack vector that was exploited in demonstrations of a number of recently-fixed security bugs. The regression test added to `t/t7417-submodule-path-url.sh` reflects that attack vector. Note that we have to adjust the test case "prevent git~1 squatting on Windows" in `t/t7415-submodule-names.sh` because of a very subtle issue. It tries to clone two submodules whose names differ only in a trailing period character, and as a consequence their git directories differ in the same way. Previously, when Git tried to clone the second submodule, it thought that the git directory already existed (because on Windows, when you create a directory with the name `b.` it actually creates `b`), but with this patch, the first submodule's clone will fail because of the illegal name of the git directory. Therefore, when cloning the second submodule, Git will take a different code path: a fresh clone (without an existing git directory). Both code paths fail to clone the second submodule, both because the the corresponding worktree directory exists and is not empty, but the error messages are worded differently. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Johannes Schindelin committed Sep 5, 2019 at 13:27 UTC d2c84dad1c88f40906799bc879f70b965efd8ba6

8 files changed +123 -2

compat/mingw.c

+56 -1

@@ -333,6 +333,12 @@ int mingw_mkdir(const char *path, int mode)

333 {

334 int ret;

335 wchar_t wpath[MAX_PATH];

336 +

337 + if (!is_valid_win32_path(path)) {

338 + errno = EINVAL;

339 + return -1;

340 + }

341 +

342 if (xutftowcs_path(wpath, path) < 0)

343 return -1;

344 ret = _wmkdir(wpath);

@@ -345,13 +351,18 @@ int mingw_open (const char *filename, int oflags, ...)

351 {

352 va_list args;

353 unsigned mode;

348 - int fd;

354 + int fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);

355 wchar_t wfilename[MAX_PATH];

356

357 va_start(args, oflags);

358 mode = va_arg(args, int);

359 va_end(args);

360

361 + if (!is_valid_win32_path(filename)) {

362 + errno = create ? EINVAL : ENOENT;

363 + return -1;

364 + }

365 +

366 if (filename && !strcmp(filename, "/dev/null"))

367 filename = "nul";

368

@@ -413,6 +424,11 @@ FILE *mingw_fopen (const char *filename, const char *otype)

424 int hide = needs_hiding(filename);

425 FILE *file;

426 wchar_t wfilename[MAX_PATH], wotype[4];

427 + if (!is_valid_win32_path(filename)) {

428 + int create = otype && strchr(otype, 'w');

429 + errno = create ? EINVAL : ENOENT;

430 + return NULL;

431 + }

432 if (filename && !strcmp(filename, "/dev/null"))

433 filename = "nul";

434 if (xutftowcs_path(wfilename, filename) < 0 ||

@@ -435,6 +451,11 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream)

451 int hide = needs_hiding(filename);

452 FILE *file;

453 wchar_t wfilename[MAX_PATH], wotype[4];

454 + if (!is_valid_win32_path(filename)) {

455 + int create = otype && strchr(otype, 'w');

456 + errno = create ? EINVAL : ENOENT;

457 + return NULL;

458 + }

459 if (filename && !strcmp(filename, "/dev/null"))

460 filename = "nul";

461 if (xutftowcs_path(wfilename, filename) < 0 ||

@@ -2106,6 +2127,40 @@ static void setup_windows_environment(void)

2127 setenv("TERM", "cygwin", 1);

2128 }

2129

2130 +int is_valid_win32_path(const char *path)

2131 +{

2132 + int preceding_space_or_period = 0, i = 0, periods = 0;

2133 +

2134 + if (!protect_ntfs)

2135 + return 1;

2136 +

2137 + for (;;) {

2138 + char c = *(path++);

2139 + switch (c) {

2140 + case '\0':

2141 + case '/': case '\\':

2142 + /* cannot end in ` ` or `.`, except for `.` and `..` */

2143 + if (preceding_space_or_period &&

2144 + (i != periods || periods > 2))

2145 + return 0;

2146 + if (!c)

2147 + return 1;

2148 +

2149 + i = periods = preceding_space_or_period = 0;

2150 + continue;

2151 + case '.':

2152 + periods++;

2153 + /* fallthru */

2154 + case ' ':

2155 + preceding_space_or_period = 1;

2156 + i++;

2157 + continue;

2158 + }

2159 + preceding_space_or_period = 0;

2160 + i++;

2161 + }

2162 +}

2163 +

2164 /*

2165 * Disable MSVCRT command line wildcard expansion (__getmainargs called from

2166 * mingw startup code, see init.c in mingw runtime).

compat/mingw.h

+11

@@ -428,6 +428,17 @@ int mingw_offset_1st_component(const char *path);

428 #include <inttypes.h>

429 #endif

430

431 +/**

432 + * Verifies that the given path is a valid one on Windows.

433 + *

434 + * In particular, path segments are disallowed which end in a period or a

435 + * space (except the special directories `.` and `..`).

436 + *

437 + * Returns 1 upon success, otherwise 0.

438 + */

439 +int is_valid_win32_path(const char *path);

440 +#define is_valid_path(path) is_valid_win32_path(path)

441 +

442 /**

443 * Converts UTF-8 encoded string to UTF-16LE.

444 *

git-compat-util.h

@@ -370,6 +370,10 @@ static inline int git_offset_1st_component(const char *path)

370 #define offset_1st_component git_offset_1st_component

371 #endif

372

373 +#ifndef is_valid_path

374 +#define is_valid_path(path) 1

375 +#endif

376 +

377 #ifndef find_last_dir_sep

378 static inline char *git_find_last_dir_sep(const char *path)

379 {

read-cache.c

@@ -847,6 +847,9 @@ int verify_path(const char *path, unsigned mode)

847 if (has_dos_drive_prefix(path))

848 return 0;

849

850 + if (!is_valid_path(path))

851 + return 0;

852 +

853 goto inside;

854 for (;;) {

855 if (!c)

t/helper/test-path-utils.c

+17

@@ -386,6 +386,23 @@ int cmd_main(int argc, const char **argv)

386 if (argc > 1 && !strcmp(argv[1], "protect_ntfs_hfs"))

387 return !!protect_ntfs_hfs_benchmark(argc - 1, argv + 1);

388

389 + if (argc > 1 && !strcmp(argv[1], "is_valid_path")) {

390 + int res = 0, expect = 1, i;

391 +

392 + for (i = 2; i < argc; i++)

393 + if (!strcmp("--not", argv[i]))

394 + expect = 0;

395 + else if (expect != is_valid_path(argv[i]))

396 + res = error("'%s' is%s a valid path",

397 + argv[i], expect ? " not" : "");

398 + else

399 + fprintf(stderr,

400 + "'%s' is%s a valid path\n",

401 + argv[i], expect ? "" : " not");

402 +

403 + return !!res;

404 + }

405 +

406 fprintf(stderr, "%s: unknown function name: %s\n", argv[0],

407 argv[1] ? argv[1] : "(there was none)");

408 return 1;

t/t0060-path-utils.sh

+14

@@ -440,4 +440,18 @@ test_expect_success 'match .gitmodules' '

440 .gitmodules,:\$DATA

441 '

442

443 +test_expect_success MINGW 'is_valid_path() on Windows' '

444 + test-path-utils is_valid_path \

445 + win32 \

446 + "win32 x" \

447 + ../hello.txt \

448 + \

449 + --not \

450 + "win32 " \

451 + "win32 /x " \

452 + "win32." \

453 + "win32 . ." \

454 + .../hello.txt

455 +'

456 +

457 test_done

t/t7415-submodule-names.sh

+1 -1

@@ -102,7 +102,7 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '

102 ) &&

103 test_must_fail git -c core.protectNTFS=false \

104 clone --recurse-submodules squatting squatting-clone 2>err &&

105 - test_i18ngrep "directory not empty" err &&

105 + test_i18ngrep -e "directory not empty" -e "not an empty directory" err &&

106 ! grep gitdir squatting-clone/d/a/git~2

107 '

108

t/t7417-submodule-path-url.sh

+17

@@ -17,4 +17,21 @@ test_expect_success 'clone rejects unprotected dash' '

17 test_i18ngrep ignoring err

18 '

20 +test_expect_success MINGW 'submodule paths disallows trailing spaces' '

21 + git init super &&

22 + test_must_fail git -C super submodule add ../upstream "sub " &&

23 +

24 + : add "sub", then rename "sub" to "sub ", the hard way &&

25 + git -C super submodule add ../upstream sub &&

26 + tree=$(git -C super write-tree) &&

27 + git -C super ls-tree $tree >tree &&

28 + sed "s/sub/sub /" <tree >tree.new &&

29 + tree=$(git -C super mktree <tree.new) &&

30 + commit=$(echo with space | git -C super commit-tree $tree) &&

31 + git -C super update-ref refs/heads/master $commit &&

32 +

33 + test_must_fail git clone --recurse-submodules super dst 2>err &&

34 + test_i18ngrep "sub " err

35 +'

36 +

37 test_done