1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
|
From 43803880b954a020dbffa5250a5b7fd893442c7c Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Thu, 12 Mar 2020 01:31:11 -0400
Subject: [PATCH 02/12] credential: detect unrepresentable values when parsing
urls
The credential protocol can't represent newlines in values, but URLs can
embed percent-encoded newlines in various components. A previous commit
taught the low-level writing routines to die() when encountering this,
but we can be a little friendlier to the user by detecting them earlier
and handling them gracefully.
This patch teaches credential_from_url() to notice such components,
issue a warning, and blank the credential (which will generally result
in prompting the user for a username and password). We blank the whole
credential in this case. Another option would be to blank only the
invalid component. However, we're probably better off not feeding a
partially-parsed URL result to a credential helper. We don't know how a
given helper would handle it, so we're better off to err on the side of
matching nothing rather than something unexpected.
The die() call in credential_write() is _probably_ impossible to reach
after this patch. Values should end up in credential structs only by URL
parsing (which is covered here), or by reading credential protocol input
(which by definition cannot read a newline into a value). But we should
definitely keep the low-level check, as it's our final and most accurate
line of defense against protocol injection attacks. Arguably it could
become a BUG(), but it probably doesn't matter much either way.
Note that the public interface of credential_from_url() grows a little
more than we need here. We'll use the extra flexibility in a future
patch to help fsck catch these cases.
Upstream-Status: Backport
Signed-off-by: Li Zhou <li.zhou@windriver.com>
---
credential.c | 36 ++++++++++++++++++++++++++++++++++--
credential.h | 16 ++++++++++++++++
t/t0300-credentials.sh | 12 ++++++++++--
3 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/credential.c b/credential.c
index a79aff0..2482382 100644
--- a/credential.c
+++ b/credential.c
@@ -324,7 +324,22 @@ void credential_reject(struct credential *c)
c->approved = 0;
}
-void credential_from_url(struct credential *c, const char *url)
+static int check_url_component(const char *url, int quiet,
+ const char *name, const char *value)
+{
+ if (!value)
+ return 0;
+ if (!strchr(value, '\n'))
+ return 0;
+
+ if (!quiet)
+ warning(_("url contains a newline in its %s component: %s"),
+ name, url);
+ return -1;
+}
+
+int credential_from_url_gently(struct credential *c, const char *url,
+ int quiet)
{
const char *at, *colon, *cp, *slash, *host, *proto_end;
@@ -338,7 +353,7 @@ void credential_from_url(struct credential *c, const char *url)
*/
proto_end = strstr(url, "://");
if (!proto_end)
- return;
+ return 0;
cp = proto_end + 3;
at = strchr(cp, '@');
colon = strchr(cp, ':');
@@ -373,4 +388,21 @@ void credential_from_url(struct credential *c, const char *url)
while (p > c->path && *p == '/')
*p-- = '\0';
}
+
+ if (check_url_component(url, quiet, "username", c->username) < 0 ||
+ check_url_component(url, quiet, "password", c->password) < 0 ||
+ check_url_component(url, quiet, "protocol", c->protocol) < 0 ||
+ check_url_component(url, quiet, "host", c->host) < 0 ||
+ check_url_component(url, quiet, "path", c->path) < 0)
+ return -1;
+
+ return 0;
+}
+
+void credential_from_url(struct credential *c, const char *url)
+{
+ if (credential_from_url_gently(c, url, 0) < 0) {
+ warning(_("skipping credential lookup for url: %s"), url);
+ credential_clear(c);
+ }
}
diff --git a/credential.h b/credential.h
index 6b0cd16..122a23c 100644
--- a/credential.h
+++ b/credential.h
@@ -28,7 +28,23 @@ struct credential {
int credential_read(struct credential *, FILE *);
void credential_write(const struct credential *, FILE *);
+
+/*
+ * Parse a url into a credential struct, replacing any existing contents.
+ *
+ * Ifthe url can't be parsed (e.g., a missing "proto://" component), the
+ * resulting credential will be empty but we'll still return success from the
+ * "gently" form.
+ *
+ * If we encounter a component which cannot be represented as a credential
+ * value (e.g., because it contains a newline), the "gently" form will return
+ * an error but leave the broken state in the credential object for further
+ * examination. The non-gentle form will issue a warning to stderr and return
+ * an empty credential.
+ */
void credential_from_url(struct credential *, const char *url);
+int credential_from_url_gently(struct credential *, const char *url, int quiet);
+
int credential_match(const struct credential *have,
const struct credential *want);
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 26f3c3a..b9c0f1f 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -308,9 +308,17 @@ test_expect_success 'empty helper spec resets helper list' '
EOF
'
-test_expect_success 'url parser rejects embedded newlines' '
- test_must_fail git credential fill <<-\EOF
+test_expect_success 'url parser ignores embedded newlines' '
+ check fill <<-EOF
url=https://one.example.com?%0ahost=two.example.com/
+ --
+ username=askpass-username
+ password=askpass-password
+ --
+ warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/
+ warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/
+ askpass: Username:
+ askpass: Password:
EOF
'
--
1.9.1
|
