Skip to content

Commit dd36d29

Browse files
KojiNakamaruKojiNakamaru
committed
osxkeychain: avoid incorrectly skipping store operation
git-credential-osxkeychain skips storing a credential if its "get" action sets "state[]=osxkeychain:seen=1". This behavior was introduced in e1ab45b (osxkeychain: state to skip unnecessary store operations, 2024-05-15), which appeared in v2.46. However, this state[] persists even if a credential returned by "git-credential-osxkeychain get" is invalid and a subsequent helper's "get" operation returns a valid credential. Another subsequent helper (such as [1]) may expect git-credential-osxkeychain to store the valid credential, but the "store" operation is incorrectly skipped because it only checks "state[]=osxkeychain:seen=1". To solve this issue, "state[]=osxkeychain:seen" needs to contain enough information to identify whether the current "store" input matches the output from the previous "get" operation (and not a credential from another helper). Set "state[]=osxkeychain:seen" to a value encoding the credential output by "get", and compare it with a value encoding the credential input by "store". [1]: https://github.com/hickford/git-credential-oauth Reported-by: Petter Sælen <petter@saelen.eu> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
1 parent fd372d9 commit dd36d29

File tree

3 files changed

+132
-30
lines changed

3 files changed

+132
-30
lines changed

‎contrib/credential/osxkeychain/Makefile‎

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,65 @@
11
# The default target of this Makefile is...
22
all:: git-credential-osxkeychain
33

4+
include ../../../config.mak.uname
45
-include ../../../config.mak.autogen
56
-include ../../../config.mak
67

8+
ifdef ZLIB_NG
9+
BASIC_CFLAGS += -DHAVE_ZLIB_NG
10+
ifdef ZLIB_NG_PATH
11+
BASIC_CFLAGS += -I$(ZLIB_NG_PATH)/include
12+
EXTLIBS += $(call libpath_template,$(ZLIB_NG_PATH)/$(lib))
13+
endif
14+
EXTLIBS += -lz-ng
15+
else
16+
ifdef ZLIB_PATH
17+
BASIC_CFLAGS += -I$(ZLIB_PATH)/include
18+
EXTLIBS += $(call libpath_template,$(ZLIB_PATH)/$(lib))
19+
endif
20+
EXTLIBS += -lz
21+
endif
22+
ifndef NO_ICONV
23+
ifdef NEEDS_LIBICONV
24+
ifdef ICONVDIR
25+
BASIC_CFLAGS += -I$(ICONVDIR)/include
26+
ICONV_LINK = $(call libpath_template,$(ICONVDIR)/$(lib))
27+
else
28+
ICONV_LINK =
29+
endif
30+
ifdef NEEDS_LIBINTL_BEFORE_LIBICONV
31+
ICONV_LINK += -lintl
32+
endif
33+
EXTLIBS += $(ICONV_LINK) -liconv
34+
endif
35+
endif
36+
ifndef LIBC_CONTAINS_LIBINTL
37+
EXTLIBS += -lintl
38+
endif
39+
740
prefix ?= /usr/local
841
gitexecdir ?= $(prefix)/libexec/git-core
942

1043
CC ?= gcc
11-
CFLAGS ?= -g -O2 -Wall
44+
CFLAGS ?= -g -O2 -Wall -I../../.. $(BASIC_CFLAGS)
45+
LDFLAGS ?= $(BASIC_LDFLAGS) $(EXTLIBS)
1246
INSTALL ?= install
1347
RM ?= rm -f
1448

1549
%.o: %.c
1650
$(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<
1751

18-
git-credential-osxkeychain: git-credential-osxkeychain.o
52+
git-credential-osxkeychain: git-credential-osxkeychain.o ../../../libgit.a
1953
$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \
2054
-framework Security -framework CoreFoundation
2155

2256
install: git-credential-osxkeychain
2357
$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)
2458
$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)
2559

60+
../../../libgit.a:
61+
cd ../../..; make libgit.a
62+
2663
clean:
2764
$(RM) git-credential-osxkeychain git-credential-osxkeychain.o
2865

‎contrib/credential/osxkeychain/git-credential-osxkeychain.c‎

Lines changed: 92 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22
#include <string.h>
33
#include <stdlib.h>
44
#include <Security/Security.h>
5+
#include "git-compat-util.h"
6+
#include "strbuf.h"
7+
#include "wrapper.h"
58

69
#define ENCODING kCFStringEncodingUTF8
710
static CFStringRef protocol; /* Stores constant strings - not memory managed */
@@ -12,7 +15,7 @@ static CFStringRef username;
1215
static CFDataRef password;
1316
static CFDataRef password_expiry_utc;
1417
static CFDataRef oauth_refresh_token;
15-
static int state_seen;
18+
static char *state_seen;
1619

1720
static void clear_credential(void)
1821
{
@@ -48,27 +51,6 @@ static void clear_credential(void)
4851

4952
#define STRING_WITH_LENGTH(s) s, sizeof(s) - 1
5053

51-
__attribute__((format (printf, 1, 2), __noreturn__))
52-
static void die(const char *err, ...)
53-
{
54-
char msg[4096];
55-
va_list params;
56-
va_start(params, err);
57-
vsnprintf(msg, sizeof(msg), err, params);
58-
fprintf(stderr, "%s\n", msg);
59-
va_end(params);
60-
clear_credential();
61-
exit(1);
62-
}
63-
64-
static void *xmalloc(size_t len)
65-
{
66-
void *ret = malloc(len);
67-
if (!ret)
68-
die("Out of memory");
69-
return ret;
70-
}
71-
7254
static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
7355
{
7456
va_list args;
@@ -112,6 +94,66 @@ static void write_item(const char *what, const char *buf, size_t len)
11294
putchar('\n');
11395
}
11496

97+
static void write_item_strbuf(struct strbuf *sb, const char *what, const char *buf, int n)
98+
{
99+
char s[32];
100+
101+
xsnprintf(s, sizeof(s), "__%s=", what);
102+
strbuf_add(sb, s, strlen(s));
103+
strbuf_add(sb, buf, n);
104+
}
105+
106+
static void write_item_strbuf_cfstring(struct strbuf *sb, const char *what, CFStringRef ref)
107+
{
108+
char *buf;
109+
int len;
110+
111+
if (!ref)
112+
return;
113+
len = CFStringGetMaximumSizeForEncoding(CFStringGetLength(ref), ENCODING) + 1;
114+
buf = xmalloc(len);
115+
if (CFStringGetCString(ref, buf, len, ENCODING))
116+
write_item_strbuf(sb, what, buf, strlen(buf));
117+
free(buf);
118+
}
119+
120+
static void write_item_strbuf_cfnumber(struct strbuf *sb, const char *what, CFNumberRef ref)
121+
{
122+
short n;
123+
char buf[32];
124+
125+
if (!ref)
126+
return;
127+
if (!CFNumberGetValue(ref, kCFNumberShortType, &n))
128+
return;
129+
xsnprintf(buf, sizeof(buf), "%d", n);
130+
write_item_strbuf(sb, what, buf, strlen(buf));
131+
}
132+
133+
static void write_item_strbuf_cfdata(struct strbuf *sb, const char *what, CFDataRef ref)
134+
{
135+
char *buf;
136+
int len;
137+
138+
if (!ref)
139+
return;
140+
buf = (char *)CFDataGetBytePtr(ref);
141+
if (!buf || strlen(buf) == 0)
142+
return;
143+
len = CFDataGetLength(ref);
144+
write_item_strbuf(sb, what, buf, len);
145+
}
146+
147+
static void encode_state_seen(struct strbuf *sb)
148+
{
149+
strbuf_add(sb, "osxkeychain:seen=", strlen("osxkeychain:seen="));
150+
write_item_strbuf_cfstring(sb, "host", host);
151+
write_item_strbuf_cfnumber(sb, "port", port);
152+
write_item_strbuf_cfstring(sb, "path", path);
153+
write_item_strbuf_cfstring(sb, "username", username);
154+
write_item_strbuf_cfdata(sb, "password", password);
155+
}
156+
115157
static void find_username_in_item(CFDictionaryRef item)
116158
{
117159
CFStringRef account_ref;
@@ -124,6 +166,7 @@ static void find_username_in_item(CFDictionaryRef item)
124166
write_item("username", "", 0);
125167
return;
126168
}
169+
username = CFStringCreateCopy(kCFAllocatorDefault, account_ref);
127170

128171
username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
129172
if (username_buf)
@@ -163,6 +206,7 @@ static OSStatus find_internet_password(void)
163206
}
164207

165208
data = CFDictionaryGetValue(item, kSecValueData);
209+
password = CFDataCreateCopy(kCFAllocatorDefault, data);
166210

167211
write_item("password",
168212
(const char *)CFDataGetBytePtr(data),
@@ -173,7 +217,14 @@ static OSStatus find_internet_password(void)
173217
CFRelease(item);
174218

175219
write_item("capability[]", "state", strlen("state"));
176-
write_item("state[]", "osxkeychain:seen=1", strlen("osxkeychain:seen=1"));
220+
{
221+
struct strbuf sb;
222+
223+
strbuf_init(&sb, 1024);
224+
encode_state_seen(&sb);
225+
write_item("state[]", sb.buf, strlen(sb.buf));
226+
strbuf_release(&sb);
227+
}
177228

178229
out:
179230
CFRelease(attrs);
@@ -288,13 +339,22 @@ static OSStatus add_internet_password(void)
288339
CFDictionaryRef attrs;
289340
OSStatus result;
290341

291-
if (state_seen)
292-
return errSecSuccess;
293-
294342
/* Only store complete credentials */
295343
if (!protocol || !host || !username || !password)
296344
return -1;
297345

346+
if (state_seen) {
347+
struct strbuf sb;
348+
349+
strbuf_init(&sb, 1024);
350+
encode_state_seen(&sb);
351+
if (!strcmp(state_seen, sb.buf)) {
352+
strbuf_release(&sb);
353+
return errSecSuccess;
354+
}
355+
strbuf_release(&sb);
356+
}
357+
298358
data = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password);
299359
if (password_expiry_utc) {
300360
CFDataAppendBytes(data,
@@ -403,8 +463,9 @@ static void read_credential(void)
403463
(UInt8 *)v,
404464
strlen(v));
405465
else if (!strcmp(buf, "state[]")) {
406-
if (!strcmp(v, "osxkeychain:seen=1"))
407-
state_seen = 1;
466+
int len = strlen("osxkeychain:seen=");
467+
if (!strncmp(v, "osxkeychain:seen=", len))
468+
state_seen = xstrdup(v);
408469
}
409470
/*
410471
* Ignore other lines; we don't know what they mean, but
@@ -443,5 +504,8 @@ int main(int argc, const char **argv)
443504

444505
clear_credential();
445506

507+
if (state_seen)
508+
free(state_seen);
509+
446510
return 0;
447511
}

‎contrib/credential/osxkeychain/meson.build‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
executable('git-credential-osxkeychain',
22
sources: 'git-credential-osxkeychain.c',
33
dependencies: [
4+
libgit,
45
dependency('CoreFoundation'),
56
dependency('Security'),
67
],

0 commit comments

Comments
 (0)