Skip to content

Commit 9b4afad

Browse files
flaixflaix
committed
fix: Fix StoredUserConfig not escaping control characters
The `StoredUserConfig` only escaped the escape character, i.e. backslash. But it does not escape control characters like tab or newline. This introduces a vulnerability where an attacker can create new entries in their user account and create new accounts. In addition, other characters are also not properly handled. Field values with a comment character need to be quoted. This only happens for the `#` character and only when the value starts with it. Also the quote is note escaped in values. This change completely rewrites the `escape` method of `StoredUserConfig`. It takes care of properly escaping characters that need escaping for the git configuration file format. This fixes #1410
1 parent 16ec6d0 commit 9b4afad

File tree

2 files changed

+191
-3
lines changed

2 files changed

+191
-3
lines changed

‎src/main/java/com/gitblit/StoredUserConfig.java‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,48 @@ private static void writeEntry(PrintWriter printWriter, String key, String value
8989
}
9090

9191
private static String escape(String value) {
92-
String fixedValue = '#' == value.charAt(0) ? "\"" + value + "\"" : value;
93-
fixedValue = fixedValue.replace("\\", "\\\\");
94-
return fixedValue;
92+
boolean quoteIt = false;
93+
StringBuilder fixedValue = new StringBuilder(value.length() + 20);
94+
95+
for (char c : value.toCharArray()) {
96+
switch (c) {
97+
case '\n':
98+
fixedValue.append("\\n");
99+
break;
100+
101+
case '\t':
102+
fixedValue.append("\\t");
103+
break;
104+
105+
case '\b':
106+
fixedValue.append("\\b");
107+
break;
108+
109+
case '\\':
110+
fixedValue.append("\\\\");
111+
break;
112+
113+
case '"':
114+
fixedValue.append("\\\"");
115+
break;
116+
117+
case ';':
118+
case '#':
119+
quoteIt = true;
120+
fixedValue.append(c);
121+
break;
122+
123+
default:
124+
fixedValue.append(c);
125+
break;
126+
}
127+
}
128+
129+
if (quoteIt) {
130+
fixedValue.insert(0,"\"");
131+
fixedValue.append("\"");
132+
}
133+
return fixedValue.toString();
95134
}
96135

97136
private static String generateKey(String key, String subKey) {

‎src/test/java/com/gitblit/StoredUserConfigTest.java‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,4 +55,153 @@ public void testSection() throws Exception
5555
assertEquals("noone", cfg.getString("TEAM", null, "displayName"));
5656
}
5757

58+
59+
@Test
60+
public void testStringFields() throws Exception
61+
{
62+
StoredUserConfig config = new StoredUserConfig(file);
63+
config.setString("USER", "admin", "password", "secret");
64+
config.setString("USER", "admin", "displayName", "marusha");
65+
config.setString("USER", "admin", "email", "name@example.com");
66+
67+
config.setString("USER", "other", "password", "password");
68+
config.setString("USER", "other", "displayName", "mama");
69+
config.setString("USER", "other", "email", "other@example.com");
70+
config.setString("USER", "other", "repository", "RW+:repo1");
71+
config.setString("USER", "other", "repository", "RW+:repo2");
72+
73+
config.setString("USER", null, "displayName", "default");
74+
75+
config.save();
76+
77+
StoredConfig cfg = new FileBasedConfig(file, FS.detect());
78+
cfg.load();
79+
assertEquals("secret", cfg.getString("USER", "admin", "password"));
80+
assertEquals("marusha", cfg.getString("USER", "admin", "displayName"));
81+
assertEquals("name@example.com", cfg.getString("USER", "admin", "email"));
82+
83+
assertEquals("password", cfg.getString("USER", "other", "password"));
84+
assertEquals("mama", cfg.getString("USER", "other", "displayName"));
85+
assertEquals("other@example.com", cfg.getString("USER", "other", "email"));
86+
87+
String[] stringList = cfg.getStringList("USER", "other", "repository");
88+
assertNotNull(stringList);
89+
assertEquals(2, stringList.length);
90+
int i = 0;
91+
for (String s : stringList) {
92+
if (s.equalsIgnoreCase("RW+:repo1")) i += 1;
93+
else if (s.equalsIgnoreCase("RW+:repo2")) i += 2;
94+
}
95+
assertEquals("Not all repository strings found", 3, i);
96+
97+
assertEquals("default", cfg.getString("USER", null, "displayName"));
98+
}
99+
100+
101+
@Test
102+
public void testBooleanFields() throws Exception
103+
{
104+
StoredUserConfig config = new StoredUserConfig(file);
105+
config.setBoolean("USER", "admin", "emailMeOnMyTicketChanges", true);
106+
config.setBoolean("USER", "master", "emailMeOnMyTicketChanges", false);
107+
config.setBoolean("TEAM", "admin", "excludeFromFederation", true);
108+
config.setBoolean("USER", null, "excludeFromFederation", false);
109+
110+
config.save();
111+
112+
StoredConfig cfg = new FileBasedConfig(file, FS.detect());
113+
cfg.load();
114+
assertTrue(cfg.getBoolean("USER", "admin", "emailMeOnMyTicketChanges", false));
115+
assertFalse(cfg.getBoolean("USER", "master", "emailMeOnMyTicketChanges", true));
116+
assertTrue(cfg.getBoolean("TEAM", "admin", "excludeFromFederation", false));
117+
assertFalse(cfg.getBoolean("USER", null, "excludeFromFederation", true));
118+
}
119+
120+
121+
@Test
122+
public void testHashEscape() throws Exception
123+
{
124+
StoredUserConfig config = new StoredUserConfig(file);
125+
config.setString("USER", "admin", "role", "#admin");
126+
127+
config.setString("USER", "other", "role", "#none");
128+
config.setString("USER", "other", "displayName", "big#");
129+
config.setString("USER", "other", "email", "user#name@home.de");
130+
131+
config.save();
132+
133+
StoredConfig cfg = new FileBasedConfig(file, FS.detect());
134+
cfg.load();
135+
assertEquals("#admin", cfg.getString("USER", "admin", "role"));
136+
assertEquals("#none", cfg.getString("USER", "other", "role"));
137+
assertEquals("big#", cfg.getString("USER", "other", "displayName"));
138+
assertEquals("user#name@home.de", cfg.getString("USER", "other", "email"));
139+
}
140+
141+
142+
@Test
143+
public void testCtrlEscape() throws Exception
144+
{
145+
StoredUserConfig config = new StoredUserConfig(file);
146+
config.setString("USER", "name", "password", "bing\bbong");
147+
config.setString("USER", "name", "role", "domain\\admin");
148+
config.setString("USER", "name", "displayName", "horny\n\telephant");
149+
config.setString("USER", "name", "org", "\tbig\tblue");
150+
config.setString("USER", "name", "unit", "the end\n");
151+
152+
config.setString("USER", null, "unit", "the\ndefault");
153+
154+
config.save();
155+
156+
StoredConfig cfg = new FileBasedConfig(file, FS.detect());
157+
cfg.load();
158+
assertEquals("bing\bbong", cfg.getString("USER", "name", "password"));
159+
assertEquals("domain\\admin", cfg.getString("USER", "name", "role"));
160+
assertEquals("horny\n\telephant", cfg.getString("USER", "name", "displayName"));
161+
assertEquals("\tbig\tblue", cfg.getString("USER", "name", "org"));
162+
assertEquals("the end\n", cfg.getString("USER", "name", "unit"));
163+
164+
assertEquals("the\ndefault", cfg.getString("USER", null, "unit"));
165+
}
166+
167+
168+
@Test
169+
public void testQuoteEscape() throws Exception
170+
{
171+
StoredUserConfig config = new StoredUserConfig(file);
172+
config.setString("USER", "dude", "password", "going\"places");
173+
config.setString("USER", "dude", "role", "\"dude\"");
174+
config.setString("USER", "dude", "displayName", "John \"The Dude\" Lebowski");
175+
config.setString("USER", "dude", "repo", "\"front matter");
176+
config.setString("USER", "dude", "peepo", "leadout\"");
177+
178+
config.save();
179+
180+
StoredConfig cfg = new FileBasedConfig(file, FS.detect());
181+
cfg.load();
182+
assertEquals("going\"places", cfg.getString("USER", "dude", "password"));
183+
assertEquals("\"dude\"", cfg.getString("USER", "dude", "role"));
184+
assertEquals("John \"The Dude\" Lebowski", cfg.getString("USER", "dude", "displayName"));
185+
assertEquals("\"front matter", cfg.getString("USER", "dude", "repo"));
186+
assertEquals("leadout\"", cfg.getString("USER", "dude", "peepo"));
187+
}
188+
189+
190+
@Test
191+
public void testUTF8() throws Exception
192+
{
193+
StoredUserConfig config = new StoredUserConfig(file);
194+
config.setString("USER", "ming", "password", "一\t二\n三");
195+
config.setString("USER", "ming", "displayName", "白老鼠");
196+
config.setString("USER", "ming", "peepo", "Mickey \"白老鼠\" Whitfield");
197+
198+
config.save();
199+
200+
StoredConfig cfg = new FileBasedConfig(file, FS.detect());
201+
cfg.load();
202+
assertEquals("一\t二\n三", cfg.getString("USER", "ming", "password"));
203+
assertEquals("白老鼠", cfg.getString("USER", "ming", "displayName"));
204+
assertEquals("Mickey \"白老鼠\" Whitfield", cfg.getString("USER", "ming", "peepo"));
205+
}
206+
58207
}

0 commit comments

Comments
 (0)