Skip to content

Commit 76bd129

Browse files
gitstergitster
committed
Merge branch 'vd/fsck-submodule-url-test'
Tighten URL checks fsck makes in a URL recorded for submodules. * vd/fsck-submodule-url-test: submodule-config.c: strengthen URL fsck check t7450: test submodule urls test-submodule: remove command line handling for check-name submodule-config.h: move check_submodule_url
2 parents 12ee4ed + 8430b43 commit 76bd129

File tree

5 files changed

+203
-151
lines changed

5 files changed

+203
-151
lines changed

‎fsck.c‎

Lines changed: 0 additions & 133 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
#include "packfile.h"
2121
#include "submodule-config.h"
2222
#include "config.h"
23-
#include "credential.h"
2423
#include "help.h"
2524

2625
static ssize_t max_tree_entry_len = 4096;
@@ -1047,138 +1046,6 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,
10471046
return ret;
10481047
}
10491048

1050-
static int starts_with_dot_slash(const char *const path)
1051-
{
1052-
return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |
1053-
PATH_MATCH_XPLATFORM);
1054-
}
1055-
1056-
static int starts_with_dot_dot_slash(const char *const path)
1057-
{
1058-
return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |
1059-
PATH_MATCH_XPLATFORM);
1060-
}
1061-
1062-
static int submodule_url_is_relative(const char *url)
1063-
{
1064-
return starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);
1065-
}
1066-
1067-
/*
1068-
* Count directory components that a relative submodule URL should chop
1069-
* from the remote_url it is to be resolved against.
1070-
*
1071-
* In other words, this counts "../" components at the start of a
1072-
* submodule URL.
1073-
*
1074-
* Returns the number of directory components to chop and writes a
1075-
* pointer to the next character of url after all leading "./" and
1076-
* "../" components to out.
1077-
*/
1078-
static int count_leading_dotdots(const char *url, const char **out)
1079-
{
1080-
int result = 0;
1081-
while (1) {
1082-
if (starts_with_dot_dot_slash(url)) {
1083-
result++;
1084-
url += strlen("../");
1085-
continue;
1086-
}
1087-
if (starts_with_dot_slash(url)) {
1088-
url += strlen("./");
1089-
continue;
1090-
}
1091-
*out = url;
1092-
return result;
1093-
}
1094-
}
1095-
/*
1096-
* Check whether a transport is implemented by git-remote-curl.
1097-
*
1098-
* If it is, returns 1 and writes the URL that would be passed to
1099-
* git-remote-curl to the "out" parameter.
1100-
*
1101-
* Otherwise, returns 0 and leaves "out" untouched.
1102-
*
1103-
* Examples:
1104-
* http::https://example.com/repo.git -> 1, https://example.com/repo.git
1105-
* https://example.com/repo.git -> 1, https://example.com/repo.git
1106-
* git://example.com/repo.git -> 0
1107-
*
1108-
* This is for use in checking for previously exploitable bugs that
1109-
* required a submodule URL to be passed to git-remote-curl.
1110-
*/
1111-
static int url_to_curl_url(const char *url, const char **out)
1112-
{
1113-
/*
1114-
* We don't need to check for case-aliases, "http.exe", and so
1115-
* on because in the default configuration, is_transport_allowed
1116-
* prevents URLs with those schemes from being cloned
1117-
* automatically.
1118-
*/
1119-
if (skip_prefix(url, "http::", out) ||
1120-
skip_prefix(url, "https::", out) ||
1121-
skip_prefix(url, "ftp::", out) ||
1122-
skip_prefix(url, "ftps::", out))
1123-
return 1;
1124-
if (starts_with(url, "http://") ||
1125-
starts_with(url, "https://") ||
1126-
starts_with(url, "ftp://") ||
1127-
starts_with(url, "ftps://")) {
1128-
*out = url;
1129-
return 1;
1130-
}
1131-
return 0;
1132-
}
1133-
1134-
static int check_submodule_url(const char *url)
1135-
{
1136-
const char *curl_url;
1137-
1138-
if (looks_like_command_line_option(url))
1139-
return -1;
1140-
1141-
if (submodule_url_is_relative(url) || starts_with(url, "git://")) {
1142-
char *decoded;
1143-
const char *next;
1144-
int has_nl;
1145-
1146-
/*
1147-
* This could be appended to an http URL and url-decoded;
1148-
* check for malicious characters.
1149-
*/
1150-
decoded = url_decode(url);
1151-
has_nl = !!strchr(decoded, '\n');
1152-
1153-
free(decoded);
1154-
if (has_nl)
1155-
return -1;
1156-
1157-
/*
1158-
* URLs which escape their root via "../" can overwrite
1159-
* the host field and previous components, resolving to
1160-
* URLs like https::example.com/submodule.git and
1161-
* https:///example.com/submodule.git that were
1162-
* susceptible to CVE-2020-11008.
1163-
*/
1164-
if (count_leading_dotdots(url, &next) > 0 &&
1165-
(*next == ':' || *next == '/'))
1166-
return -1;
1167-
}
1168-
1169-
else if (url_to_curl_url(url, &curl_url)) {
1170-
struct credential c = CREDENTIAL_INIT;
1171-
int ret = 0;
1172-
if (credential_from_url_gently(&c, curl_url, 1) ||
1173-
!*c.host)
1174-
ret = -1;
1175-
credential_clear(&c);
1176-
return ret;
1177-
}
1178-
1179-
return 0;
1180-
}
1181-
11821049
struct fsck_gitmodules_data {
11831050
const struct object_id *oid;
11841051
struct fsck_options *options;

‎submodule-config.c‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
#include "parse-options.h"
1515
#include "thread-utils.h"
1616
#include "tree-walk.h"
17+
#include "url.h"
18+
#include "urlmatch.h"
1719

1820
/*
1921
* submodule cache lookup structure
@@ -228,6 +230,144 @@ int check_submodule_name(const char *name)
228230
return 0;
229231
}
230232

233+
static int starts_with_dot_slash(const char *const path)
234+
{
235+
return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |
236+
PATH_MATCH_XPLATFORM);
237+
}
238+
239+
static int starts_with_dot_dot_slash(const char *const path)
240+
{
241+
return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |
242+
PATH_MATCH_XPLATFORM);
243+
}
244+
245+
static int submodule_url_is_relative(const char *url)
246+
{
247+
return starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);
248+
}
249+
250+
/*
251+
* Count directory components that a relative submodule URL should chop
252+
* from the remote_url it is to be resolved against.
253+
*
254+
* In other words, this counts "../" components at the start of a
255+
* submodule URL.
256+
*
257+
* Returns the number of directory components to chop and writes a
258+
* pointer to the next character of url after all leading "./" and
259+
* "../" components to out.
260+
*/
261+
static int count_leading_dotdots(const char *url, const char **out)
262+
{
263+
int result = 0;
264+
while (1) {
265+
if (starts_with_dot_dot_slash(url)) {
266+
result++;
267+
url += strlen("../");
268+
continue;
269+
}
270+
if (starts_with_dot_slash(url)) {
271+
url += strlen("./");
272+
continue;
273+
}
274+
*out = url;
275+
return result;
276+
}
277+
}
278+
/*
279+
* Check whether a transport is implemented by git-remote-curl.
280+
*
281+
* If it is, returns 1 and writes the URL that would be passed to
282+
* git-remote-curl to the "out" parameter.
283+
*
284+
* Otherwise, returns 0 and leaves "out" untouched.
285+
*
286+
* Examples:
287+
* http::https://example.com/repo.git -> 1, https://example.com/repo.git
288+
* https://example.com/repo.git -> 1, https://example.com/repo.git
289+
* git://example.com/repo.git -> 0
290+
*
291+
* This is for use in checking for previously exploitable bugs that
292+
* required a submodule URL to be passed to git-remote-curl.
293+
*/
294+
static int url_to_curl_url(const char *url, const char **out)
295+
{
296+
/*
297+
* We don't need to check for case-aliases, "http.exe", and so
298+
* on because in the default configuration, is_transport_allowed
299+
* prevents URLs with those schemes from being cloned
300+
* automatically.
301+
*/
302+
if (skip_prefix(url, "http::", out) ||
303+
skip_prefix(url, "https::", out) ||
304+
skip_prefix(url, "ftp::", out) ||
305+
skip_prefix(url, "ftps::", out))
306+
return 1;
307+
if (starts_with(url, "http://") ||
308+
starts_with(url, "https://") ||
309+
starts_with(url, "ftp://") ||
310+
starts_with(url, "ftps://")) {
311+
*out = url;
312+
return 1;
313+
}
314+
return 0;
315+
}
316+
317+
int check_submodule_url(const char *url)
318+
{
319+
const char *curl_url;
320+
321+
if (looks_like_command_line_option(url))
322+
return -1;
323+
324+
if (submodule_url_is_relative(url) || starts_with(url, "git://")) {
325+
char *decoded;
326+
const char *next;
327+
int has_nl;
328+
329+
/*
330+
* This could be appended to an http URL and url-decoded;
331+
* check for malicious characters.
332+
*/
333+
decoded = url_decode(url);
334+
has_nl = !!strchr(decoded, '\n');
335+
336+
free(decoded);
337+
if (has_nl)
338+
return -1;
339+
340+
/*
341+
* URLs which escape their root via "../" can overwrite
342+
* the host field and previous components, resolving to
343+
* URLs like https::example.com/submodule.git and
344+
* https:///example.com/submodule.git that were
345+
* susceptible to CVE-2020-11008.
346+
*/
347+
if (count_leading_dotdots(url, &next) > 0 &&
348+
(*next == ':' || *next == '/'))
349+
return -1;
350+
}
351+
352+
else if (url_to_curl_url(url, &curl_url)) {
353+
int ret = 0;
354+
char *normalized = url_normalize(curl_url, NULL);
355+
if (normalized) {
356+
char *decoded = url_decode(normalized);
357+
if (strchr(decoded, '\n'))
358+
ret = -1;
359+
free(normalized);
360+
free(decoded);
361+
} else {
362+
ret = -1;
363+
}
364+
365+
return ret;
366+
}
367+
368+
return 0;
369+
}
370+
231371
static int name_and_item_from_var(const char *var, struct strbuf *name,
232372
struct strbuf *item)
233373
{

‎submodule-config.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,9 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value);
8989
*/
9090
int check_submodule_name(const char *name);
9191

92+
/* Returns 0 if the URL valid per RFC3986 and -1 otherwise. */
93+
int check_submodule_url(const char *url);
94+
9295
/*
9396
* Note: these helper functions exist solely to maintain backward
9497
* compatibility with 'fetch' and 'update_clone' storing configuration in

0 commit comments

Comments
 (0)