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

diff --git a/src/main/java/com/gitblit/StoredUserConfig.java b/src/main/java/com/gitblit/StoredUserConfig.java
index 63e1015ca2c1a98baa54c9343233e1a81d26dc9e..c8f93b20a5f437f01a0997070551a6e9bc5dbbcf 100644 (file)
--- a/src/main/java/com/gitblit/StoredUserConfig.java
+++ b/src/main/java/com/gitblit/StoredUserConfig.java
@@ -89,9 +89,48 @@ public class StoredUserConfig {
        }
 
        private static String escape(String value) {
-               String fixedValue = '#' == value.charAt(0) ? "\"" + value + "\"" : value;
-               fixedValue = fixedValue.replace("\\", "\\\\");
-               return fixedValue;
+               boolean quoteIt = false;
+               StringBuilder fixedValue = new StringBuilder(value.length() + 20);
+
+               for (char c : value.toCharArray()) {
+                       switch (c) {
+                               case '\n':
+                                       fixedValue.append("\\n");
+                                       break;
+
+                               case '\t':
+                                       fixedValue.append("\\t");
+                                       break;
+
+                               case '\b':
+                                       fixedValue.append("\\b");
+                                       break;
+
+                               case '\\':
+                                       fixedValue.append("\\\\");
+                                       break;
+
+                               case '"':
+                                       fixedValue.append("\\\"");
+                                       break;
+
+                               case ';':
+                               case '#':
+                                       quoteIt = true;
+                                       fixedValue.append(c);
+                                       break;
+
+                               default:
+                                       fixedValue.append(c);
+                                       break;
+                       }
+               }
+
+               if (quoteIt) {
+                       fixedValue.insert(0,"\"");
+                       fixedValue.append("\"");
+               }
+               return fixedValue.toString();
        }
 
        private static String generateKey(String key, String subKey) {

diff --git a/src/test/java/com/gitblit/StoredUserConfigTest.java b/src/test/java/com/gitblit/StoredUserConfigTest.java
index 01964617aad876e0315eb62b3a1d789995c88865..012139d300347fc836710fa8601e27d1bae09655 100644 (file)
--- a/src/test/java/com/gitblit/StoredUserConfigTest.java
+++ b/src/test/java/com/gitblit/StoredUserConfigTest.java
@@ -55,4 +55,153 @@ public class StoredUserConfigTest
         assertEquals("noone", cfg.getString("TEAM", null, "displayName"));
     }
 
+
+    @Test
+    public void testStringFields() throws Exception
+    {
+        StoredUserConfig config = new StoredUserConfig(file);
+        config.setString("USER", "admin", "password", "secret");
+        config.setString("USER", "admin", "displayName", "marusha");
+        config.setString("USER", "admin", "email", "name@example.com");
+
+        config.setString("USER", "other", "password", "password");
+        config.setString("USER", "other", "displayName", "mama");
+        config.setString("USER", "other", "email", "other@example.com");
+        config.setString("USER", "other", "repository", "RW+:repo1");
+        config.setString("USER", "other", "repository", "RW+:repo2");
+
+        config.setString("USER", null, "displayName", "default");
+
+        config.save();
+
+        StoredConfig cfg = new FileBasedConfig(file, FS.detect());
+        cfg.load();
+        assertEquals("secret", cfg.getString("USER", "admin", "password"));
+        assertEquals("marusha", cfg.getString("USER", "admin", "displayName"));
+        assertEquals("name@example.com", cfg.getString("USER", "admin", "email"));
+
+        assertEquals("password", cfg.getString("USER", "other", "password"));
+        assertEquals("mama", cfg.getString("USER", "other", "displayName"));
+        assertEquals("other@example.com", cfg.getString("USER", "other", "email"));
+
+        String[] stringList = cfg.getStringList("USER", "other", "repository");
+        assertNotNull(stringList);
+        assertEquals(2, stringList.length);
+        int i = 0;
+        for (String s : stringList) {
+            if (s.equalsIgnoreCase("RW+:repo1")) i += 1;
+            else if (s.equalsIgnoreCase("RW+:repo2")) i += 2;
+        }
+        assertEquals("Not all repository strings found", 3, i);
+
+        assertEquals("default", cfg.getString("USER", null, "displayName"));
+    }
+
+
+    @Test
+    public void testBooleanFields() throws Exception
+    {
+        StoredUserConfig config = new StoredUserConfig(file);
+        config.setBoolean("USER", "admin", "emailMeOnMyTicketChanges", true);
+        config.setBoolean("USER", "master", "emailMeOnMyTicketChanges", false);
+        config.setBoolean("TEAM", "admin", "excludeFromFederation", true);
+        config.setBoolean("USER", null, "excludeFromFederation", false);
+
+        config.save();
+
+        StoredConfig cfg = new FileBasedConfig(file, FS.detect());
+        cfg.load();
+        assertTrue(cfg.getBoolean("USER", "admin", "emailMeOnMyTicketChanges", false));
+        assertFalse(cfg.getBoolean("USER", "master", "emailMeOnMyTicketChanges", true));
+        assertTrue(cfg.getBoolean("TEAM", "admin", "excludeFromFederation", false));
+        assertFalse(cfg.getBoolean("USER", null, "excludeFromFederation", true));
+    }
+
+
+    @Test
+    public void testHashEscape() throws Exception
+    {
+        StoredUserConfig config = new StoredUserConfig(file);
+        config.setString("USER", "admin", "role", "#admin");
+
+        config.setString("USER", "other", "role", "#none");
+        config.setString("USER", "other", "displayName", "big#");
+        config.setString("USER", "other", "email", "user#name@home.de");
+
+        config.save();
+
+        StoredConfig cfg = new FileBasedConfig(file, FS.detect());
+        cfg.load();
+        assertEquals("#admin", cfg.getString("USER", "admin", "role"));
+        assertEquals("#none", cfg.getString("USER", "other", "role"));
+        assertEquals("big#", cfg.getString("USER", "other", "displayName"));
+        assertEquals("user#name@home.de", cfg.getString("USER", "other", "email"));
+    }
+
+
+    @Test
+    public void testCtrlEscape() throws Exception
+    {
+        StoredUserConfig config = new StoredUserConfig(file);
+        config.setString("USER", "name", "password", "bing\bbong");
+        config.setString("USER", "name", "role", "domain\\admin");
+        config.setString("USER", "name", "displayName", "horny\n\telephant");
+        config.setString("USER", "name", "org", "\tbig\tblue");
+        config.setString("USER", "name", "unit", "the end\n");
+
+        config.setString("USER", null, "unit", "the\ndefault");
+
+        config.save();
+
+        StoredConfig cfg = new FileBasedConfig(file, FS.detect());
+        cfg.load();
+        assertEquals("bing\bbong", cfg.getString("USER", "name", "password"));
+        assertEquals("domain\\admin", cfg.getString("USER", "name", "role"));
+        assertEquals("horny\n\telephant", cfg.getString("USER", "name", "displayName"));
+        assertEquals("\tbig\tblue", cfg.getString("USER", "name", "org"));
+        assertEquals("the end\n", cfg.getString("USER", "name", "unit"));
+
+        assertEquals("the\ndefault", cfg.getString("USER", null, "unit"));
+    }
+
+
+    @Test
+    public void testQuoteEscape() throws Exception
+    {
+        StoredUserConfig config = new StoredUserConfig(file);
+        config.setString("USER", "dude", "password", "going\"places");
+        config.setString("USER", "dude", "role", "\"dude\"");
+        config.setString("USER", "dude", "displayName", "John \"The Dude\" Lebowski");
+        config.setString("USER", "dude", "repo", "\"front matter");
+        config.setString("USER", "dude", "peepo", "leadout\"");
+
+        config.save();
+
+        StoredConfig cfg = new FileBasedConfig(file, FS.detect());
+        cfg.load();
+        assertEquals("going\"places", cfg.getString("USER", "dude", "password"));
+        assertEquals("\"dude\"", cfg.getString("USER", "dude", "role"));
+        assertEquals("John \"The Dude\" Lebowski", cfg.getString("USER", "dude", "displayName"));
+        assertEquals("\"front matter", cfg.getString("USER", "dude", "repo"));
+        assertEquals("leadout\"", cfg.getString("USER", "dude", "peepo"));
+    }
+
+
+    @Test
+    public void testUTF8() throws Exception
+    {
+        StoredUserConfig config = new StoredUserConfig(file);
+        config.setString("USER", "ming", "password", "一\t二\n三");
+        config.setString("USER", "ming", "displayName", "白老鼠");
+        config.setString("USER", "ming", "peepo", "Mickey \"白老鼠\" Whitfield");
+
+        config.save();
+
+        StoredConfig cfg = new FileBasedConfig(file, FS.detect());
+        cfg.load();
+        assertEquals("一\t二\n三", cfg.getString("USER", "ming", "password"));
+        assertEquals("白老鼠", cfg.getString("USER", "ming", "displayName"));
+        assertEquals("Mickey \"白老鼠\" Whitfield", cfg.getString("USER", "ming", "peepo"));
+    }
+
 }